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

Fix clang-tidy checks, including some use-after-move #1352

Merged
merged 5 commits into from
Aug 5, 2024

Conversation

esseivaju
Copy link
Contributor

Fix a few clang-tidy checks. There were a few use-after-move in LocalTransporter.cc, Solid.cc and ValueGridBuilder.cc that could lead to bugs. In addition, ignore some checks making a lot of noise. The remaining warnings are mostly false alerts and could use a // NOLINT comment explaining why the code is fine.

@esseivaju esseivaju added the minor Minor internal changes or fixes label Aug 2, 2024
@esseivaju esseivaju requested a review from sethrj August 2, 2024 19:34
app/celer-g4/DetectorConstruction.cc Outdated Show resolved Hide resolved
@@ -113,7 +113,7 @@ class GeantLoggerAdapter : public G4coutDestination
public:
// Assign to Geant handlers on construction
GeantLoggerAdapter();
~GeantLoggerAdapter();
~GeantLoggerAdapter() override;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this override? It's not like you can declare a destructor "final"...

Copy link
Contributor Author

@esseivaju esseivaju Aug 2, 2024

Choose a reason for hiding this comment

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

To document/verify that the base class has a virtual dtor. This is following the discussion isocpp/CppCoreGuidelines#1448 virtual instead of override is also fine.

Copy link
Member

Choose a reason for hiding this comment

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

That's weird. The parent class doesn't have a ~GeantLoggerAdapter method. Maybe next c++ version will name the destructor __del__ so you can actually tell it's overriding... I think I prefer virtual here since it says "polymorphic and safe to destruct" rather than "I'm replacing what the parent classdoes".

Copy link
Contributor Author

@esseivaju esseivaju Aug 3, 2024

Choose a reason for hiding this comment

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

The parent G4coutDestination still has a virtual destructor. Even though technically destructor override their parent's I agree that semantically virtual makes sense since they are chained and don't replace the parent's behavior. However, the advantage of override is that it will result in an error if the parent dtor is no longer virtual. Using virtual would silently redeclare it virtual in the derived which would be confusing.

Anyway, in that specific case, it's not really important since this is a TU-local class but in general I prefer the destructor be declared override if it needs to be defined again in the derived class for the reason above.

Maybe for that case we could declare the class final? It's local anyway and would silence the warning

Copy link
Member

Choose a reason for hiding this comment

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

Declaring as final sounds good to me, and now that I know about this behavior I think override for virtual destructors is sensible. (It's partly weird because the daughter classes will automatically define virtual override destructors...)

src/orange/orangeinp/detail/ProtoBuilder.cc Outdated Show resolved Hide resolved
Copy link
Member

@sethrj sethrj left a comment

Choose a reason for hiding this comment

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

Thanks @esseivaju ! Looks like there's only 20 warnings left, after those are gone we can make "warning" fatal 😄

.clang-tidy Show resolved Hide resolved
.clang-tidy Show resolved Hide resolved
.clang-tidy Show resolved Hide resolved
.clang-tidy Show resolved Hide resolved
.clang-tidy Show resolved Hide resolved
.clang-tidy Show resolved Hide resolved
.clang-tidy Show resolved Hide resolved
Copy link
Contributor Author

@esseivaju esseivaju left a comment

Choose a reason for hiding this comment

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

Even though some checks are not showing up on the CI, they'll show up in my environment. If we don't want these checks I suppose we can keep them disabled for future-proofing when we switch to ubuntu-24.04 runners.

.clang-tidy Show resolved Hide resolved
.clang-tidy Show resolved Hide resolved
.clang-tidy Show resolved Hide resolved
.clang-tidy Show resolved Hide resolved
@sethrj sethrj enabled auto-merge (squash) August 5, 2024 17:25
@sethrj sethrj merged commit c73636c into celeritas-project:develop Aug 5, 2024
29 checks passed
@esseivaju esseivaju deleted the fix-clang-checks branch August 5, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Minor internal changes or fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants