Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authoritatively forward declare core types #312

Merged
merged 2 commits into from
Oct 28, 2024
Merged

Authoritatively forward declare core types #312

merged 2 commits into from
Oct 28, 2024

Conversation

chiphogg
Copy link
Contributor

From now on, anyone can just include "au/fwd.hh", and get
authoritatively correct forward declarations. These will be implicitly
tested automatically by adding an #include of this new file at the top
of every file that has a definition for which we now provide a forward
declaration: if we get a forward declaration wrong, we'll get a compiler
warning.

We also include fwd.hh in a few places where it lets us get rid of
some manual forward declarations.

The rep-named aliases move upstream into fwd.hh, because they'll be
directly useful for forward declaration use cases, and because you can't
"forward declare" an alias: you just have to move the definition
upstream.

Helps #232. To resolve it completely, we will need to provide a forward
declaration file for each individual unit file.

From now on, anyone can just include `"au/fwd.hh"`, and get
authoritatively correct forward declarations.  These will be implicitly
tested automatically by adding an `#include` of this new file at the top
of every file that has a definition for which we now provide a forward
declaration: if we get a forward declaration wrong, we'll get a compiler
warning.

We also include `fwd.hh` in a few places where it lets us get rid of
some manual forward declarations.

The rep-named aliases move upstream into `fwd.hh`, because they'll be
directly useful for forward declaration use cases, and because you can't
"forward declare" an alias: you just have to move the definition
upstream.

Helps #232.  To resolve it completely, we will need to provide a forward
declaration file for each individual unit file.
@chiphogg chiphogg added the release notes: ✨ lib (enhancement) PR enhancing the library code label Oct 27, 2024
@chiphogg chiphogg requested a review from geoffviola October 27, 2024 00:43
Copy link
Contributor

@geoffviola geoffviola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks useful 😄

@@ -17,6 +17,8 @@
#include <chrono>
#include <type_traits>

#include "au/fwd.hh"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to include the forward declaration file in every file whose types are forward declared. This should give us better test coverage for cases where the forward declaration goes out of sync with the true definition, such as if we forward declare as class Zero; but define as struct Zero {...};.

That said, I tried creating exactly this error condition, and I wasn't able to get a warning to come up from //au:zero_test alone. I do get errors from some other test in //...:all, but I get these regardless of whether this file includes the fwd.hh file or not.

All in all: I think it's a good best practice, and I'm going to keep it for that reason, although I'm mystified about why I can't seem to trigger the warnings as easily as I would expect.

@chiphogg chiphogg merged commit 654f1d9 into main Oct 28, 2024
11 checks passed
@chiphogg chiphogg deleted the chiphogg/fwd#232 branch October 28, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: ✨ lib (enhancement) PR enhancing the library code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants