* Problems with 592fc5b349
@ 2023-06-01 13:47 Alejandro R. Sedeño
2023-06-01 14:33 ` Elijah Newren
0 siblings, 1 reply; 3+ messages in thread
From: Alejandro R. Sedeño @ 2023-06-01 13:47 UTC (permalink / raw)
To: Git List; +Cc: newren
592fc5b3495bf4ff17252d31109f1d9c0134684b moved backup definitions of
#define DT_
from cache.h to dir.h, but did not include dir.h in cache.h despite those
#defines being used there. Easy fix, `#include "dir.h"` in cache.h,
which I'd submit as a patch, but then name-hash.c, which includes
cache.h, which would now include dir.h, ends up with two definitions
of `struct dir_entry`.
Suggestions?
-Alejandro
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Problems with 592fc5b349
2023-06-01 13:47 Problems with 592fc5b349 Alejandro R. Sedeño
@ 2023-06-01 14:33 ` Elijah Newren
2023-06-01 15:20 ` Alejandro R. Sedeño
0 siblings, 1 reply; 3+ messages in thread
From: Elijah Newren @ 2023-06-01 14:33 UTC (permalink / raw)
To: Alejandro R. Sedeño; +Cc: Git List
On Thu, Jun 1, 2023 at 6:47 AM Alejandro R. Sedeño <asedeno@mit.edu> wrote:
>
> 592fc5b3495bf4ff17252d31109f1d9c0134684b moved backup definitions of
>
> #define DT_
>
> from cache.h to dir.h, but did not include dir.h in cache.h despite those
> #defines being used there. Easy fix, `#include "dir.h"` in cache.h,
> which I'd submit as a patch, but then name-hash.c, which includes
> cache.h, which would now include dir.h, ends up with two definitions
> of `struct dir_entry`.
>
> Suggestions?
Oh, interesting; none of our platform testing caught this. After a
little digging, I'm guessing you're on cygwin < 1.7? However, I'm
still surprised you noticed, on any platform. The only use of the
DT_* defines in cache.h is in the inline function ce_to_dtype(). The
only places ce_to_dtype() is used are in (1) unpack-trees.c (which
includes both cache.h and dir.h) and (2) builtin/ls-files.c (which
also includes both cache.h and dir.h). So, as far as I can tell, this
can't cause compilation issues anywhere. How did you find this?
In commits in follow-on series, I moved this inline function to a new
header, read-cache.h. name-cache.c does not end up including that
header, so we could add a #include "dir.h" directive to read-cache.h.
An alternative fix, if you need something for v2.41.0 (am I guessing
correctly that you tried out v2.41.0 right after it's release and
that's when you found this?), would be to move the DT_ defines from
dir.h to statinfo.h (a header included by both dir.h and cache.h). Or
perhaps another fix is to stop having two things in the codebase named
"struct dir_entry", since it's bound to cause confusion for humans if
not also be a lurking timebomb for some future code file that needs
access to both. But I still don't understand why any suggestions are
needed for an immediate fix, since all users of ce_to_dtype() should
have the necessary headers. Is there an issue where "inline" is
ignored, and this function is being defined & compiled for every file
that includes cache.h, and then the linker removes the duplicates or
something?
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Problems with 592fc5b349
2023-06-01 14:33 ` Elijah Newren
@ 2023-06-01 15:20 ` Alejandro R. Sedeño
0 siblings, 0 replies; 3+ messages in thread
From: Alejandro R. Sedeño @ 2023-06-01 15:20 UTC (permalink / raw)
To: newren; +Cc: git
On Thu, June 1, 2023 at 10:33AM Elijah Newren <newren@gmail.com> wrote:
> Oh, interesting; none of our platform testing caught this. After a
> little digging, I'm guessing you're on cygwin < 1.7? However, I'm
> still surprised you noticed, on any platform. The only use of the
> DT_* defines in cache.h is in the inline function ce_to_dtype(). The
> only places ce_to_dtype() is used are in (1) unpack-trees.c (which
> includes both cache.h and dir.h) and (2) builtin/ls-files.c (which
> also includes both cache.h and dir.h). So, as far as I can tell, this
> can't cause compilation issues anywhere. How did you find this?
I build on an ancient Solaris (5.10), for reasons. One day I'll give up
on it, but today is not that day.
> In commits in follow-on series, I moved this inline function to a new
> header, read-cache.h. name-cache.c does not end up including that
> header, so we could add a #include "dir.h" directive to read-cache.h.
> An alternative fix, if you need something for v2.41.0 (am I guessing
> correctly that you tried out v2.41.0 right after it's release and
> that's when you found this?), would be to move the DT_ defines from
> dir.h to statinfo.h (a header included by both dir.h and cache.h).
Yeah, I built v2.41.0 this morning and saw that my sun4x_510 build
failed with DT_REG not defined in cache.h while building
add-interactive.c I tried the patch I described earlier (add dir.h
to cache.h) and ran into the duplicate `struct dir_entry` in
name-hash.c. I'm testing a patch where I move DT_ definitions into
a new dtype.h, and include it where needed, but statinfo.h seems
resonable.
> ... Or
> perhaps another fix is to stop having two things in the codebase named
> "struct dir_entry", since it's bound to cause confusion for humans if
> not also be a lurking timebomb for some future code file that needs
> access to both.
Agreed, though I did not want to pull on that particular thread for fear
of what else might unravel.
> ... But I still don't understand why any suggestions are
> needed for an immediate fix, since all users of ce_to_dtype() should
> have the necessary headers. Is there an issue where "inline" is
> ignored, and this function is being defined & compiled for every file
> that includes cache.h, and then the linker removes the duplicates or
> something?
I could believe that gcc 3.4.3 (again, ancient), is not being as clever
as newer compilers here.
-Alejandro
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-06-01 15:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-01 13:47 Problems with 592fc5b349 Alejandro R. Sedeño
2023-06-01 14:33 ` Elijah Newren
2023-06-01 15:20 ` Alejandro R. Sedeño
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).