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

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.

Thank you,

Alex.
Received on Tue Nov 09 2010 - 16:28:20 MST