{"thread":{"id":"59820","subject":"Problems with 592fc5b349","startedAt":"2023-06-01T13:47:47Z","lastAt":"2023-06-01T15:31:12Z","messageCount":3,"participants":["Alejandro R. Sedeño","Elijah Newren"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"477878","messageId":"CAOO-Oz2ua31xDOA9hdE-mMx3qwctDHK6Tu6AKdGc1_beuJMkwA@mail.gmail.com","threadId":"59820","inReplyTo":null,"subject":"Problems with 592fc5b349","fromName":"Alejandro R. Sedeño","fromEmail":"asedeno@mit.edu","sentAt":"2023-06-01T13:47:27Z","receivedAt":"2023-06-01T13:47:47Z","isPatch":false,"sender":{"key":"asedeno@mit.edu","avatar":"https://avatars.githubusercontent.com/u/28302?v=4"},"body":"592fc5b3495bf4ff17252d31109f1d9c0134684b moved backup definitions of\n\n  #define DT_\n\nfrom cache.h to dir.h, but did not include dir.h in cache.h despite those\n#defines being used there. Easy fix, `#include \"dir.h\"` in cache.h,\nwhich I'd submit as a patch, but then name-hash.c, which includes\ncache.h, which would now include dir.h, ends up with two definitions\nof `struct dir_entry`.\n\nSuggestions?\n\n-Alejandro\n"},{"id":"477880","messageId":"CABPp-BHKR5GP2NUFWDMSw-Pnra+yGP0kYAiwu-iWgtu66p-1RQ@mail.gmail.com","threadId":"59820","inReplyTo":"CAOO-Oz2ua31xDOA9hdE-mMx3qwctDHK6Tu6AKdGc1_beuJMkwA@mail.gmail.com","subject":"Re: Problems with 592fc5b349","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-06-01T14:33:26Z","receivedAt":"2023-06-01T14:33:49Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jun 1, 2023 at 6:47 AM Alejandro R. Sedeño <asedeno@mit.edu> wrote:\n>\n> 592fc5b3495bf4ff17252d31109f1d9c0134684b moved backup definitions of\n>\n>   #define DT_\n>\n> from cache.h to dir.h, but did not include dir.h in cache.h despite those\n> #defines being used there. Easy fix, `#include \"dir.h\"` in cache.h,\n> which I'd submit as a patch, but then name-hash.c, which includes\n> cache.h, which would now include dir.h, ends up with two definitions\n> of `struct dir_entry`.\n>\n> Suggestions?\n\nOh, interesting; none of our platform testing caught this.  After a\nlittle digging, I'm guessing you're on cygwin < 1.7?  However, I'm\nstill surprised you noticed, on any platform.  The only use of the\nDT_* defines in cache.h is in the inline function ce_to_dtype().  The\nonly places ce_to_dtype() is used are in (1) unpack-trees.c (which\nincludes both cache.h and dir.h) and (2) builtin/ls-files.c (which\nalso includes both cache.h and dir.h).  So, as far as I can tell, this\ncan't cause compilation issues anywhere.  How did you find this?\n\nIn commits in follow-on series, I moved this inline function to a new\nheader, read-cache.h.  name-cache.c does not end up including that\nheader, so we could add a #include \"dir.h\" directive to read-cache.h.\n\nAn alternative fix, if you need something for v2.41.0 (am I guessing\ncorrectly that you tried out v2.41.0 right after it's release and\nthat's when you found this?), would be to move the DT_ defines from\ndir.h to statinfo.h (a header included by both dir.h and cache.h).  Or\nperhaps another fix is to stop having two things in the codebase named\n\"struct dir_entry\", since it's bound to cause confusion for humans if\nnot also be a lurking timebomb for some future code file that needs\naccess to both.  But I still don't understand why any suggestions are\nneeded for an immediate fix, since all users of ce_to_dtype() should\nhave the necessary headers.  Is there an issue where \"inline\" is\nignored, and this function is being defined & compiled for every file\nthat includes cache.h, and then the linker removes the duplicates or\nsomething?\n"},{"id":"477881","messageId":"20230601152025.116126-1-asedeno@mit.edu","threadId":"59820","inReplyTo":"CABPp-BHKR5GP2NUFWDMSw-Pnra+yGP0kYAiwu-iWgtu66p-1RQ@mail.gmail.com","subject":"Re: Problems with 592fc5b349","fromName":"Alejandro R. Sedeño","fromEmail":"asedeno@mit.edu","sentAt":"2023-06-01T15:20:25Z","receivedAt":"2023-06-01T15:31:12Z","isPatch":false,"sender":{"key":"asedeno@mit.edu","avatar":"https://avatars.githubusercontent.com/u/28302?v=4"},"body":"On Thu, June 1, 2023 at 10:33AM Elijah Newren <newren@gmail.com> wrote:\n> Oh, interesting; none of our platform testing caught this.  After a\n> little digging, I'm guessing you're on cygwin < 1.7?  However, I'm\n> still surprised you noticed, on any platform.  The only use of the\n> DT_* defines in cache.h is in the inline function ce_to_dtype().  The\n> only places ce_to_dtype() is used are in (1) unpack-trees.c (which\n> includes both cache.h and dir.h) and (2) builtin/ls-files.c (which\n> also includes both cache.h and dir.h).  So, as far as I can tell, this\n> can't cause compilation issues anywhere.  How did you find this?\n\nI build on an ancient Solaris (5.10), for reasons. One day I'll give up\non it, but today is not that day.\n\n> In commits in follow-on series, I moved this inline function to a new\n> header, read-cache.h.  name-cache.c does not end up including that\n> header, so we could add a #include \"dir.h\" directive to read-cache.h.\n\n> An alternative fix, if you need something for v2.41.0 (am I guessing\n> correctly that you tried out v2.41.0 right after it's release and\n> that's when you found this?), would be to move the DT_ defines from\n> dir.h to statinfo.h (a header included by both dir.h and cache.h).\n\nYeah, I built v2.41.0 this morning and saw that my sun4x_510 build\nfailed with DT_REG not defined in cache.h while building\nadd-interactive.c I tried the patch I described earlier (add dir.h\nto cache.h) and ran into the duplicate `struct dir_entry` in\nname-hash.c. I'm testing a patch where I move DT_ definitions into\na new dtype.h, and include it where needed, but statinfo.h seems\nresonable.\n\n\n> ... Or\n> perhaps another fix is to stop having two things in the codebase named\n> \"struct dir_entry\", since it's bound to cause confusion for humans if\n> not also be a lurking timebomb for some future code file that needs\n> access to both.\n\nAgreed, though I did not want to pull on that particular thread for fear\nof what else might unravel.\n\n> ... But I still don't understand why any suggestions are\n> needed for an immediate fix, since all users of ce_to_dtype() should\n> have the necessary headers.  Is there an issue where \"inline\" is\n> ignored, and this function is being defined & compiled for every file\n> that includes cache.h, and then the linker removes the duplicates or\n> something?\n\nI could believe that gcc 3.4.3 (again, ancient), is not being as clever\nas newer compilers here.\n\n-Alejandro\n"}]}