Re: [PATCH v2] Support --disable-fixincludes.

[Jeff Law <jeffreyalaw@gmail.com>]

On 7/8/2022 5:14 AM, Martin Liška wrote:
> 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?
Once Alex is OK with this patch, then it'll be good to go.

jeff