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 narrowing in Cov3f #59

Merged
merged 5 commits into from
May 3, 2024
Merged

Fix narrowing in Cov3f #59

merged 5 commits into from
May 3, 2024

Conversation

jmcarcell
Copy link
Member

BEGINRELEASENOTES

  • Fix narrowing in Cov3f from int to float. This triggers a warning in GCC 13 and a compiler error in Clang 17.
  • Fix a compiler error and a warning with Clang. The first one about constness of the LCIO pid handler and the other one about a lambda parameter not being used

ENDRELEASENOTES

The narrowing could be fixed in EDM4hep by doing static_cast<float> first but then there is no info when there is a narrowing conversion and there is possibly loss of some information because the destination type (float for CovNf) can't represent all the values of the original type. What I don't like is that it's a bit inconsistent in GCC (warning) and Clang (error).

The LCIO PID handler one is:

   80 |     for (const auto id : pidHandler.getAlgorithmIDs()) {
      |                          ^~~~~~~~~~
/LCIO/src/cpp/include/UTIL/PIDHandler.h:104:26: note: 'getAlgorithmIDs' declared here
  104 |     const EVENT::IntVec& getAlgorithmIDs() ;

@@ -98,7 +98,7 @@ bool compareValuesNanSafe(LCIO lcioV, EDM4hepT edm4hepV, const std::string& msg)
>;

if constexpr (isVectorLike) {
const auto vecSize = [&edm4hepV]() -> std::size_t {
const auto vecSize = [](EDM4hepT& edm4hepV) -> std::size_t {
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 something compilers start to warn about?

@tmadlener tmadlener merged commit feeadba into main May 3, 2024
6 of 9 checks passed
@tmadlener tmadlener deleted the covnf-narrowing branch May 3, 2024 07:35
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.

2 participants