Re: The order of #include statements

On 10/14/2010 08:41 PM, Amos Jeffries wrote:
> On 15/10/10 06:09, Alex Rousskov wrote:
>> Hello,
>>
>> We require that #include statements are alphabetized, with certain
>> exceptions. The presence of old files with out-of-order #includes makes
>> it difficult to enforce this policy because it is often difficult to say
>> whether the #includes were already 100% ordered without ordering them
>> first and then comparing the results -- a time consuming process.
>>
>> Also, it is not clear what the alphabetical order is, exactly, when
>> files contain non-alphabet characters such as '.' and '_'. The "natural"
>> order may differ from what a sort(1) command would produce, for example.
>> There is also a question of case-sensitivity.
>>
>> Finally, it is not clear how conditional #include statements should be
>> handled, especially when there are multiple #includes under a single
>> #ifdef condition.
>>
>> I suggest that we automate this instead of doing everything manually. It
>> should not be very difficult:
>>
>> 1. #include "" goes before any #include <>.
>>
>> 2. do not touch the inner #include <> order. Move <> includes carefully
>> as they may have #ifdef protections. It is probably easier and safer to
>> move "" includes only. No "" includes should be included conditionally
>> (this will require a few code changes).
>
> The current policy (which has not been held to) is to wrap all <>
> includes and test for them in configure.in.
>
> This could work both for and against automation.
> The pro being that we can shuffle at will outside of compat/ and enforce
> the wrapping #if. Along with compat/os/* performing undef of the
> wrappers after doing any exception includes.

We can automate the generation of wrappers for <> and even auto-check
that configure.in has the right checks. Phase2, perhaps?

>> 3. each src/ file, with a few hard-coded exceptions, must start with
>> #include "". If there is no "" to include, include "config.h". Do not
>> include "config.h" or "squid.h" if there is another "" include.
>
> Why the exception to not include config.h or squid.h?

It simply wastes a little bit of compilation time. If the above rules
are followed, config.h or squid.h will always be included at least once
per translation unit.

>> 4. "config.h", if any, goes first. "squid.h", if any, goes first or
>> second, depending on whether there is "config.h"
>
> squid.h requires config.h internally first. If squid.h is included it
> overrides config.h being needed.

Understood:
4. "squid.h" or "config.h", if any, goes first. If both are present,
include just "squid.h".

> The goal with these two was to include config.h first outside of src/
> and squid.h first inside of src/.

Wrong goal, IMO. We should have one and only one "first" header
everywhere (squid.h would be a natural name choice).

> Currently hampered by the long list of dependencies added to the unit
> tests by using squid.h. The earlier suggestion to move squid.h to
> squid_old.h and shuffle stuff into a cleaner squid.h for src/ may have
> to be taken first.

Yes, but that is a lot of work. I think this project can kick in before
this major reshuffle is finished. The alternative is to stop enforcing
the alphabetical order of #includes until squid.h is fixed.

>> 5. Other "" includes are sorted using ascending ASCII character value
>> order. Small letters are capitalized for sorting purposes.
>
> I don't think we need that capitilization clause. Enforcement and
> formatting is done centrally so we don't exactly have issues with
> portability on the comparisons. ASCII sorting is not a black-box process
> to any of us I hope.

No strong opinion here, but capitalizing will place "foo*" and "Foo*"
close to each other, which may look more "natural".

>> I am sure there will be a few exceptions and the above needs some
>> polishing, but I think we can automate this pretty well and add to the
>> SourceFormating scripts.
>>
>> Is there a better way to enforce the #include rules? Or should we drop
>> the ordering rules instead?
>
> I'm for keeping them.
>
> It's a bit of a no-op on small include lists. But really matters on the
> long ones. The patch conflicts from adding a new include at end of the
> list are also mostly gone with this policy.
>
> The exceptions will be in compat/ and the third-party library sources
> which Henrik has suggested should all be under lib/.
>
>
> I have a patch ready to test that

We can limit automation to src/, at least to start with.

Thank you,

Alex.
Received on Tue Oct 19 2010 - 18:32:40 MDT