[PATCH 6/6] Forbid "the_index" in dir.c and unpack-trees.c
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 --- 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. 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
Re: [PATCH 6/6] Forbid "the_index" in dir.c and unpack-trees.c
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 > --- > 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. 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
Re: [PATCH 6/6] Forbid "the_index" in dir.c and unpack-trees.c
On Tue, Jun 5, 2018 at 6:58 PM, Brandon Williams wrote: > 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 >> --- >> 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. > 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