This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] new cppcoreguidelines-slicing
ClosedPublic

Authored by courbet on Jul 5 2016, 6:52 AM.

Diff Detail

Event Timeline

courbet updated this revision to Diff 62742.Jul 5 2016, 6:52 AM
courbet retitled this revision from to [clang-tidy] new cppcoreguidelines-slicing.
courbet updated this object.
courbet added a subscriber: cfe-commits.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

courbet updated this revision to Diff 62845.Jul 6 2016, 4:20 AM

Update release notes.

alexfh edited edge metadata.Jul 7 2016, 5:46 AM

Thanks for the new awesome check!

Please run the check on LLVM and include your analysis of the results in the patch description. Another couple of comments below.

clang-tidy/cppcoreguidelines/SlicingCheck.cpp
94

Please use auto *

docs/clang-tidy/checks/cppcoreguidelines-slicing.rst
8

Links to the C++ Core Guidelines are good, but a few words of explanation and an example here wouldn't hurt either.

hokein added inline comments.Jul 11 2016, 12:41 AM
clang-tidy/cppcoreguidelines/SlicingCheck.cpp
49

Looks like you are missing some cases here, like assigning a Subclass object from a function call to a Baseclass object, passing a Subclass object to a function whose parameter is a BaseClass.

But I think it's fine to implement in a separate patch, but you'd better to add a TODO here.

SubClass f1();
BaseClass a = f1();

void f1(BaseClass a);
SubClass sc;
f1(sc);
85

There are two diagnostic messages in the code. For subclasses with override base class methods and extra member, this will print two messages, which is a bit of redundant.

95

The code can be simplified like:

if (const auto *BR = cast_or_null<CXXRecordDecl>(BaseRecordType->getDecl()->getDefinition())) {
  DiagnoseSlicedOverriddenMethods(Call, *BR, BaseDecl); 
}
113

s/method/methods

courbet updated this revision to Diff 63477.Jul 11 2016, 1:49 AM
courbet edited edge metadata.
courbet marked 4 inline comments as done.
  • Add a test case following comments.
  • Add more comments in tests.
  • Add examples in doc.
  • Simplify code a bit.

Thanks for the comments.

clang-tidy/cppcoreguidelines/SlicingCheck.cpp
49

Actually these will still create a CXXConstructExpr in the AST, e.g for case (2):

