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.
