Clean up headers with include-what-you-use #567

jmcarcell · 2024-03-10T21:25:31Z

BEGINRELEASENOTES

Clean up headers with include-what-you-use, second part after Remove unnecessary header includes #652

ENDRELEASENOTES

tmadlener

Thanks. Did you get include-what-you-use to work for this? If yes, can we also have that in CI somehow?

There is one minor comment below.

src/ROOTReader.cc

jmcarcell · 2024-05-09T17:30:05Z

I added iwyu to the stack and ran it. I think it's not in a state where it can be added to CI. The authors say it's in "alpha" and after playing with it a bit, it needed to add back one include that keeps being removed, doesn't take into account that some includes can only be used with C++20 and for some reason clangd told me there a few headers that could be removed.

m-fila · 2024-05-09T17:47:24Z

there is also misc-include-cleaner check for clang-tidy, which is supposed to given close results to clangd. And clangd also has CLI tool called clang-include-cleaner

tmadlener · 2024-05-10T11:40:15Z

include/podio/CollectionBufferFactory.h

+#include "podio/CollectionBuffers.h"
+#include "podio/SchemaEvolution.h"
+


Can we keep "our" headers above the STL ones? Or did something change here for the recommendations? (Mainly because this makes it more likely to miss some STL / system headers in our included files)

Fixed by changing .clang-format, at least the headers I checked had the system ones below.

tmadlener

I am not entirely convinced yet that all of the proposed changes from IWYU are what we want in the end. I have left a few comments inline.

Since we are most likely not planning to run this in CI, I would propose to clean this PR up a bit starting from the IWYU, trying a bit to also follow some of our conventions.

tmadlener · 2024-05-23T07:32:34Z

tests/extra_code/implementations.cc

@@ -1 +1,6 @@
-bool {name}::lt(int i) const { return number() < i; }


These should not be clang-formatted (and I think they should also be excluded from the pre-commit workflow)

This should be already excluded from pre-commit right?

podio/.pre-commit-config.yaml

Line 14 in 0a5af60

exclude: (tests/(datamodel|src|extra_code)/.*(h|cc)|podioVersion.in.h|SIOFrame.*h)

podio/.pre-commit-config.yaml

Line 19 in 0a5af60

exclude: (tests/(datamodel|src|extra_code)/.*(h|cc)$|podioVersion.in.h)

To exclude from clang-format run automatically by some IDE, adding a new file clang-format config should work.
tests/extra_code/.clang-format with:

DisableFormat: true

Would you like a separate PR or is it fine to do it here?

I was not aware that is a possibility. Can you make a separate PR for that and also put one into the python/templates directory? Then I could probably remove the custom hook from my config :)

Opened #605

tmadlener · 2024-05-23T07:33:53Z

include/podio/RNTupleReader.h

+
+#include <RVersion.h>


Suggested change

#include <RVersion.h>

#include <RVersion.h>

include/podio/RNTupleWriter.h

include/podio/SIOBlock.h

include/podio/utilities/DatamodelRegistryIOHelpers.h

tmadlener · 2024-05-23T07:38:29Z

src/ROOTReader.cc

+#include <TBranch.h>
+#include <TObjArray.h>
+#include <TObject.h>
+#include <algorithm>
+#include <functional>
+#include <optional>
+#include <stdexcept>
+#include <unordered_map>


Suggested change

#include <TBranch.h>

#include <TObjArray.h>

#include <TObject.h>

#include <algorithm>

#include <functional>

#include <optional>

#include <stdexcept>

#include <unordered_map>

#include <TBranch.h>

#include <TObjArray.h>

#include <TObject.h>

#include <algorithm>

#include <functional>

#include <optional>

#include <stdexcept>

#include <unordered_map>

Additionally, the system headers should go to the bottom.

tmadlener · 2024-05-23T07:39:06Z

src/SIOBlock.cc


+#include "podio/CollectionBase.h"
+#include "podio/GenericParameters.h"


Suggested change

#include "podio/CollectionBase.h"

#include "podio/GenericParameters.h"

#include "podio/CollectionBase.h"

#include "podio/GenericParameters.h"

tmadlener · 2024-05-23T07:39:23Z

src/SIOFrameData.cc

+#include "sio/buffer.h"
+#include "sio/definitions.h"

+#include <sio/api.h>
+#include <sio/block.h>


Suggested change

#include "sio/buffer.h"

#include "sio/definitions.h"

#include <sio/api.h>

#include <sio/block.h>

#include "sio/buffer.h"

#include "sio/definitions.h"

#include <sio/api.h>

#include <sio/block.h>

tests/unittests/unittest.cpp

tests/ostream_operator.cpp

hegner · 2024-06-14T08:42:36Z

@jmcarcell - can you update this? thanks

tmadlener

I have looked through a few files and it looks like we are getting there, but there are still a few things that I would like to explore to see whether things can be improved.

tmadlener · 2024-07-02T09:23:18Z

.clang-format

+  - Regex:           '<[[:alnum:]._]+>'
+    Priority:        5


Suggested change

- Regex: '<[[:alnum:]._]+>'

Priority: 5

- Regex: '<[[:alnum:]._]+>'

Priority: 5

- Regex: '^(<"podio/)'

Priority: 1

To keep the podio ones at the top?

include/podio/RNTupleReader.h

tmadlener · 2024-07-02T09:26:16Z

include/podio/RNTupleWriter.h

@@ -1,25 +1,34 @@
 #ifndef PODIO_RNTUPLEWRITER_H
 #define PODIO_RNTUPLEWRITER_H

-#include "podio/Frame.h"
-#include "podio/GenericParameters.h"
+#include "TFile.h"


Would move this to the other ROOT includes (might be solved by the addition to clang-format above).

include/podio/Reader.h

