Make include order follow Google/LLVM/Lakos guidelines

[hazelnusse]

@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.

[hazelnusse]

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.

[TheLartians]

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.

[hazelnusse]

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.

[TheLartians]

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.

[hazelnusse]

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.

[ClausKlein]

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

[ClausKlein]

@TheLartians Any plan to merge this?