Include style fixes, part 0

[javagl]

This is a first pass of addressing #80

This primarily addresses the include style for internal projects and third-party includes. Originally, there have (for example) been classes in CesiumGeospatial that included files from other projects, like CesiumUtility, using "", as in

#include "CesiumUtility/Math.h"

These have been changed to

#include <CesiumUtility/Math.h>

Similarly, for some third-party includes.

---

Beyond that, there are different guidelines regarding the order of includes, and they vary ... (and the degree to which they vary varies...). One consensus is, of course, that every .cpp file should include its corresponding .h file as the first include. After that, I now usually used an order like this:

// Include for the corresponding header
#include "Cesium3DTilesSelection/Tileset.h"

// Includes from the same project, using ""
#include "Cesium3DTilesSelection/CreditSystem.h"
...
#include "calcQuadtreeMaxGeometricError.h"

// Includes from own projects, using <>
#include <CesiumAsync/AsyncSystem.h>
...
#include <CesiumUtility/Uri.h>

// Includes from third parties
#include <glm/common.hpp>
#include <rapidjson/document.h>

// "Real" system includes
#include <algorithm>
...
#include <unordered_set>

_{If somebody now says "Nah, all wrong, make it so-and-so", then I'll update that accordingly. But I might ask: "Why didn't you establish that policy one year ago?". Keeping something consistent is often far easier than making something consistent that is not consistent in the first place...}

---

This PR does not cover the CesiumGltf* projects yet, because of their auto-generated files.

---

An aside: This issue (and therefore, this PR) may appear to have "low priority". But I have done some experiments with clang-tidy and similar tools, related to #203 , and I think that a proper and consistent include style is a crucial precondition for that: The build process should not bail out due to a warning in a third-party header that was included with the wrong include style....

[kring]

Beyond that, there are different guidelines regarding the order of includes, and they vary ...

I'm not too fussed about the include order, and can certainly see the wisdom in the one you've proposed, but the most important thing to my mind is that clang-format be able to sort our includes automatically. I'd rather not manually keep them in groups. If clang-format can do this grouping automatically, great! If not, I think its default behavior is just fine. I believe its default behavior is: corresponding header first, then quoted headers alphabetically, then angle bracketed headers alphabetically, with no blank lines in between. And it's the policy we've been using for quite awhile now.

[javagl]

Our style is based on the LLVM style, and it basically suggests the order that I mentioned for the different "categories" of includes. The handling of these categories is then further configured with the IncludeBlocks option (as documented on https://clang.llvm.org/docs/ClangFormatStyleOptions.html ). For our style, this is Preserve, which means that the categories/blocks will be preserved, but each block is sorted alphabetically.

The result can actually be seen in the formatting commit, where I only did the usual, final clang-format pass: The blocks are preserved, but only the order within these blocks changed.

[lilleyse]

tilers follows a similar structure but only has one project instead of several projects like cesium native.

The includes for "Projection.cpp" might look like

#include "tilers/core/Projection.h"

#include "tilers/core/Logger.h"
#include "tilers/core/Tracer.h"

#include <gdal.h>
#include <glm/trigonometric.hpp>

#include <cmath>
#include <filesystem>

The rules are:

• corresponding header should go first
• project headers should use quotation marks and be descendants of the include directory
• third party libraries should use angle brackets
• std headers should always use the C++ include style instead of the C include style (e.g. <cmath> instead of <math.h>)

We can get most of the way there with these clang-format rules:

IncludeBlocks: Regroup
IncludeCategories:
  # tilers includes
  - Regex: '"tilers\/[A-Za-z0-9\/\-_\+]+\.h"'
    Priority: 1
  # third party includes
  - Regex: '<[A-Za-z0-9\/\-_\+]+\.(h|hpp)>'
    Priority: 2
  # std library includes
  - Regex: '<[A-Za-z0-9_]+>'
    Priority: 3

[lilleyse]

The Regroup + regex approach seems to be the best way to sort includes automatically. You can give it really messy input and it will just work.

Before

#include <glm/trigonometric.hpp>

#include "tilers/core/Logger.h"


#include <gdal.h>
#include "tilers/core/Tracer.h"

#include <cmath>
#include "tilers/core/Projection.h"
#include <filesystem>

After

#include "tilers/core/Projection.h"

#include "tilers/core/Logger.h"
#include "tilers/core/Tracer.h"

#include <gdal.h>
#include <glm/trigonometric.hpp>

#include <cmath>
#include <filesystem>

[javagl]

I don't have a strong preference of whether it should be Preserve or Regroup, Kevin or Bao can decide that. (If we use Regroup, I'd check the "third party" category regex whether it also catches #include <gsl/span> (without .h), but that's a detail for now).

[kring]

Ok let's try regroup. If we can get that working well, it sounds great! If we can't do that, then (like I said before) I'd rather use an order that can be created automatically than maintain groups manually.

[javagl]

I have enabled the Regroup of clang-format, with this .clang-format entry :

# Disregard any exising include order, and re-order the headers
# to use the following order based on the include path regexes:
IncludeBlocks: Regroup
IncludeCategories:
  # Local includes, enclosed in quotation marks, ending in `.h`
  - Regex: '"[A-Za-z0-9+_\/\-]+\.h"'
    Priority: 1
  # Cesium project includes, starting with `<Cesium`, and ending with `.h>`
  - Regex: '<Cesium[A-Za-z0-9+_\/\-\]+\.h>'
    Priority: 2
  # Third party includes, of the form `<something/something>`
  - Regex: '<[A-Za-z0-9_\/\-]+\/[A-Za-z0-9_\/\-\.]+>'
    Priority: 3
  # Standard library includes, of the form `<something>
  - Regex: '<[A-Za-z0-9_]+>'
    Priority: 4

As expected, this required a few minor updates compared to what @lilleyse posted, for example, to seapare between local- and "internal project" includes, or to cover <gsl/span> (without extension). And apparently, a + does not have to be escaped when it is enclosed in [..]? At least, the original RegExes seemed to choke on the + in CesiumAsync/Impl/cesium-async++.h, but... there are some subtle differences in "dialects" of RegEx (as compared to what is suggested in https://xkcd.com/208/ ), and it seems to work fine now. At least, here is an example of the result, run on a completely messsed up order in Tileset.cpp:

#include "Cesium3DTilesSelection/Tileset.h"

#include "Cesium3DTilesSelection/CreditSystem.h"
#include "Cesium3DTilesSelection/ExternalTilesetContent.h"
#include "Cesium3DTilesSelection/ITileExcluder.h"
#include "Cesium3DTilesSelection/RasterOverlayTile.h"
#include "Cesium3DTilesSelection/RasterizedPolygonsOverlay.h"
#include "Cesium3DTilesSelection/TileID.h"
#include "Cesium3DTilesSelection/spdlog-cesium.h"
#include "TileUtilities.h"
#include "calcQuadtreeMaxGeometricError.h"

#include <CesiumAsync/AsyncSystem.h>
#include <CesiumAsync/IAssetAccessor.h>
#include <CesiumAsync/IAssetResponse.h>
#include <CesiumAsync/ITaskProcessor.h>
#include <CesiumGeometry/Axis.h>
#include <CesiumGeometry/QuadtreeTileAvailability.h>
#include <CesiumGeospatial/Cartographic.h>
#include <CesiumGeospatial/GeographicProjection.h>
#include <CesiumGeospatial/GlobeRectangle.h>
#include <CesiumUtility/JsonHelpers.h>
#include <CesiumUtility/Math.h>
#include <CesiumUtility/Tracing.h>
#include <CesiumUtility/Uri.h>

#include <glm/common.hpp>
#include <gsl/span>
#include <rapidjson/document.h>

#include <algorithm>
#include <cstddef>
#include <limits>
#include <optional>
#include <unordered_set>

(The gsl/span was only included for testing). But that LGTM...

[kring]

Thanks @javagl, this is looking great. I submitted a tweak to .clang-format to treat <something.h> as third-party, too, not just <something/something[.h]> because there were a couple of third-party headers missed. Let me know if you see a problem with this.

Some (small) problems remaining:

• The files in the CesiumAsync/Impl directory aren't using relative paths correctly. For example in TaskScheduler.h:

#include "CesiumAsync/ITaskProcessor.h"
#include "CesiumAsync/Impl/ImmediateScheduler.h"

I think it should be:

#include "../ITaskProcessor.h"
#include "ImmediateScheduler.h"

• All of the .cpp files don't really use relative paths correctly either, but I'm not sure this one is worth fixing. For example, AsyncSystem.cpp has:

#include "CesiumAsync/AsyncSystem.h"

#include "CesiumAsync/ITaskProcessor.h"

The actual relative paths are:

#include "../include/CesiumAsync/AsyncSystem.h"

#include "../include/CesiumAsync/ITaskProcessor.h"

But that's pretty ugly. And maybe it doesn't matter because those paths only need to be resolveable by the cesium-native build process itself (since CPP files won't be included in third-party projects directly). So is this our policy in C++ files? Same-library headers should use quotes and a "library-like" path? It's not obvious to me what would be better.

[javagl]

I submitted a tweak to .clang-format to treat <something.h> as third-party, too

I had seen some of these cases, but particularly for sqlite3.h, I wasn't entirely sure how to treat that (there was some back and forth of ~"Which sqlite is used"). But the change looks fine.

For the point of using "relative paths correctly" (be it in headers or implementation files): You linked to https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf12-prefer-the-quoted-form-of-include-for-files-relative-to-the-including-file-and-the-angle-bracket-form-everywhere-else in an earlier comment elsewhere. Our paths are currently not really relative to the file but relative to the "project root", and that works, but that may only be an artifact of the black magic that the compiler uses when searching for these files.

In that regard, considering the options of

#include "CesiumAsync/AsyncSystem.h"
vs.
#include "../include/CesiumAsync/AsyncSystem.h"

the first one could cause trouble (as indicated by the "Note" in the Guidelines). I just checked this by creating an empty

cesium-native\CesiumAsync\include\CesiumAsync\Impl\CesiumAsync\ITaskProcessor.h

The #include "CesiumAsync/ITaskProcessor.h" from TaskScheduler.h that you mentioned then causes this empty file to be picked up, because ... well, that's the actual "relative path" then. Of course, this is a bit contrived, and very unlikely to happen "accidentally" in this exact form. But it shows that using a #include "../ITaskProcessor.h" or the clumsy ones with "../include/..." would resolve such ambiguities, and I think that it would be "cleaner and safer" to use the actual relative paths...

[kring]

I think the #include "../ITaskProcessor.h" from the header in the CesiumAsync/Impl directory is acceptable/necessary because it's a public header file. It's an implementation detail (hence the Impl) but it the headers still need to be included from user code because of the requirements of C++ templates.

But I think #include "../include/CesiumAsync/AsyncSystem.h" from AsyncSystem.cpp is a step too far. Yes it's possible that #include "CesiumAsync/AsyncSystem.h" will end up including the wrong file if we add a file with such a relative path, but that would only impact our library's build system. Users of our library will never have to worry about it. And therefore the pain of that super-awkward relative path exceeds the benefit of avoiding this unlikely scenario within our own code.

So let's the fix my first bullet in #349 (comment) but not the second one.

[kring]

Also @javagl can I bug you to merge in main since you're probably best placed to fix the conflicts?

[javagl]

I think there are not other libraries with Impl-like subdirectories, but generally: Is it valid to assume that your first bullet point only refers to the CesiumAsync/Impl files? (I haven't thoroughly reviewed all other includes for this pattern, and wondered whether there was a nice trick or tool to detect "ambiguous relative includes" in general, but if it referred only to this directory, I'd do it manually).

And of course, I'd prepare everything for the merge then. This would mainly be to "ignore everything from this branch and just re-run format on the state of main", except for minor things like the tinyxml2 wrapper and the relative path fixes.

[javagl]

I think there are not other libraries with Impl-like subdirectories, but generally: Is it valid to assume that your first bullet point only refers to the CesiumAsync/Impl files? (I haven't thoroughly reviewed all other includes for this pattern, and wondered whether there was a nice trick or tool to detect "ambiguous relative includes" in general, but if it referred only to this directory, I'd do it manually).

And of course, I'd prepare everything for the merge then. This would mainly be to "ignore everything from this branch and just re-run format on the state of main", except for minor things like the tinyxml2 wrapper and the relative path fixes.

[kring]

Is it valid to assume that your first bullet point only refers to the CesiumAsync/Impl files?

I can't think of any other cases like this.

[javagl]

I can't think of any other cases like this.

I wondered whether the include paths in public headers should also be relative. As such, for the files

Tile.h 
BoundingVolume.h

with Tile.h containing

#include "Cesium3DTilesSelection/BoundingVolume.h"

which could just be

#include "BoundingVolume.h"

I mean, this would prevent the aforementioned problem (if somebody added a Cesium3DTilesSelection/BoundingVolume.h in the existing Cesium3DTilesSelection directory), and would not involve the "ugly" relative "../include/" part. Any thoughts on that?

[kring]

Ah yep, sorry, for some reason I thought that was already addressed.

To summarize it, all .h files in a subdirectory of the include directory, should use actual, valid relative paths. .cpp files and .h files in src or a subdirectory of src may use not-strictly-relative paths in order to avoid ugly ../include/ stuff.

[javagl]

I have updated the public headers to always use the proper relative include paths.

The manual part was straightforward, with one subtle detail: GltfReader.h, contained, for example

#include "CesiumGltf/ReaderLibrary.h"
#include "CesiumGltf/Model.h"

and now it contains the apparently inconsistent

#include "ReaderLibrary.h"
#include <CesiumGltf/Model.h>

because Model.h is in a different project (despite the same directory- and namespace name). But that's the way it is.

---

I also updated the generate-gltf-classes to generate the headers with the proper relative includes. This brought up the issue of some unexpected .cpp files floating around in the generated directory, as already mentioned at the bottom of #350 (comment) , but I hope that Git will sort out the fact that the unnecessary files have been removed in both branches now.

[javagl]

(A totally unrelated aside: I wonder whether all this will have a noticable effect (be it positive or negative) on the average build times...)

_Edit:

Quickly comparing two of the latest main builds to this one...

... and not having a clear idea about the variance of the build times, it appears to be not noticably faster or slower than before. OK...

[kring]

Thanks @javagl!