llvm/include/llvm/DebugInfo/CodeView/TypeHashing.h: 2 * pointless copy (#94385)

[akshaykumars614]

modified parameters for code quality

[llvmbot]

@llvm/pr-subscribers-debuginfo

Author: None (akshaykumars614)

Changes

modified parameters for code quality

---

Full diff: https://github.com/llvm/llvm-project/pull/94410.diff

1 Files Affected:

• (modified) llvm/include/llvm/DebugInfo/CodeView/TypeHashing.h (+2-2)

diff --git a/llvm/include/llvm/DebugInfo/CodeView/TypeHashing.h b/llvm/include/llvm/DebugInfo/CodeView/TypeHashing.h
index e92ec4855b252..453b38e0c11e3 100644
--- a/llvm/include/llvm/DebugInfo/CodeView/TypeHashing.h
+++ b/llvm/include/llvm/DebugInfo/CodeView/TypeHashing.h
@@ -212,8 +212,8 @@ template <> struct DenseMapInfo<codeview::GloballyHashedType> {
     return *reinterpret_cast<const unsigned *>(Val.Hash.data());
   }
 
-  static bool isEqual(codeview::GloballyHashedType LHS,
-                      codeview::GloballyHashedType RHS) {
+  static bool isEqual(const codeview::GloballyHashedType &LHS,
+                      const codeview::GloballyHashedType &RHS) {
     return LHS == RHS;
   }
 };

[dwblaikie]

This data type is trivially copyable and seems to be 8 bytes - that seems small enough that passing by value might be OK?

What tooling caught this/what is the rules that determine what's too expensive to copy/should be passed by const ref?

[akshaykumars614]

Static analyser cppcheck caught saying
trunk/llvm/include/llvm/DebugInfo/CodeView/TypeHashing.h:215:52: performance: Function parameter 'LHS' should be passed by const reference. [passedByValue]
trunk/llvm/include/llvm/DebugInfo/CodeView/TypeHashing.h:216:52: performance: Function parameter 'RHS' should be passed by const reference. [passedByValue]

[dwblaikie]

Do you happen to know where the documentation for this check is? I'm curious what the conditions are for the check to validate whether it's something we want to try to conform to or not

[akshaykumars614]

I could not able to find the documentation.

[dwblaikie]

I found the code at least ( https://github.com/danmar/cppcheck/blob/e0ce6483a006bf709d4485cb204c19f14656cf1b/lib/checkother.cpp#L1220 - and found a bugfix for the check from a couple of years ago, for std::string_view (which is a similar/same size - two pointers) https://github.com/danmar/cppcheck/pull/3817/files#diff-73c7189eeb09cde9a3e544f18ad317cc595f7a316fe720d781e35b7235be71b1 )

I think this check is poorly implemented - the bugfix for std::string_view was to detect if it was a view-like type, for instance. But that's not the property that makes the thing cheap to copy - the property is its size, and the fact it doesn't have a non-trivial copy ctor. And that property would apply equally to this code/the std::array inside it.

Hmm, nope, the code does seem to have a size check (else if (!var->valueType() || ValueFlow::getSizeOf(*var->valueType(), *mSettings) <= 2 * mSettings->platform.sizeof_pointer)) - I would've expected that this type would satisfy that requirement... so I wonder why the warning is still firing.