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

Remove class specifier prefixing Location in function declarations #26503

Conversation

srmainwaring
Copy link
Contributor

Remove occurrences of class prefixing Location in function declarations:

…ations

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
…ations

Signed-off-by: Rhys Mainwaring <rhys.mainwaring@me.com>
@rmackay9
Copy link
Contributor

rmackay9 commented Mar 13, 2024

I'm happy with this but 3 of the 4 commits are outside my area so I've marked for next week's dev call. I suspect @peterbarker knows best if there's any difference in having the "class" in the front or not.

@nexton-winjeel
Copy link
Contributor

if there's any difference in having the "class" in the front or not.

AFAIK having the class there makes it a forward declaration.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

These header files are not including AP_Common/Location.h

That means they know this thing is a class only because they're getting that information transitively, and that's bad.

We do not want to include Location.h if we don't need to as it just slows things down unnecessarily.

I don't think this PR is a good idea.

Stripping them out where not required in e.g. .cpp files is a good idea, but not in headers if we then need to get the class aspect from elsewhere

@rmackay9
Copy link
Contributor

OK, thanks very much for the education on this issue! Sorry @srmainwaring for the extra work but we all learned something perhaps :-)

@srmainwaring
Copy link
Contributor Author

srmainwaring commented Mar 19, 2024

Stripping them out where not required in e.g. .cpp files is a good idea, but not in headers if we then need to get the class aspect from elsewhere

How about we forward declare class Location at the top of the file after the include directives rather than inline in function declarations? That makes the intent clearer. This started out as a request to clean up a function declaration in a different PR, so there is some confusion.

We do not want to include Location.h if we don't need to as it just slows things down unnecessarily.

The code base does not seem to generally apply forward declaration rules for compilation efficiency (as per J. Lakos, Large-scale C++ Software Design). Is that something we'd want to move to (or is that largely outdated now with the current round of compilers)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants