-
Notifications
You must be signed in to change notification settings - Fork 358
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
Modernize the codebase - remove leftover NULLs #2499
Conversation
@JanVogelsang I restarted one failed test, it seemed to be a technical issue on the runner. I would much prefer to put this PR in draft mode until #2498 is merge and base this PR on code with #2498 integrated. Otherwise it becomes more difficult to review and there is a small risk that merging this one would reintroduce && and II if diff should get confused. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general, but see comments. All commented-out code should be removed (in this PR only lines you touch), we have version control to keep an archive of what was. I also spotted some &&.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Other than my few comments, I'm fine with this.
One general question now that I come to think of it: Did any of your recent cleanup PRs touch code in the thirdparty
directory? Our usual policy is to leave those as is in order to match the upstream code as good as possible. They are also excluded from our static code checks in the CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few details left ;).
Co-authored-by: Hans Ekkehard Plesser <hans.ekkehard.plesser@nmbu.no>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JanVogelsang Thanks, all fine from my side now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There were some occurrences of
NULL
keywords that were not picked up byclang-tidy
and are now added in this PR.