[03/17] cache-tree.c: avoid reusing the DEBUG constant
diff mbox series

Message ID 486297ec8c146e0ed47cd1dd8fe8f6496c2b54c2.1560860634.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Fix MSVC support, at long last
Related show

Commit Message

Junio C Hamano via GitGitGadget June 18, 2019, 12:23 p.m. UTC
From: Jeff Hostetler <jeffhost@microsoft.com>

In MSVC, the DEBUG constant is set automatically whenever compiling with
debug information.

This is clearly not what was intended in cache-tree.c, so let's use a less
ambiguous constant there.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 cache-tree.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Eric Sunshine June 18, 2019, 11:14 p.m. UTC | #1
On Tue, Jun 18, 2019 at 8:24 AM Jeff Hostetler via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> In MSVC, the DEBUG constant is set automatically whenever compiling with
> debug information.
>
> This is clearly not what was intended in cache-tree.c, so let's use a less
> ambiguous constant there.

s/constant/macro name/ would be clearer.

> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Carlo Marcelo Arenas Belón June 19, 2019, 12:08 a.m. UTC | #2
shouldn't this also be a problem with builtin/blame.c?

Carlo
Johannes Schindelin June 19, 2019, 11:17 a.m. UTC | #3
Hi Carlo,

On Tue, 18 Jun 2019, Carlo Arenas wrote:

> shouldn't this also be a problem with builtin/blame.c?

Sharp eyes!

It does not *really* matter as much here, as that file defines that
`DEBUG` constant if it has not yet been defined, but yes, it assumes that
*if* it is defined, then it is set to `1`. Which is the case, but it is
fragile.

I changed it locally already, and it will be part of the next iteration.

Thanks,
Johannes
Johannes Schindelin June 19, 2019, 11:19 a.m. UTC | #4
Hi Eric,

On Tue, 18 Jun 2019, Eric Sunshine wrote:

> On Tue, Jun 18, 2019 at 8:24 AM Jeff Hostetler via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > In MSVC, the DEBUG constant is set automatically whenever compiling with
> > debug information.
> >
> > This is clearly not what was intended in cache-tree.c, so let's use a less
> > ambiguous constant there.
>
> s/constant/macro name/ would be clearer.

To me, "macro" always sounds as if it referred to executable code, or at
least to something that expands to code.

I went with s/constant/name/ instead.

Thanks,
Dscho

>
> > Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
Junio C Hamano June 19, 2019, 3:21 p.m. UTC | #5
Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Jun 18, 2019 at 8:24 AM Jeff Hostetler via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> In MSVC, the DEBUG constant is set automatically whenever compiling with
>> debug information.
>>
>> This is clearly not what was intended in cache-tree.c, so let's use a less
>> ambiguous constant there.
>
> s/constant/macro name/ would be clearer.

It is closer to the standard-kosher terminology, I would think; if
somebody is in more pedantic mood, "C preprocessor macro" is
probably the phrase to use.
Carlo Marcelo Arenas Belón June 19, 2019, 5:15 p.m. UTC | #6
while those two changes (from DEBUG to DEBUG_$foo) are worth doing in
their own merit, I am more inclined to consider this as orthogonal
since by your own description[1] the right name to use would be _DEBUG
(with a preceding dash) and that would obviously not conflict here.

the only remaining change then would be to drop the -DDEBUG that gets
added to your BASIC_CFLAGS

Carlo
[1] https://docs.microsoft.com/en-us/cpp/build/reference/md-mt-ld-use-run-time-library?view=vs-2019
Johannes Schindelin June 19, 2019, 9:03 p.m. UTC | #7
Hi Carlo,

On Wed, 19 Jun 2019, Carlo Arenas wrote:

> while those two changes (from DEBUG to DEBUG_$foo) are worth doing in
> their own merit, I am more inclined to consider this as orthogonal
> since by your own description[1] the right name to use would be _DEBUG
> (with a preceding dash) and that would obviously not conflict here.

Well, both `_DEBUG` and `DEBUG` appear to be defined by Visual Studio in
debug mode. So we still need to rename them, and I'd rather rename them
here than in the next patch series (that will (re-)add support to build in
Visual Studio), as I want the command-line MSVC build to also define both
constants.

> the only remaining change then would be to drop the -DDEBUG that gets
> added to your BASIC_CFLAGS

No, for consistency with Visual Studio, I want to keep defining both
`DEBUG` and `_DEBUG`. It's just easier that way, as I won't have to spend
extra cycles to remember that those two ways to build Git are slightly
different, when they shouldn't be.

Thanks,
Dscho

> Carlo
> [1] https://docs.microsoft.com/en-us/cpp/build/reference/md-mt-ld-use-run-time-library?view=vs-2019
>
>

Patch
diff mbox series

diff --git a/cache-tree.c b/cache-tree.c
index b13bfaf71e..706ffcf188 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -6,8 +6,8 @@ 
 #include "object-store.h"
 #include "replace-object.h"
 
-#ifndef DEBUG
-#define DEBUG 0
+#ifndef DEBUG_CACHE_TREE
+#define DEBUG_CACHE_TREE 0
 #endif
 
 struct cache_tree *cache_tree(void)
@@ -111,7 +111,7 @@  static int do_invalidate_path(struct cache_tree *it, const char *path)
 	int namelen;
 	struct cache_tree_sub *down;
 
-#if DEBUG
+#if DEBUG_CACHE_TREE
 	fprintf(stderr, "cache-tree invalidate <%s>\n", path);
 #endif
 
@@ -398,7 +398,7 @@  static int update_one(struct cache_tree *it,
 		strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
 		strbuf_add(&buffer, oid->hash, the_hash_algo->rawsz);
 
-#if DEBUG
+#if DEBUG_CACHE_TREE
 		fprintf(stderr, "cache-tree update-one %o %.*s\n",
 			mode, entlen, path + baselen);
 #endif
@@ -421,7 +421,7 @@  static int update_one(struct cache_tree *it,
 
 	strbuf_release(&buffer);
 	it->entry_count = to_invalidate ? -1 : i - *skip_count;
-#if DEBUG
+#if DEBUG_CACHE_TREE
 	fprintf(stderr, "cache-tree update-one (%d ent, %d subtree) %s\n",
 		it->entry_count, it->subtree_nr,
 		oid_to_hex(&it->oid));
@@ -462,7 +462,7 @@  static void write_one(struct strbuf *buffer, struct cache_tree *it,
 	strbuf_add(buffer, path, pathlen);
 	strbuf_addf(buffer, "%c%d %d\n", 0, it->entry_count, it->subtree_nr);
 
-#if DEBUG
+#if DEBUG_CACHE_TREE
 	if (0 <= it->entry_count)
 		fprintf(stderr, "cache-tree <%.*s> (%d ent, %d subtree) %s\n",
 			pathlen, path, it->entry_count, it->subtree_nr,
@@ -536,7 +536,7 @@  static struct cache_tree *read_one(const char **buffer, unsigned long *size_p)
 		size -= rawsz;
 	}
 
-#if DEBUG
+#if DEBUG_CACHE_TREE
 	if (0 <= it->entry_count)
 		fprintf(stderr, "cache-tree <%s> (%d ent, %d subtree) %s\n",
 			*buffer, it->entry_count, subtree_nr,