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
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
[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