src/ROOTLegacyReader.cc

tmadlener · 2024-07-02T09:31:17Z

src/ROOTReader.cc

@@ -1,4 +1,5 @@
 #include "podio/ROOTReader.h"
+


Suggested change

tmadlener · 2024-07-02T09:33:04Z

src/SIOLegacyReader.cc

@@ -1,11 +1,17 @@
 #include "podio/SIOLegacyReader.h"
-#include "podio/SIOBlock.h"



Suggested change

tests/unittests/unittest.cpp

tmadlener · 2024-08-28T10:00:00Z

Will you rebase this PR to do further cleanup after #652? Or should we close this and open a new one?

tmadlener

Can we still try to have the "podio/..." headers sorted at the top? Currently some of the ROOT headers are still above them in quite some places.

tmadlener · 2024-09-09T07:52:55Z

src/DataSource.cc

@@ -1,4 +1,5 @@
 #include "podio/DataSource.h"
+


Suggested change

tmadlener · 2024-09-09T07:53:04Z

src/RNTupleReader.cc

@@ -1,14 +1,26 @@
 #include "podio/RNTupleReader.h"
+


Suggested change

tmadlener · 2024-09-09T07:54:45Z

src/SIOFrameData.cc


+#include "podio/SIOBlock.h"


Suggested change

#include "podio/SIOBlock.h"

#include "podio/SIOBlock.h"

Would effectively leave the file unchanged.

tmadlener · 2024-09-09T07:55:00Z

src/SIOWriter.cc

@@ -1,7 +1,7 @@
 #include "podio/SIOWriter.h"
+


Suggested change

Argh I think some of these are coming from clang-format

Interesting. Wondering which setting causes that 😂 Anyhow, if it is truly from clang-format and we don't find the setting to turn it off, we can also leave them instead of trying to fight clang-format.

tmadlener · 2024-09-09T07:55:12Z

src/SchemaEvolution.cc

@@ -1,7 +1,10 @@
 #include "podio/SchemaEvolution.h"
+


Suggested change

tmadlener · 2024-09-09T07:55:20Z

src/UserDataCollection.cc

@@ -1,9 +1,13 @@
 #include "podio/UserDataCollection.h"
+


Suggested change

tmadlener reviewed Mar 11, 2024

View reviewed changes

src/ROOTReader.cc Outdated Show resolved Hide resolved

jmcarcell force-pushed the headers branch from 8759bb4 to 3dac52f Compare May 9, 2024 15:00

jmcarcell changed the title ~~Clean up a few headers~~ Clean up headers with include-what-you-use May 9, 2024

tmadlener reviewed May 10, 2024

View reviewed changes

jmcarcell force-pushed the headers branch from 974f618 to 26a7ad7 Compare May 22, 2024 16:09

tmadlener reviewed May 23, 2024

View reviewed changes

jmcarcell force-pushed the headers branch from d5086eb to e537456 Compare July 1, 2024 17:10

tmadlener reviewed Jul 2, 2024

View reviewed changes

jmcarcell mentioned this pull request Aug 7, 2024

Remove unnecessary header includes #652

Merged

tmadlener closed this Aug 28, 2024

tmadlener reopened this Aug 28, 2024

jmcarcell added 2 commits September 8, 2024 21:31

Use IWYU to add new headers

fa120f9

Change stddef.h and stdint.h to cstddef and cstdint

0b8d1f4

jmcarcell force-pushed the headers branch from e537456 to 0b8d1f4 Compare September 8, 2024 19:33

jmcarcell added 3 commits September 8, 2024 21:41

Remove extra includes when including the collection ones

4195ab0

Remove unneeded header

6c68330

Remove unneeded header

4d97615

tmadlener reviewed Sep 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up headers with include-what-you-use #567

Clean up headers with include-what-you-use #567

jmcarcell commented Mar 10, 2024 •

edited

Loading

tmadlener left a comment

jmcarcell commented May 9, 2024

m-fila commented May 9, 2024

tmadlener May 10, 2024

jmcarcell May 22, 2024

tmadlener left a comment

tmadlener May 23, 2024

m-fila May 23, 2024

tmadlener May 23, 2024

m-fila May 23, 2024

tmadlener May 23, 2024

tmadlener May 23, 2024

tmadlener May 23, 2024

tmadlener May 23, 2024

hegner commented Jun 14, 2024

tmadlener left a comment

tmadlener Jul 2, 2024

tmadlener Jul 2, 2024

tmadlener Jul 2, 2024

tmadlener Jul 2, 2024

tmadlener commented Aug 28, 2024

tmadlener left a comment

tmadlener Sep 9, 2024

tmadlener Sep 9, 2024

tmadlener Sep 9, 2024

tmadlener Sep 9, 2024

jmcarcell Sep 9, 2024

tmadlener Sep 9, 2024

tmadlener Sep 9, 2024

tmadlener Sep 9, 2024

		#include "podio/CollectionBuffers.h"
		#include "podio/SchemaEvolution.h"

		@@ -1 +1,6 @@
		bool {name}::lt(int i) const { return number() < i; }


		#include "podio/CollectionBase.h"
		#include "podio/GenericParameters.h"

		@@ -1,11 +1,17 @@
		#include "podio/SIOLegacyReader.h"
		#include "podio/SIOBlock.h"

Clean up headers with include-what-you-use #567

Are you sure you want to change the base?

Clean up headers with include-what-you-use #567

Conversation

jmcarcell commented Mar 10, 2024 • edited Loading

tmadlener left a comment

Choose a reason for hiding this comment

jmcarcell commented May 9, 2024

m-fila commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hegner commented Jun 14, 2024

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmadlener commented Aug 28, 2024

tmadlener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcarcell commented Mar 10, 2024 •

edited

Loading