Re: [PATCH] Policy Enforcement: config.h or squid.h includes

On 10/11/10 05:27, Alex Rousskov wrote:
> On 11/08/2010 09:59 PM, Amos Jeffries wrote:
>> On 09/11/10 10:21, Alex Rousskov wrote:
>>> On 11/06/2010 07:49 PM, Amos Jeffries wrote:
>>>> We currently have a policy that config.h MUST be included first so
>>>> as to
>>>> pull in the portability definitions early. It MAY be included via the
>>>> legacy squid.h at present.
>>>>
>>>> This alteration to the source maintenance script validates that each .c
>>>> and .cc file in the sources includes config.h or squid.h first in its
>>>> include order.
>>>>
>>>> * an ERROR: line is produces for each violating file
>>>> * as yet the maintenance run is not blocked so as to catch as many
>>>> errors as possible in one run.
>>>> * as yet there are no related code alterations performed by this
>>>> script.
>>>> It only validates.
>>>> * trace FORMAT: informative lines are silenced to make ERROR: more
>>>> visible.
>>>>
>>>> Unless there are objections I'll commit immediately.
>>>>
>>>> (see other Cleanup patch for fixes of the problems this already
>>>> detects).
>>>
>>> I think this check is too restrictive. As we discussed on a
>>> #include-order related thread, it should be OK to not include config.h
>>> or squid.h at all, as long as some other Squid header is included first.
>>> This "either config.h or another Squid header" rule should prevent the
>>> same kind of problems with less ink and fewer CPU build cycles.
>>>
>>> Alex.
>>
>> We came to the conclusion on 20th Oct that requiring it only in .c/.cc
>> was fewer inclusions (versus once via every single leaf .h). This patch
>> follows that conclusion and ignores the state of .h and .cci files.
>
> Sorry, I did not realize that you only check .c/.cc files. Please adjust
> the patch description (i.e., commit message), but the patch itself looks
> fine to me then.
>
>
>> The build testing of the other patch which adds config.h to all .c/.cc
>> this picked up has uncovered several bugs in some of those files. If
>> indirect inclusion was reliable that would have been a no-op.
>
> The failures you discover most likely prove that not all header files
> follow that indirect inclusion rule. It is not important since we will
> use a different rule going forward. It would be nice to remove
> "config.h" and "squid.h" from most the include files though. In fact,
> you can add a second case ${FILENAME} that will warn if somebody adds
> config.h to a regular Squid header.
>

Done and applied. Thanks.

Amos
Received on Wed Nov 10 2010 - 09:56:46 MST