Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Fri, Aug 7, 2015 at 10:13 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak karthik@gmail.com wrote:
On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak karthik@gmail.com wrote:
+static void apply_formatting_state(struct ref_formatting_state *state,
struct strbuf *final)
+{
+ /* More formatting options to be evetually added */
+ strbuf_addbuf(final, state-output);
+ strbuf_release(state-output);
I guess the idea here is that you intend state-output to be re-used
and it is convenient to clear it here rather than making that the
responsibility of each caller. For re-use, it is more typical to use
strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
it seems like a smarter way to around this without much overhead But it
was more of to release it as its no longer required unless another modifier
atom
is encountered. Is it worth keeping hoping for another modifier atom
eventually,
and release it at the end like you suggested below?
If I understand your question correctly, it sounds like you're asking
about a memory micro-optimization. From an architectural standpoint,
it's cleaner for the entity which allocates a resource to also release
it. In this case, show_ref_array_item() allocates the strbuf, thus it
should be the one to release it.
And, although we shouldn't be worrying about micro-optimizations at
this point, if it were to be an issue, resetting the strbuf via
strbuf_reset(), which doesn't involve slow memory
deallocation/reallocation, is likely to be a winner over repeated
strbuf_release().
Exactly what I was asking, thanks for explaining :D
+ memset(state, 0, sizeof(state));
+ state.quote_style = quote_style;
+ state.output = value;
It feels strange to assign a local variable reference to state.output,
and there's no obvious reason why you should need to do so. I would
have instead expected ref_format_state to be declared as:
struct ref_formatting_state {
int quote_style;
struct strbuf output;
};
and initialized as so:
memset(state, 0, sizeof(state));
state.quote_style = quote_style;
strbuf_init(state.output, 0);
This looks neater, thanks. It'll go along with the previous patch.
(In fact, the memset() isn't even necessary here since you're
initializing all fields explicitly, though perhaps you want the
memset() because a future patch adds more fields which are not
initialized explicitly?)
Yea the memset is needed for bit fields evnetually added in the future.
Perhaps move the memset() to the first patch which actually requires
it, where it won't be (effectively) dead code, as it becomes here once
you make the above change.
But why would I need it there, we need to only memset() the ref_formatting_state
which is introduced here. Also here it helps in setting the strbuf
within ref_formatting_state
to {0, 0, 0}.
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
- struct atom_value *atomv;
+ struct atom_value *atomv = NULL;
What is this change about?
To remove the warning about atomv being unassigned before usage.
Hmm, where were you seeing that warning? The first use of 'atomv'
following its declaration is in the get_ref_atom_value() below, and
(as far as the compiler knows) that should be setting its value.
I'll check this out.
ep = strchr(sp, ')');
- if (cp sp)
- emit(cp, sp, output);
+ if (cp sp) {
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
+ }
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2,
ep), atomv);
- print_value(atomv, quote_style, output);
+ process_formatting_state(atomv, state);
+ print_value(atomv, state);
+ apply_formatting_state(state, final_buf);
}
if (*cp) {
sp = cp + strlen(cp);
- emit(cp, sp, output);
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
I'm getting the feeling that these functions
(process_formatting_state, print_value, emit, apply_formatting_state)
are becoming misnamed (again) with the latest structural changes (but
perhaps I haven't read far enough into the series yet?).
process_formatting_state() is rather generic.
perhaps set_formatting_state()?
I don't know. I don't have a proper high-level overview of the
functionality yet to say if that is a good name or not (which is one
reason I didn't suggest an alternative).
Ah! okay.
print_value() and emit() both imply outputting something, but neither
does so anymore.
I think I'll append a to_state to each of them.
Meh.
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Fri, Aug 7, 2015 at 7:37 AM, Karthik Nayak karthik@gmail.com wrote:
On Fri, Aug 7, 2015 at 10:13 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak karthik@gmail.com wrote:
On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
It feels strange to assign a local variable reference to state.output,
and there's no obvious reason why you should need to do so. I would
have instead expected ref_format_state to be declared as:
struct ref_formatting_state {
int quote_style;
struct strbuf output;
};
and initialized as so:
memset(state, 0, sizeof(state));
state.quote_style = quote_style;
strbuf_init(state.output, 0);
(In fact, the memset() isn't even necessary here since you're
initializing all fields explicitly, though perhaps you want the
memset() because a future patch adds more fields which are not
initialized explicitly?)
Yea the memset is needed for bit fields evnetually added in the future.
Perhaps move the memset() to the first patch which actually requires
it, where it won't be (effectively) dead code, as it becomes here once
you make the above change.
But why would I need it there, we need to only memset() the
ref_formatting_state
which is introduced here. Also here it helps in setting the strbuf
within ref_formatting_state to {0, 0, 0}.
If you declare ref_formatting_state as shown above, and then
initialize it like so:
state.quote_style = quote_style;
strbuf_init(state.output, 0);
then (as of this patch) the structure is fully initialized because
you've initialized each member individually. Adding a memset() above
those two lines would be do-nothing -- it would be wasted code -- and
would likely confuse someone reading the code, specifically because
the code is do-nothing and has no value (in this patch). Making each
patch understandable is one of your goals when organizing the patch
series; if a patch confuses a reader (say, by doing unnecessary work),
then it isn't satisfying that goal.
As for the strbuf member, it's initialized explicitly via
strbuf_init(), so there's no value in having memset() first initialize
it to {0, 0, 0}. Again, that's wasted code.
In a later patch, when you add another ref_formatting_state member or
two, then you will need to initialize those members too. That
initialization may be in the form of explicit assignment to each
member, or it may be the memset() sledgehammer approach, but the
initialization for those members should be added in the patch which
introduces them.
It's true that the end result is the same. By the end of the series,
you'll have memset() above the 'state.quote' and 'state.output'
initialization lines to ensure that your various boolean fields and
whatnot are initialized to zero, but each patch should be
self-contained and make sense on its own, doing just the work that it
needs to do, and not doing unrelated work. For this patch, the
memset() is unrelated work. For the later patch in which you add more
fields to ref_formatting_state(), the memset() is work necessary to
satisfy that patch's objective, thus belongs there.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Fri, Aug 7, 2015 at 11:00 PM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Fri, Aug 7, 2015 at 7:37 AM, Karthik Nayak karthik@gmail.com wrote:
On Fri, Aug 7, 2015 at 10:13 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak karthik@gmail.com
wrote:
On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine sunsh...@sunshineco.com
wrote:
It feels strange to assign a local variable reference to state.output,
and there's no obvious reason why you should need to do so. I would
have instead expected ref_format_state to be declared as:
struct ref_formatting_state {
int quote_style;
struct strbuf output;
};
and initialized as so:
memset(state, 0, sizeof(state));
state.quote_style = quote_style;
strbuf_init(state.output, 0);
(In fact, the memset() isn't even necessary here since you're
initializing all fields explicitly, though perhaps you want the
memset() because a future patch adds more fields which are not
initialized explicitly?)
Yea the memset is needed for bit fields evnetually added in the future.
Perhaps move the memset() to the first patch which actually requires
it, where it won't be (effectively) dead code, as it becomes here once
you make the above change.
But why would I need it there, we need to only memset() the
ref_formatting_state
which is introduced here. Also here it helps in setting the strbuf
within ref_formatting_state to {0, 0, 0}.
If you declare ref_formatting_state as shown above, and then
initialize it like so:
state.quote_style = quote_style;
strbuf_init(state.output, 0);
then (as of this patch) the structure is fully initialized because
you've initialized each member individually. Adding a memset() above
those two lines would be do-nothing -- it would be wasted code -- and
would likely confuse someone reading the code, specifically because
the code is do-nothing and has no value (in this patch). Making each
patch understandable is one of your goals when organizing the patch
series; if a patch confuses a reader (say, by doing unnecessary work),
then it isn't satisfying that goal.
As for the strbuf member, it's initialized explicitly via
strbuf_init(), so there's no value in having memset() first initialize
it to {0, 0, 0}. Again, that's wasted code.
Yes, what you're saying is true, if we replace memset() with
state.quote_style = quote_style;
strbuf_init(state.output, 0);
That does replace the need for memset and make the patch
more self-explanatory.
In a later patch, when you add another ref_formatting_state member or
two, then you will need to initialize those members too. That
initialization may be in the form of explicit assignment to each
member, or it may be the memset() sledgehammer approach, but the
initialization for those members should be added in the patch which
introduces them.
It's true that the end result is the same. By the end of the series,
you'll have memset() above the 'state.quote' and 'state.output'
initialization lines to ensure that your various boolean fields and
whatnot are initialized to zero, but each patch should be
self-contained and make sense on its own, doing just the work that it
needs to do, and not doing unrelated work. For this patch, the
memset() is unrelated work. For the later patch in which you add more
fields to ref_formatting_state(), the memset() is work necessary to
satisfy that patch's objective, thus belongs there.
I understand what you're saying, it makes sense to have individual patches
only bring in changes related to the patch. This makes a lot of sense.
I will change it up. Thanks :)
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Thu, Aug 6, 2015 at 11:53 PM, Karthik Nayak karthik@gmail.com wrote:
On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak karthik@gmail.com wrote:
+static void apply_formatting_state(struct ref_formatting_state *state,
struct strbuf *final)
+{
+ /* More formatting options to be evetually added */
+ strbuf_addbuf(final, state-output);
+ strbuf_release(state-output);
I guess the idea here is that you intend state-output to be re-used
and it is convenient to clear it here rather than making that the
responsibility of each caller. For re-use, it is more typical to use
strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
it seems like a smarter way to around this without much overhead But it
was more of to release it as its no longer required unless another modifier
atom
is encountered. Is it worth keeping hoping for another modifier atom
eventually,
and release it at the end like you suggested below?
If I understand your question correctly, it sounds like you're asking
about a memory micro-optimization. From an architectural standpoint,
it's cleaner for the entity which allocates a resource to also release
it. In this case, show_ref_array_item() allocates the strbuf, thus it
should be the one to release it.
And, although we shouldn't be worrying about micro-optimizations at
this point, if it were to be an issue, resetting the strbuf via
strbuf_reset(), which doesn't involve slow memory
deallocation/reallocation, is likely to be a winner over repeated
strbuf_release().
+ memset(state, 0, sizeof(state));
+ state.quote_style = quote_style;
+ state.output = value;
It feels strange to assign a local variable reference to state.output,
and there's no obvious reason why you should need to do so. I would
have instead expected ref_format_state to be declared as:
struct ref_formatting_state {
int quote_style;
struct strbuf output;
};
and initialized as so:
memset(state, 0, sizeof(state));
state.quote_style = quote_style;
strbuf_init(state.output, 0);
This looks neater, thanks. It'll go along with the previous patch.
(In fact, the memset() isn't even necessary here since you're
initializing all fields explicitly, though perhaps you want the
memset() because a future patch adds more fields which are not
initialized explicitly?)
Yea the memset is needed for bit fields evnetually added in the future.
Perhaps move the memset() to the first patch which actually requires
it, where it won't be (effectively) dead code, as it becomes here once
you make the above change.
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
- struct atom_value *atomv;
+ struct atom_value *atomv = NULL;
What is this change about?
To remove the warning about atomv being unassigned before usage.
Hmm, where were you seeing that warning? The first use of 'atomv'
following its declaration is in the get_ref_atom_value() below, and
(as far as the compiler knows) that should be setting its value.
ep = strchr(sp, ')');
- if (cp sp)
- emit(cp, sp, output);
+ if (cp sp) {
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
+ }
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
- print_value(atomv, quote_style, output);
+ process_formatting_state(atomv, state);
+ print_value(atomv, state);
+ apply_formatting_state(state, final_buf);
}
if (*cp) {
sp = cp + strlen(cp);
- emit(cp, sp, output);
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
I'm getting the feeling that these functions
(process_formatting_state, print_value, emit, apply_formatting_state)
are becoming misnamed (again) with the latest structural changes (but
perhaps I haven't read far enough into the series yet?).
process_formatting_state() is rather generic.
perhaps set_formatting_state()?
I don't know. I don't have a proper high-level overview of the
functionality yet to say if that is a good name or not (which is one
reason I didn't suggest an alternative).
print_value() and emit() both imply outputting something, but neither
does so anymore.
I think I'll append a to_state to each of them.
Meh. print_value() might be better named format_value(). emit() might
become append_literal() or append_non_atom() or something.
apply_formatting_state() seems to be more about finalizing the
already-formatted output.
perform_state_formatting()? perhaps.
Dunno.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak karthik@gmail.com wrote:
Introduce a ref_formatting_state which will eventually hold the values
of modifier atoms. Implement this within ref-filter.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
diff --git a/ref-filter.c b/ref-filter.c
index 91482c9..2c074a1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1238,35 +1238,58 @@ static void emit(const char *cp, const char *ep,
struct strbuf *output)
+static void apply_formatting_state(struct ref_formatting_state *state,
struct strbuf *final)
+{
+ /* More formatting options to be evetually added */
I forgot to mention this in my earlier review of this patch:
s/evetually/eventually/
+ strbuf_addbuf(final, state-output);
+ strbuf_release(state-output);
+}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Fri, Aug 7, 2015 at 5:49 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak karthik@gmail.com wrote:
Introduce a ref_formatting_state which will eventually hold the values
of modifier atoms. Implement this within ref-filter.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
+static void apply_formatting_state(struct ref_formatting_state *state,
struct strbuf *final)
+{
+ /* More formatting options to be evetually added */
+ strbuf_addbuf(final, state-output);
+ strbuf_release(state-output);
I guess the idea here is that you intend state-output to be re-used
and it is convenient to clear it here rather than making that the
responsibility of each caller. For re-use, it is more typical to use
strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
[1]: http://article.gmane.org/gmane.comp.version-control.git/273094
it seems like a smarter way to around this without much overhead But it
was more of to release it as its no longer required unless another modifier atom
is encountered. Is it worth keeping hoping for another modifier atom eventually,
and release it at the end like you suggested below?
+}
+
void show_ref_array_item(struct ref_array_item *info, const char *format,
int quote_style)
{
const char *cp, *sp, *ep;
- struct strbuf output = STRBUF_INIT;
+ struct strbuf value = STRBUF_INIT;
+ struct strbuf final_buf = STRBUF_INIT;
+ struct ref_formatting_state state;
int i;
+ memset(state, 0, sizeof(state));
+ state.quote_style = quote_style;
+ state.output = value;
It feels strange to assign a local variable reference to state.output,
and there's no obvious reason why you should need to do so. I would
have instead expected ref_format_state to be declared as:
struct ref_formatting_state {
int quote_style;
struct strbuf output;
};
and initialized as so:
memset(state, 0, sizeof(state));
state.quote_style = quote_style;
strbuf_init(state.output, 0);
This looks neater, thanks. It'll go along with the previous patch.
(In fact, the memset() isn't even necessary here since you're
initializing all fields explicitly, though perhaps you want the
memset() because a future patch adds more fields which are not
initialized explicitly?)
Yea the memset is needed for bit fields evnetually added in the future.
This still allows re-use via strbuf_reset() mentioned above.
And, of course, you'd want to strbuf_release() it at the end of this
function where you're already releasing final_buf.
Addressed this above.
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
- struct atom_value *atomv;
+ struct atom_value *atomv = NULL;
What is this change about?
To remove the warning about atomv being unassigned before usage.
ep = strchr(sp, ')');
- if (cp sp)
- emit(cp, sp, output);
+ if (cp sp) {
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
+ }
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
- print_value(atomv, quote_style, output);
+ process_formatting_state(atomv, state);
+ print_value(atomv, state);
+ apply_formatting_state(state, final_buf);
}
if (*cp) {
sp = cp + strlen(cp);
- emit(cp, sp, output);
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
I'm getting the feeling that these functions
(process_formatting_state, print_value, emit, apply_formatting_state)
are becoming misnamed (again) with the latest structural changes (but
perhaps I haven't read far enough into the series yet?).
process_formatting_state() is rather generic.
perhaps set_formatting_state()?
print_value() and emit() both imply outputting something, but neither
does so anymore.
I think I'll append a to_state to each of them.
apply_formatting_state() seems to be more about finalizing the
already-formatted output.
perform_state_formatting()? perhaps.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 02/11] ref-filter: introduce ref_formatting_state
On Tue, Aug 4, 2015 at 8:42 AM, Karthik Nayak karthik@gmail.com wrote:
Introduce a ref_formatting_state which will eventually hold the values
of modifier atoms. Implement this within ref-filter.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
+static void apply_formatting_state(struct ref_formatting_state *state,
struct strbuf *final)
+{
+ /* More formatting options to be evetually added */
+ strbuf_addbuf(final, state-output);
+ strbuf_release(state-output);
I guess the idea here is that you intend state-output to be re-used
and it is convenient to clear it here rather than making that the
responsibility of each caller. For re-use, it is more typical to use
strbuf_reset() than strbuf_release() (though Junio may disagree[1]).
[1]: http://article.gmane.org/gmane.comp.version-control.git/273094
+}
+
void show_ref_array_item(struct ref_array_item *info, const char *format,
int quote_style)
{
const char *cp, *sp, *ep;
- struct strbuf output = STRBUF_INIT;
+ struct strbuf value = STRBUF_INIT;
+ struct strbuf final_buf = STRBUF_INIT;
+ struct ref_formatting_state state;
int i;
+ memset(state, 0, sizeof(state));
+ state.quote_style = quote_style;
+ state.output = value;
It feels strange to assign a local variable reference to state.output,
and there's no obvious reason why you should need to do so. I would
have instead expected ref_format_state to be declared as:
struct ref_formatting_state {
int quote_style;
struct strbuf output;
};
and initialized as so:
memset(state, 0, sizeof(state));
state.quote_style = quote_style;
strbuf_init(state.output, 0);
(In fact, the memset() isn't even necessary here since you're
initializing all fields explicitly, though perhaps you want the
memset() because a future patch adds more fields which are not
initialized explicitly?)
This still allows re-use via strbuf_reset() mentioned above.
And, of course, you'd want to strbuf_release() it at the end of this
function where you're already releasing final_buf.
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
- struct atom_value *atomv;
+ struct atom_value *atomv = NULL;
What is this change about?
ep = strchr(sp, ')');
- if (cp sp)
- emit(cp, sp, output);
+ if (cp sp) {
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
+ }
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
- print_value(atomv, quote_style, output);
+ process_formatting_state(atomv, state);
+ print_value(atomv, state);
+ apply_formatting_state(state, final_buf);
}
if (*cp) {
sp = cp + strlen(cp);
- emit(cp, sp, output);
+ emit(cp, sp, state);
+ apply_formatting_state(state, final_buf);
I'm getting the feeling that these functions
(process_formatting_state, print_value, emit, apply_formatting_state)
are becoming misnamed (again) with the latest structural changes (but
perhaps I haven't read far enough into the series yet?).
process_formatting_state() is rather generic.
print_value() and emit() both imply outputting something, but neither
does so anymore.
apply_formatting_state() seems to be more about finalizing the
already-formatted output.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
