| Message ID | 20230602192700.1548636-1-asedeno@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | statinfo.h: move DTYPE defines from dir.h | expand |
On Fri, Jun 2, 2023 at 12:27 PM Alejandro R Sedeño <asedeno@google.com> wrote: > > From: Alejandro R. Sedeño <asedeno@mit.edu> > > These definitions are used in cache.h, which can't include dir.h > without causing name-hash.c to have two definitions of > `struct dir_entry`. ...are _currently_ used in cache.h (your commit message is fine, just pointing it out for below...) > Both dir.h and cache.h include statinfo.h, and this seems a reasonable > place for these definitions. > > This change fixes a broken build issue on old SunOS. Maintainer note for Junio: en/header-split-cache-h-part-3 moves the inline functions in cache.h that use the DT_* defines, but that series should both textually and semantically merge cleanly with this change. (Just noting this for your peace of mind.) After en/header-split-cache-h-part-3 merges down, I might opt for a different fix (I'm still mulling it over), but that other fix isn't possible until cache.h is split up more. Alejandro's fix is the cleanest interim solution I can think of. Reviewed-by: Elijah Newren <newren@gmail.com>
"Alejandro R Sedeño" <asedeno@google.com> writes: > From: Alejandro R. Sedeño <asedeno@mit.edu> > > These definitions are used in cache.h, which can't include dir.h > without causing name-hash.c to have two definitions of > `struct dir_entry`. > > Both dir.h and cache.h include statinfo.h, and this seems a reasonable > place for these definitions. > > This change fixes a broken build issue on old SunOS. > > Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu> > Signed-off-by: Alejandro R Sedeño <asedeno@google.com> This is a bit unusual; do you want to publish both names (I am assuming that they are the same single person)? I thought somebody in the earlier discussion identified the topic that was problematic by bisecting. It is a shame to lose that. Perhaps it is a good idea to rephrase the beginning of the proposed commit log message to mention that, like 592fc5b3 (dir.h: move DTYPE defines from cache.h, 2023-04-22) moved DTYPE macros from cache.h to dir.h, but are still used by cache.h to implement ce_to_dtype(); but cache.h cannot include dir.h because ... or something? Why does name-hash.c end up with two definitions? Aren't we properly guarding against multiple inclusions with #ifndef __DIR_H__ #define __DIR_H__ ... struct dir_entry { ... }; #endif or is there something funny going on? Thanks.
On Fri, Jun 2, 2023 at 6:56 PM Junio C Hamano <gitster@pobox.com> wrote: > > Why does name-hash.c end up with two definitions? Aren't we > properly guarding against multiple inclusions with > > #ifndef __DIR_H__ > #define __DIR_H__ > ... > struct dir_entry { > ... > }; > #endif > > or is there something funny going on? There are two _different_ things named "struct dir_entry" in the codebase: dir.h:struct dir_entry { dir.h- unsigned int len; dir.h- char name[FLEX_ARRAY]; /* more */ dir.h-}; -- name-hash.c:struct dir_entry { name-hash.c- struct hashmap_entry ent; name-hash.c- struct dir_entry *parent; name-hash.c- int nr; name-hash.c- unsigned int namelen; name-hash.c- char name[FLEX_ARRAY]; name-hash.c-}; So, name-hash.c cannot include anything that includes dir.h.
Elijah Newren <newren@gmail.com> writes:
> There are two _different_ things named "struct dir_entry" in the codebase:
In the longer term, we should rename such a local type to avoid name
clashes with the global one. But I of course am OK to leave it
outside the topic to clean it up.
In any case, it is worth saying in the proposed log message why
name-hash cannot use cache.h if we make it include dir.h; it is easy
to do so (i.e. "it has its own 'dir_entry' that is used for other
purpose").
Thanks.
Sorry for the dupes; resending as plain-text as per list requirements. On Fri, Jun 2, 2023 at 9:56 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Alejandro R Sedeño" <asedeno@google.com> writes: > > > … > > Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu> > > Signed-off-by: Alejandro R Sedeño <asedeno@google.com> They are both me; it's my way of sticking with my historical personal address and still attaching my work address for work reasons. > > This is a bit unusual; do you want to publish both names (I am > assuming that they are the same single person)? > > I thought somebody in the earlier discussion identified the topic > that was problematic by bisecting. It is a shame to lose that. I identified it earlier, by inspection because the machine I build on is slow and this was easy to track down. > Perhaps it is a good idea to rephrase the beginning of the proposed > commit log message to mention that, like > > 592fc5b3 (dir.h: move DTYPE defines from cache.h, 2023-04-22) > moved DTYPE macros from cache.h to dir.h, but are still used > by cache.h to implement ce_to_dtype(); but cache.h cannot > include dir.h because ... > > or something? Happy to rephrase. -Alejandro
diff --git a/dir.h b/dir.h index 79b85a01ee..d65a40126c 100644 --- a/dir.h +++ b/dir.h @@ -641,18 +641,4 @@ static inline int starts_with_dot_dot_slash_native(const char *const path) return path_match_flags(path, what | PATH_MATCH_NATIVE); } -#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT) -#define DTYPE(de) ((de)->d_type) -#else -#undef DT_UNKNOWN -#undef DT_DIR -#undef DT_REG -#undef DT_LNK -#define DT_UNKNOWN 0 -#define DT_DIR 1 -#define DT_REG 2 -#define DT_LNK 3 -#define DTYPE(de) DT_UNKNOWN -#endif - #endif diff --git a/statinfo.h b/statinfo.h index e49e3054ea..fe8df633a4 100644 --- a/statinfo.h +++ b/statinfo.h @@ -21,4 +21,18 @@ struct stat_data { unsigned int sd_size; }; +#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT) +#define DTYPE(de) ((de)->d_type) +#else +#undef DT_UNKNOWN +#undef DT_DIR +#undef DT_REG +#undef DT_LNK +#define DT_UNKNOWN 0 +#define DT_DIR 1 +#define DT_REG 2 +#define DT_LNK 3 +#define DTYPE(de) DT_UNKNOWN +#endif + #endif