[clang-tidy] cppcoreguidelines-slicing: display discarded state in bits

[courbet]

Summary

I can't do bytes since the char width is not guaranteed to be a multiple of 8 bits. The message now looks like:

warning: slicing object from type 'DerivedWithMemberVariables' to 'Base' discards 32 bits of state [cppcoreguidelines-slicing]

This fixes https://llvm.org/bugs/show_bug.cgi?id=28675

[alexfh]

test/clang-tidy/cppcoreguidelines-slicing.cpp

34

It would be nice to test the values the check prints. It might be non-trivial to do really platform-neutral, but we can specify the -triple= in tests or assume char is always 8 bit on the platforms LLVM is compiled (I believe, this is true now).

[alexfh]

I can't do bytes since the char width is not guaranteed to be a multiple of 8 bits.

The standard uses the word "byte" ([intro.memory]) though it doesn't guarantee it's an octet, so it seems quite reasonable to use "byte" in diagnostics too, since we're in the context of C++. If we see an evidence of this wording causing confusion, we can reconsider. WDYT?

[courbet]

So you would advocate just replacing "sizeof(char)" with "bytes" ? I'm all for it if people think that there's no possible confusion.

[JDevlieghere]

Personally, I think it's reasonably clear from the context and greatly benefits the readability of the actual number.

[aaron.ballman]

In D27212#621091, @courbet wrote:

So you would advocate just replacing "sizeof(char)" with "bytes" ? I'm all for it if people think that there's no possible confusion.

Yes, "bytes" is a perfectly cromulent term to use here. Byte does not mean the same thing as Octet, at least as far as C and C++ are concerned. Bits is also reasonable, as you've used here, but I think it makes the common case a bit harder to read. It's easier to read "4096 bytes" than "32768 bits".

clang-tidy/cppcoreguidelines/SlicingCheck.cpp

127

Please don't use auto here, as the type is not spelled out in the initialization.

130

Is int safe to use here, or can the multiplication cause this to become negative? I'd like to see a test case where the number of bits sliced is too large to fit into an int.

[courbet]

Just use "bytes" to man "a certain number of char size units" as per the consensus.

clang-tidy/cppcoreguidelines/SlicingCheck.cpp

130

no longer relevant

[courbet]

Sounds good, thanks for the comments.

[alexfh]

LG

[courbet]

Thanks.

[alexfh]

This was committed a while ago as r290340.

clang-tidy/cppcoreguidelines/SlicingCheck.cpp

void SlicingCheck::check(const MatchFinder::MatchResult &Result) {
   // because in that case calling A::f is the same as calling B::f.
   DiagnoseSlicedOverriddenMethods(*Call, *DerivedDecl, *BaseDecl);
 
   // Warn when slicing member variables.
   const auto &BaseLayout =
       BaseDecl->getASTContext().getASTRecordLayout(BaseDecl);
   const auto &DerivedLayout =
       DerivedDecl->getASTContext().getASTRecordLayout(DerivedDecl);
-  const auto StateSize = DerivedLayout.getDataSize() - BaseLayout.getDataSize();
+  const CharUnits StateSize =
+      DerivedLayout.getDataSize() - BaseLayout.getDataSize();
   if (StateSize.isPositive()) {
     diag(Call->getExprLoc(), "slicing object from type %0 to %1 discards "
-                             "%2*sizeof(char) bytes of state")
+                             "%2 bytes of state")
         << DerivedDecl << BaseDecl << static_cast<int>(StateSize.getQuantity());
   }
 }
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

test/clang-tidy/cppcoreguidelines-slicing.cpp

 
 void TakesBaseByValue(Base base);
 
 DerivedWithMemberVariables ReturnsDerived();
 
 void positivesWithMemberVariables() {
   DerivedWithMemberVariables b;
   Base a{b};
-  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: slicing object from type 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}}*sizeof(char) bytes of state [cppcoreguidelines-slicing]
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: slicing object from type 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}} bytes of state [cppcoreguidelines-slicing]
   a = b;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}}*sizeof(char) bytes of state
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}} bytes of state
   TakesBaseByValue(b);
-  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: slicing object from type 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}}*sizeof(char) bytes of state
+  // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: slicing object from type 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}} bytes of state
 
   TwiceDerivedWithNoMemberVariables c;
   a = c;
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type 'TwiceDerivedWithNoMemberVariables' to 'Base' discards {{[0-9]*}}*sizeof(char) bytes of state
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type 'TwiceDerivedWithNoMemberVariables' to 'Base' discards {{[0-9]*}} bytes of state
 
   a = ReturnsDerived();
-  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}}*sizeof(char) bytes of state
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type 'DerivedWithMemberVariables' to 'Base' discards {{[0-9]*}} bytes of state
 }
 
 void positivesWithOverride() {
   DerivedWithOverride b;
   Base a{b};
   // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: slicing object from type 'DerivedWithOverride' to 'Base' discards override 'g'
   a = b;
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: slicing object from type 'DerivedWithOverride' to 'Base' discards override 'g'