On Mon, Aug 17, 2015 at 5:01 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> Introduce ref_formatting_state which will hold the formatted
>> output strbuf and is used for nesting of modifier atoms.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index edfb1c7..3259363 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -55,6 +55,12 @@ static struct {
>> { "color" },
>> };
>>
>> +struct ref_formatting_state {
>> + struct strbuf output;
>> + struct ref_formatting_state *prev;
>
> Upon initial read-through of this patch, I found the name 'prev'
> confusing since it seems you sometimes treat this as a linked-list
> and, for a linked-list, this member is customarily named 'next'.
> However, you also sometimes treat it as a stack, in which case 'prev'
> makes a certain amount of sense semantically, although so does 'next'.
> I'd probably have named it 'next', however, attr.c:struct attr_stack
> names its member 'prev', so there is some precedence for the current
> choice.
Its sort of a stack in all sense, I mean we get new elements and push it onto
the top and pop them once we're done. So that's why prev, also it's
more of pushing the
current state into the previously defined state.
>
> Also, it's customary for this member to be the first (or less
> frequently last) member in the structure.
>
Ok will do that.
>> + int quote_style;
>> +};
>> +
>> struct atom_value {
>> const char *s;
>> unsigned long ul; /* used for sorting when not FIELD_STR */
>> @@ -129,6 +135,26 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>> return at;
>> }
>>
>> +static struct ref_formatting_state *push_new_state(struct ref_formatting_state **state)
>
> This interface seems confused. The caller receives the new state as
> both the return value of the function and as an out-value of its sole
> argument. I'd suggest choosing one or the other.
>
> Which one you choose depends upon how you view the operation and the
> data structure. If you view it as linked-list manipulation, then you'd
> probably want the new state returned, and accept a pointer argument
> (rather than pointer-to-pointer). On the other hand, if you view it as
> a stack, then you'd probably want to have it return void and
> manipulate the sole argument. For this case, it might be clearer to
> name the argument 'stack' rather than 'state'.
>
I guess it makes to stick to the stack arrangement, will change the argument
name to stack.
>> +{
>> + struct ref_formatting_state *new_state = xcalloc(1, sizeof(struct ref_formatting_state));
>> + struct ref_formatting_state *tmp = *state;
>> +
>> + *state = new_state;
>> + new_state->prev = tmp;
>> + return new_state;
>> +}
>
> A couple issues:
>
> First, you need to initialize the strbuf member of
> ref_formatting_state after allocation:
>
> new_state = xcalloc(1, sizeof(struct ref_formatting_state));
> strbuf_init(&new_state->output, 0);
>
Was under the impression that strbuf_init() is not needed after a xcalloc,
had to look at that again.
> Second, if you re-order the code slightly, the 'tmp' variable becomes
> unnecessary.
>
> Assuming that your intention was to match pop_state() and treat this
> opaquely as a stack rather than exposing its linked-list
> implementation, I'd have expected the function to look something like
> this:
>
> static void push_new_state(struct ref_formatting_state **stack)
> {
> struct ref_formatting_state *s = xcalloc(...);
> s->next = *stack;
> strbuf_init(&s->output, 0);
> *stack = s;
> }
>
You mean:
static void push_new_state(struct ref_formatting_state **stack)
{
struct ref_formatting_state *s = xcalloc(...);
strbuf_init(&s->output, 0);
s->prev = *stack;
*stack = s;
}
>> +static void pop_state(struct ref_formatting_state **state)
>> +{
>> + struct ref_formatting_state *current = *state;
>> + struct ref_formatting_state *prev = current->prev;
>> +
>> + strbuf_release(¤t->output);
>> + free(current);
>> + *state = prev;
>> +}
>
> This interface suggests that you're treating it as an opaque stack, in
> which case naming the argument 'stack' might be clearer.
>
Will do thanks.
>> /*
>> * In a format string, find the next occurrence of %(atom).
>> */
>> @@ -1195,23 +1221,23 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
>> qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
>> }
>>
>> -static void append_atom(struct atom_value *v, int quote_style, struct strbuf *output)
>> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
>> {
>> - switch (quote_style) {
>> + switch (state->quote_style) {
>> case QUOTE_NONE:
>> - strbuf_addstr(output, v->s);
>> + strbuf_addstr(&state->output, v->s);
>> break;
>> case QUOTE_SHELL:
>> - sq_quote_buf(output, v->s);
>> + sq_quote_buf(&state->output, v->s);
>> break;
>> case QUOTE_PERL:
>> - perl_quote_buf(output, v->s);
>> + perl_quote_buf(&state->output, v->s);
>> break;
>> case QUOTE_PYTHON:
>> - python_quote_buf(output, v->s);
>> + python_quote_buf(&state->output, v->s);
>> break;
>> case QUOTE_TCL:
>> - tcl_quote_buf(output, v->s);
>> + tcl_quote_buf(&state->output, v->s);
>> break;
>> }
>> }
>
> This patch touches all the same lines as the previous patch which
> converted the code to append to a strbuf rather than emit to stdout,
> thus it makes the previous patch seem wasted and the current patch
> much noisier. As such, it might make sense to repartition these two
> patches as so:
>
> patch 2: Introduce ref_formatting_state (but without the 'prev'
> field), and update callers to append to its 'output' member rather
> than emitting to stdout.
>
> patch 3: Introduce the ref_formatting_state stack machinery; this
> patch would also add the 'prev' field.
>
This sounds good, will do this.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html