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 v15 warnings #874

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

feltech
Copy link
Member

@feltech feltech commented Apr 3, 2023

Although we have yet to move to the newer clang-tidy on CI, we aim to support devs using a newer clang-tidy than v12.

Test Instructions

We seem to be getting cross-platform differences coming from a std::rethrow_exception call in src/openassetio-python/cmodule/src/BatchElementErrorBinding.cpp. That is, MacOS gives a warning that the std::exception_ptr cannot be std::moved, for some reason. See also #867 (comment)

@foundrytom
Copy link
Collaborator

foundrytom commented Apr 4, 2023

Just noting:

src/openassetio-core/include/openassetio/BatchElementError.hpp|48 col 7| error: an exception may be thrown in function 'BatchElementError' which should not throw exceptions [bugprone-exception-escape,-warnings-as-errors]
src/openassetio-core/include/openassetio/EntityReference.hpp|35 col 7| error: an exception may be thrown in function 'EntityReference' which should not throw exceptions [bugprone-exception-escape,-warnings-as-errors]
src/openassetio-core/include/openassetio/hostApi/ManagerFactory.hpp|43 col 10| error: an exception may be thrown in function 'ManagerDetail' which should not throw exceptions [bugprone-exception-escape,-warnings-as-errors]

On macOS which @feltech discovered as being llvm/llvm-project#54668.

Copy link
Collaborator

@foundrytom foundrytom left a comment

Choose a reason for hiding this comment

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

Good fixes, thanks. We're no worse off on mac/etc than we were, so can tackle those in followups if you like. Do we need an Improvements entry in the release notes?

@feltech
Copy link
Member Author

feltech commented Apr 4, 2023

Good fixes, thanks. We're no worse off on mac/etc than we were, so can tackle those in followups if you like. Do we need an Improvements entry in the release notes?

Updated release notes in 3194a59

Although we have yet to move to the newer clang-tidy on CI, we aim to
support devs using a newer clang-tidy than v12.

Interestingly, fixing warnings in newer clang-tidy can make older (v12)
clang-tidy unhappy.  Apparently changing a variable to `const` causes
`bugprone-infinite-loop` to trigger on Catch2 `CHECK` macros. So add
a suppression for this.

Signed-off-by: David Feltell <david.feltell@foundry.com>
@feltech feltech merged commit c13a5d7 into OpenAssetIO:main Apr 5, 2023
35 checks passed
@feltech feltech deleted the work/clangTidy15Fixes branch April 5, 2023 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants