ModernCppStarter
ModernCppStarter copied to clipboard
TheLartians
Make include order follow Google/LLVM/Lakos guidelines
@TheLartians Feel free to take this or leave it. LLVM, Google, and John Lakos recommend this order: https://llvm.org/docs/CodingStandards.html#include-style https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
However, you may prefer to instead use "" instead of <>, as recommend by Cpp Core Guidelines: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#sf12-prefer-the-quoted-form-of-include-for-files-relative-to-the-including-file-and-the-angle-bracket-form-everywhere-else
If you use "" instead of <>, you could keep IncludeBlocks: Regroup but with the <> it seems clang-format wants to re-order.
Some places prefer/recommend <> exclusively, so YMMV.
Upon closer reading of the Cpp Core Guidelines, I think the recommendation there is to <> in our situation since greeter.cpp is not including greeter.h in a relative directory but instead from somewhere else in the project (include). So to satisfy all guidelines, I think the PR as it stands is probably most compliant (and has the advantage imo of using just the <> include form).
Again, feel free to take it or leave it as this is something clients could easily modify to their tastes.
Yeah, in my understanding, "" changes the header search path to prefer relative files to the source first. In our case all includes come from external locations, so <> makes sense.
Not too sure how I feel about preserving include groups, as that require the user to pay attention to the order and block that the includes are defined. Tbh I prefer less degrees of freedom as a starting point and allowing users to modify the template if they have more specific needs.
The most compelling reason to make the include of the corresponding header be the very first line in a cpp library implementation file is to ensure that the header file isn't missing includes, which I think is summarized nicely by the LLVM docs:
The Main Module Header file applies to .cpp files which implement an interface defined by a .h file. This #include should always be included first regardless of where it lives on the file system. By including a header file first in the .cpp files that implement the interfaces, we ensure that the header does not have any hidden dependencies which are not explicitly #included in the header, but should be. It is also a form of documentation in the .cpp file to indicate where the interfaces it implements are defined.
I think this can be partially addressed by using ICWU but the above practice is a simple and reliable method that requires no extra tooling.
Yeah, that does make sense, however it's in no way enforced by clang-format, right? It would then require discipline by the implementor and code reviewer to not end up with a bunch of meaningless include blocks after a while.
Correct, it is not enforced by clang-format, but with the change to .clang-format I made, it at least allows users to follow this practice if they want to without suggesting/making formating changes via the format/fix-format targets.
If I'm not mistaken, Google's cpplint.py does in fact check this: https://github.com/google/styleguide/blob/gh-pages/cpplint/cpplint.py
However, following this convention is desirable even if you are not following the rest of the google style guide which is why I figured it might be worth including here.
Yeah, that does make sense, however it's in no way enforced by clang-format, right? It would then require discipline by the implementor and code reviewer to not end up with a bunch of meaningless include blocks after a while.
if using the #include "greeter/..." form and this .clang-format, it would work:
IncludeBlocks: Regroup
IncludeCategories:
- Regex: '^"(llvm|llvm-c|clang|clang-c)/'
Priority: 2
- Regex: '^<(Poco|folly|gsl|asio|doctest|zmqpp|boost|fmt|json|spdlog|openssl)/'
Priority: 3
- Regex: '<[_[:alnum:]./]+>'
Priority: 4
# all other headers first!
- Regex: '.*'
Priority: 1
IncludeIsMainRegex: '(_test)?$'
see too https://clang.llvm.org/docs/ClangFormatStyleOptions.html
