Re: [PATCH] depend.c: build up a dependency tree from c entities downto tokens: entries in the tree are: macro-depend: tree of #if nesting macro-expansions: possible macro expansion source of a token tok->macro-expansions->macro tok->macro-depend->ma

[Christopher Li <sparse@xxxxxxxxxxx>]

.
On Wed, Apr 25, 2012 at 1:16 PM, Konrad Eisele <eiselekd@xxxxxxxxx> wrote:
> Hello Christopher Li,
> I'd like to send a patch for review to be included
> into sparse. The patch builds up a dependency
> tree from c parse entities downto single token,
> making it possible to write tools that analyse what
> token was generated by what macro and which
> macros it is dependent of.

Sorry I just get back from a trip, did not have a chance to
write a reply during the travel.

I briefly look through the patch, haven't got enough time
to look at it in the detail level. Here is a few things that
jump out, I am not going to apply this patch as it is.

>> +void token_allocator_nofree(void)
>> +{
>> +       token_allocator.nofree = 1;
>> +}

What is up with this nofree thing?
There should be much better way to keep the token
unfree.

>> diff --git a/allocate.h b/allocate.h
>> index 9f1dc8c..de85320 100644
>> --- a/allocate.h
>> +++ b/allocate.h
>> @@ -12,6 +12,7 @@ struct allocator_struct {
>>        struct allocation_blob *blobs;
>>        unsigned int alignment;
>>        unsigned int chunking;
>> +       unsigned int nofree;

Nack here as well.

>> diff --git a/attr.c b/attr.c
>> new file mode 100644
>> index 0000000..54c4c21
>> --- /dev/null
>> +++ b/attr.c
>> +#define HASH_LEN (1024*4)
>> +struct hash {
>> +       struct hash_v *f;
>> +} h[HASH_LEN];
>> +
>> --- /dev/null
>> +++ b/depend.c
>> @@ -0,0 +1,329 @@
>> +/*
>> + * Build macros dependency tree
>> + * Copyright (C) 2012 Konrad Eisele <konrad@xxxxxxxxxxx,eiselekd@xxxxxxxxx>
>> + * BSD-License
>> + * Redistribution and use in source and binary forms are permitted
>> + * provided that the above copyright notice and this paragraph are
>> + * duplicated in all such forms and that any documentation,
>> + * advertising materials, and other materials related to such
>> + * distribution and use acknowledge that the software was developed
>> + * by the <organization>.  The name of the
>> + * University may not be used to endorse or promote products derived
>> + * from this software without specific prior written permission.
>> + * THIS SOFTWARE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
>> + * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
>> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
>> + */

Why BSD license? Sparse current is open software license and moving to
MIT license.

>> -               struct expression *e = alloc_expression(token->pos, EXPR_STATEMENT);
>> +               struct expression *e = alloc_expression_tok(token, EXPR_STATEMENT);

Please don't do change like this. It does not have any add on value.


There are a lot more I want to comment on but I need some time to think about
what is the best way to do it. I am not oppose to adding feature to track macro
dependency, but it need to be less intrusive to the code. I want to avoid impact
the normal code path that does not care about macro dependency. e.g. the
sparse checker.

I also suspect the hashing is unnecessary.  Give me some time to come up
with some suggestion.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html