On Wed, Nov 9, 2016 at 4:52 AM, Jacob Keller <jacob.keller@xxxxxxxxx> wrote:
> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>
> Ok. How does this handle whitespace? The previous if implementation
> treated whitespace as trimming to ignore. Does this require an exact
> whitespace match? It appears by the code that strings must match
> exactly. Would it make more sense to always trim the value of
> whitespace first before comparison? Hmm.. I think we should avoid
> doing that actually.
>
This does not trim whitespace what so ever and requires an exact match.
I don't see the reason behind trimming whitespace though.
>> In addition to the above, for commit and tag objects, the header
>> field names (`tree`, `parent`, `object`, `type`, and `tag`) can
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 8392303..44481c3 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -22,6 +22,8 @@ struct align {
>> };
>>
>> struct if_then_else {
>> + const char *if_equals,
>> + *not_equals;
>
> Ok so we add both if_equals and not_equals values. Could we re-use the
> same string?
If we use an union then we'll be saving space, but that comes at the
disadvantage that
we'd need an indicator to show which subatom we're using (i.e
if_equals or not_equals)
for the checks made in the code below.
At the expense of keeping two pointers we avoid the extra indicator
while keeping the code
elegant. Either ways not too keen on this, up for discussion :)
>
>> unsigned int then_atom_seen : 1,
>> else_atom_seen : 1,
>> condition_satisfied : 1;
>> @@ -49,6 +51,10 @@ static struct used_atom {
>> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>> unsigned int nlines;
>> } contents;
>> + struct {
>> + const char *if_equals,
>> + *not_equals;
>
>
> Same here, why do we need both strings here stored separately? Could
> we instead store which state to check and store the string once? I'm
> not sure that really buys us any storage.
>
same as above, we utilize the fact that its easier to check the strings like
if (if_equals)
...
else (not_equals)
...
else we'd have to have an indicator to know which to check.
--
Regards,
Karthik Nayak