Re: [PATCH 6/6] Forbid "the_index" in dir.c and unpack-trees.c
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Jun 6, 2018, 04:57 UTC
- Message-ID
- <CACsJy8CB4-vVNY7dhyv=2ZyZgbKb9XVgB9djxBbJKa9AtxvYVg@mail.gmail.com>
- In-Reply-To
- <20180605165826.GB158365@google.com>
On Tue, Jun 5, 2018 at 6:58 PM, Brandon Williams <bmwill@google.com> wrote:
Show 19 quoted lines
> On 06/05, Nguyễn Thái Ngọc Duy wrote: >> Use of global variables like the_index makes it very hard to follow >> the code flow, especially when it's buried deep in some minor helper >> function. >> >> We are gradually avoiding global variables, this hopefully will make >> it one step closer. The end game is, the set of "library" source files >> will have just one macro set: "LIB_CODE" (or something). This macro >> will prevent access to most (if not all) global variables in those >> files. We can't do that now, so we just prevent one thing at a time. >> >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> >> --- >> Should I keep my trick of defining the_index to >> the_index_should_not_be_used_here? It may help somewhat when people >> accidentally use the_index. > > We already have a set of index compatibility macros which this could > maybe be a part of.
I like it. Only attr.c and submodule.c fail to build if I make the_index declaration part of NO_THE_INDEX_COMPATIBILITY_MACROS. So I'm going to drop this patch for now, work on kicking the_index out of submodule.c and attr.c then move the_index decl there in a separate series.
Show 57 quoted lines
> Though if we want to go this way I think we should
> do the reverse of this and instead have GLOBAL_INDEX which must be
> defined in order to have access to the global. That way new files are
> already protected by this and it may be easier to reduce the number of
> these defines through our codebase than to add them to every single
> file.
>
>>
>> cache.h | 2 ++
>> dir.c | 2 ++
>> unpack-trees.c | 2 ++
>> 3 files changed, 6 insertions(+)
>>
>> diff --git a/cache.h b/cache.h
>> index 89a107a7f7..ecc96ccb0e 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -330,7 +330,9 @@ struct index_state {
>> struct ewah_bitmap *fsmonitor_dirty;
>> };
>>
>> +#ifndef NO_GLOBAL_INDEX
>> extern struct index_state the_index;
>> +#endif
>>
>> /* Name hashing */
>> extern int test_lazy_init_name_hash(struct index_state *istate, int try_threaded);
>> diff --git a/dir.c b/dir.c
>> index ccf8b4975e..74d848db5a 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -8,6 +8,8 @@
>> * Junio Hamano, 2005-2006
>> */
>> #define NO_THE_INDEX_COMPATIBILITY_MACROS
>> +/* Do not use the_index. You should have access to it via function arg */
>> +#define NO_GLOBAL_INDEX
>> #include "cache.h"
>> #include "config.h"
>> #include "dir.h"
>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index 3ace82ca27..9aebe9762b 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -1,4 +1,6 @@
>> #define NO_THE_INDEX_COMPATIBILITY_MACROS
>> +/* Do not use the_index here, you probably want o->src_index */
>> +#define NO_GLOBAL_INDEX
>> #include "cache.h"
>> #include "argv-array.h"
>> #include "repository.h"
>> --
>> 2.18.0.rc0.333.g22e6ee6cdf
>>
>
> --
> Brandon Williams-- Duy