On 5/25/22 07:37, Alexandre Oliva wrote:
> On May 24, 2022, Martin Liška <mliska@suse.cz> wrote:
> 
>> Allways install limits.h and syslimits.h header files
>> to include folder.
> 
> typo: s/Allways/Always/

Hello.

Fixed.

> 
> I'm a little worried about this bit, too.  limitx.h includes
> "syslimits.h", mentioning it should be in the same directory.  Perhaps
> it could be left in include-fixed?

Well, I would like to go w/o include-fixed if the option --disable-fixincludes is used.

> 
> The patch also changes syslimits.h from either the fixincluded file or
> gsyslimits.h to use gsyslimits.h unconditionally, which seemed wrong at
> first.
> 
> Now I see how these two hunks work together: syslimits.h will now always
> #include_next <limits.h>, which will find it in include-fixed if it's
> there, and system header files otherwise.  Nice!, but IMHO the commit
> message could be a little more verbose on the extent of the change and
> why that (is supposed to) work.

Oh, to be honest I'm not fully familiar with how these 2 files work together.
Can you explain it to me so that I can adjust the changelog entry correspondingly?

> 
> 
> It also looks like install-mkheaders installs limits-related headers for
> when fixincludes runs; we could probably skip the whole thing if
> fixincludes is disabled, but I'm also worried about how the changes
> above might impact post-install fixincludes: if that installs
> gsyslimits.h et al in include-fixed while your changes moves it to
> include, headers might end up in a confusing state.  I haven't worked
> out whether that's safe, but there appears to be room for cleanups
> there.

I've check that 'make install-mkheaders' work fine w/ and w/o --disable-fixincludes
after the patch.

> 
> gcc/config/mips/t-sdemtk also places syslimits.h explicitly in include/
> rather than include-fixed/, as part of disabling fixincludes, which is
> good, but it could be cleaned up as well.

Can we do that as a follow-up patch?

> 
> I don't see other config fragments that might require adjustments, so I
> think the patch looks good; hopefully my worries are unjustified, and
> the cleanups it enables could be

Good.

> 
> 
> We still create the README file in there and install it, even with
> fixincludes disabled and thus unavailable, don't we?  That README then
> becomes misleading; we might be better off not installing it.

Sure, fixed in v2 of the patch.

> 
> 
>> When --disable-fixincludes is used, then no systen header files
>> are fixed by the tools in fixincludes. Moreover, the fixincludes
>> tools are not built any longer.
> 
> typo: s/systen/system/

Fixed.

> 
> 
> Could you please check that a post-install mkheaders still has a
> functional limits.h with these changes?

How do I check that, please?

> The patch is ok (with the typo
> fixes) if so.  The cleanups it enables would be welcome as separate
> patches ;-)

Can I install the v2?

Martin

> 
> Thanks!
>