CallExpr 0x3d6e560 </usr/local/google/home/courbet/test2.cc:10:3, col:6> 'void'
|-ImplicitCastExpr 0x3d6e548 <col:3> 'void (*)(class A)' <FunctionToPointerDecay>
| `-DeclRefExpr 0x3d6e4f8 <col:3> 'void (class A)' lvalue Function 0x3d66550 'g' 'void (class A)'
`-CXXConstructExpr 0x3d6e5c8 <col:5> 'class A' 'void (const class A &) throw()'
  `-ImplicitCastExpr 0x3d6e5b0 <col:5> 'const class A' lvalue <NoOp>
    `-ImplicitCastExpr 0x3d6e590 <col:5> 'class A' lvalue <DerivedToBase (A)>
      `-DeclRefExpr 0x3d6e4d0 <col:5> 'class B' lvalue Var 0x3d6e300 'b' 'class B'

I alreagy have a test to case (2) and I've added one for case (1).

85

I think we still want to point out to the user which overrides are going to be discarded:

class A { virtual void f(); virtual void g(); }
class B : public A { void f() override; void g() override; int a; }

The messages will be:
"slicing object from type 'B' to 'A' discards override 'f' "
"slicing object from type 'B' to 'A' discards override 'g' "
"slicing object from type 'B' to 'A' discards 4*sizeof(char) bytes of state"

courbet added a comment.EditedJul 11 2016, 1:55 AM

I've ran this check on llvm. There are 0 instances of virtual function slicing (which is not surprising since these usually result in actual bugs) and 71 instances of member varaible slicing:

  • 'FullSourceLoc' to 'SourceLocation': 57
  • 'APSInt' to 'APInt': 5
  • 'DeducedTemplateArgument' to 'TemplateArgument': 3
  • 'SemaDiagnosticBuilder' to 'DiagnosticBuilder': 2
  • 'RegHalf' to 'RegisterRef': 2
  • 'PathDiagnosticRange' to 'SourceRange': 2

Most are harmless (but still true positives). The 'SemaDiagnosticBuilder' and 'APSInt' are actually interesting:

llvm/llvm/tools/clang/lib/Sema/SemaExprObjC.cpp:3555
DiagnosticBuilder DiagB = … should be: SemaDiagnosticBuilder DiagB = …
DiagB is then passed by reference to a function, so there really is no reason to slice it.

AST/ExprConstant.cpp:3150
explicit APSInt(APInt I, bool isUnsigned = true)
Here it’s easy to write: APSInt MyInt(MyOtherInt); and think it’s a copy, but it’s not (it just changed the signedness). APSInt MyInt(MyOtherInt.toAPInt()); would make it clear what’s happening.

hokein added inline comments.Jul 12 2016, 12:00 AM
clang-tidy/cppcoreguidelines/SlicingCheck.cpp
49

Oh, I see it now. Sorry for missing them and thanks for explanation. It'd be more clear to if you can update comments at SlicesObjectInCtor.

85

Maybe alexfh@ has idea about this. Output multiple warning messages in a same location is not the pattern of clang-tidy message. I think we should combine three message into exact one, something like "slicing object from type 'B' to 'A' discards override 'f', 'g', 4*sizeof(char) bytes of state".

docs/clang-tidy/checks/cppcoreguidelines-slicing.rst
24

There is an extra blank line at the bottom.

test/clang-tidy/cppcoreguidelines-slicing.cpp
95

s/tnow/now

courbet updated this revision to Diff 64302.Jul 18 2016, 4:43 AM
courbet marked 3 inline comments as done.

Cosmetics.

alexfh accepted this revision.Jul 19 2016, 6:43 AM
alexfh edited edge metadata.

LG with a nit.

clang-tidy/cppcoreguidelines/SlicingCheck.cpp
87

The "slicing ... discards x bytes of state" message is not going to be repeated for the same location, so it's fine on its own.

Re: the "discards override" messages, they quite infrequent, IIUC, so the risk of spamming people with diagnostics is rather low. In case it's still problematic, we can change the repeated parts to notes.

92

Let's make the code more consistent:

if (const auto *BaseRecordType = Base.getType()->getAs<RecordType>()) {
  if (const auto *BaseRecord =
        cast_or_null<CXXRecordDecl>(BaseRecordType->getDecl()->getDefinition()))
      DiagnoseSlicedOverriddenMethods(Call, *BaseRecord, BaseDecl);
}
This revision is now accepted and ready to land.Jul 19 2016, 6:43 AM
courbet updated this revision to Diff 64641.Jul 19 2016, 11:37 PM
courbet edited edge metadata.
courbet marked an inline comment as done.

cosmetics

Thanks for the review.

clang-tidy/cppcoreguidelines/SlicingCheck.cpp
87

Thanks for the insight.

Prazek added a subscriber: Prazek.Jul 20 2016, 12:20 AM
Prazek added inline comments.
clang-tidy/cppcoreguidelines/SlicingCheck.cpp
31

What is A and what is B? I guess you are missing very important fact, that A is a base class of B

clang-tidy/cppcoreguidelines/SlicingCheck.h
20

some short description what does this check do?

Prazek added a project: Restricted Project.Jul 20 2016, 12:21 AM
courbet marked an inline comment as done.Jul 20 2016, 12:41 AM
courbet added inline comments.
clang-tidy/cppcoreguidelines/SlicingCheck.h
20

There is already a more detialed explanation provided in extra/clang-tidy/checks/cppcoreguidelines-slicing.html, and I feel that the links explain it better than anything short we could put in the comments there. Or should I copy-paste what's in extra/clang-tidy/checks/cppcoreguidelines-slicing.html ?

Is it in upstream yet?

clang-tidy/cppcoreguidelines/SlicingCheck.h
20

Well, it would be good to at least say what is slicing :)

courbet updated this revision to Diff 65028.Jul 22 2016, 12:13 AM
courbet marked an inline comment as done.
hokein accepted this revision.Jul 22 2016, 1:55 AM
hokein edited edge metadata.

Looks good now. Do you need I submit for you?

I can submit, thanks.

courbet closed this revision.Jul 25 2016, 8:19 AM