[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL318522: [clang-tidy] Add a check for undelegated copy of 
base classes (authored by xazax).

Changed prior to commit:
  https://reviews.llvm.org/D33722?vs=121886=123319#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D33722

Files:
  clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/bugprone/CopyConstructorInitCheck.cpp
  clang-tools-extra/trunk/clang-tidy/bugprone/CopyConstructorInitCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-copy-constructor-init.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/test/clang-tidy/bugprone-copy-constructor-init.cpp

Index: clang-tools-extra/trunk/clang-tidy/bugprone/CopyConstructorInitCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/CopyConstructorInitCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/CopyConstructorInitCheck.cpp
@@ -0,0 +1,121 @@
+//===--- CopyConstructorInitCheck.cpp - clang-tidy-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "CopyConstructorInitCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void CopyConstructorInitCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+return;
+
+  // In the future this might be extended to move constructors?
+  Finder->addMatcher(
+  cxxConstructorDecl(
+  isCopyConstructor(),
+  hasAnyConstructorInitializer(cxxCtorInitializer(
+  isBaseInitializer(),
+  withInitializer(cxxConstructExpr(hasDeclaration(
+  cxxConstructorDecl(isDefaultConstructor())),
+  unless(isInstantiated()))
+  .bind("ctor"),
+  this);
+}
+
+void CopyConstructorInitCheck::check(const MatchFinder::MatchResult ) {
+  const auto *Ctor = Result.Nodes.getNodeAs("ctor");
+  std::string ParamName = Ctor->getParamDecl(0)->getNameAsString();
+
+  // We want only one warning (and FixIt) for each ctor.
+  std::string FixItInitList;
+  bool HasRelevantBaseInit = false;
+  bool ShouldNotDoFixit = false;
+  bool HasWrittenInitializer = false;
+  SmallVector SafeFixIts;
+  for (const auto *Init : Ctor->inits()) {
+bool CtorInitIsWritten = Init->isWritten();
+HasWrittenInitializer = HasWrittenInitializer || CtorInitIsWritten;
+if (!Init->isBaseInitializer())
+  continue;
+const Type *BaseType = Init->getBaseClass();
+// Do not do fixits if there is a type alias involved or one of the bases
+// are explicitly initialized. In the latter case we not do fixits to avoid
+// -Wreorder warnings.
+if (const auto *TempSpecTy = dyn_cast(BaseType))
+  ShouldNotDoFixit = ShouldNotDoFixit || TempSpecTy->isTypeAlias();
+ShouldNotDoFixit = ShouldNotDoFixit || isa(BaseType);
+ShouldNotDoFixit = ShouldNotDoFixit || CtorInitIsWritten;
+const CXXRecordDecl *BaseClass =
+BaseType->getAsCXXRecordDecl()->getDefinition();
+if (BaseClass->field_empty() &&
+BaseClass->forallBases(
+[](const CXXRecordDecl *Class) { return Class->field_empty(); }))
+  continue;
+bool NonCopyableBase = false;
+for (const auto *Ctor : BaseClass->ctors()) {
+  if (Ctor->isCopyConstructor() &&
+  (Ctor->getAccess() == AS_private || Ctor->isDeleted())) {
+NonCopyableBase = true;
+break;
+  }
+}
+if (NonCopyableBase)
+  continue;
+const auto *CExpr = dyn_cast(Init->getInit());
+if (!CExpr || !CExpr->getConstructor()->isDefaultConstructor())
+  continue;
+HasRelevantBaseInit = true;
+if (CtorInitIsWritten) {
+  if (!ParamName.empty())
+SafeFixIts.push_back(
+FixItHint::CreateInsertion(CExpr->getLocEnd(), ParamName));
+} else {
+  if (Init->getSourceLocation().isMacroID() ||
+  Ctor->getLocation().isMacroID() || ShouldNotDoFixit)
+break;
+  FixItInitList += BaseClass->getNameAsString();
+  FixItInitList += "(" + ParamName + "), ";
+}
+  }
+  if (!HasRelevantBaseInit)
+return;
+
+  auto Diag = diag(Ctor->getLocation(),
+   "calling a base constructor other than the copy constructor")
+  << SafeFixIts;
+
+  if (FixItInitList.empty() || ParamName.empty() || ShouldNotDoFixit)
+return;
+
+  std::string 

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Aside from two minor nits, the check LGTM. Whether we put it in misc or 
bugprone can be answered by @alexfh or by your best judgement.




Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:61
+ShouldNotDoFixit = ShouldNotDoFixit || CtorInitIsWritten;
+const auto *BaseClass = BaseType->getAsCXXRecordDecl()->getDefinition();
+if (BaseClass->field_empty() &&

Please don't use `auto` here.



Comment at: docs/clang-tidy/checks/misc-copy-constructor-init.rst:29
+
+The check suggests a fix-it in every scenario including multiple 
+missing initializers and constructors with template argument.

This comment is no longer accurate (there are some times we don't supply a 
fix-it).


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:69
+  (Ctor->getAccess() == AS_private || Ctor->isDeleted())) {
+NonCopyableBase = true;
+break;

xazax.hun wrote:
> aaron.ballman wrote:
> > What if the base class is inherited privately? e.g.,
> > ```
> > struct Base {
> >   Base(const Base&) {}
> > };
> > 
> > struct Derived : private Base {
> >   Derived(const Derived &) {}
> > };
> > ```
> We warn in that case too. I added a test to demonstrate this. I think we 
> still want to copy private bases in copy ctors if they are not empty and 
> copyable. 
Good, thank you for adding the test (and I agree, we want to warn in that case).


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D33722#916990, @aaron.ballman wrote:

> In https://reviews.llvm.org/D33722#916540, @xazax.hun wrote:
>
> > Also, bugprone might be a better module to put this?
>
>
> I don't have strong opinions on misc vs bugprone (they're both effectively 
> catch-alls for tidy checks, as best I can tell).


@alexfh, do you have an opinion here?




Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:69
+  (Ctor->getAccess() == AS_private || Ctor->isDeleted())) {
+NonCopyableBase = true;
+break;

aaron.ballman wrote:
> What if the base class is inherited privately? e.g.,
> ```
> struct Base {
>   Base(const Base&) {}
> };
> 
> struct Derived : private Base {
>   Derived(const Derived &) {}
> };
> ```
We warn in that case too. I added a test to demonstrate this. I think we still 
want to copy private bases in copy ctors if they are not empty and copyable. 


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 121886.
xazax.hun marked 2 inline comments as done.
xazax.hun added a comment.

- Fix doc comments that I overlooked earlier


https://reviews.llvm.org/D33722

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/CopyConstructorInitCheck.cpp
  clang-tidy/misc/CopyConstructorInitCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-copy-constructor-init.rst
  test/clang-tidy/misc-copy-constructor-init.cpp

Index: test/clang-tidy/misc-copy-constructor-init.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-copy-constructor-init.cpp
@@ -0,0 +1,188 @@
+// RUN: %check_clang_tidy %s misc-copy-constructor-init %t
+
+class NonCopyable {
+public:
+  NonCopyable() = default;
+  NonCopyable(const NonCopyable &) = delete;
+
+private:
+  int a;
+};
+
+class NonCopyable2 {
+public:
+  NonCopyable2() = default;
+
+private:
+  NonCopyable2(const NonCopyable2 &);
+  int a;
+};
+
+class Copyable {
+public:
+  Copyable() = default;
+  Copyable(const Copyable &) = default;
+
+private:
+  int a;
+};
+
+class Copyable2 {
+public:
+  Copyable2() = default;
+  Copyable2(const Copyable2 &) = default;
+
+private:
+  int a;
+};
+
+class Copyable3 : public Copyable {
+public:
+  Copyable3() = default;
+  Copyable3(const Copyable3 &) = default;
+};
+
+template 
+class Copyable4 {
+public:
+  Copyable4() = default;
+  Copyable4(const Copyable4 &) = default;
+
+private:
+  int a;
+};
+
+template 
+class Copyable5 {
+public:
+  Copyable5() = default;
+  Copyable5(const Copyable5 &) = default;
+
+private:
+  int a;
+};
+
+class EmptyCopyable {
+public:
+  EmptyCopyable() = default;
+  EmptyCopyable(const EmptyCopyable &) = default;
+};
+
+template 
+using CopyableAlias = Copyable5;
+
+typedef Copyable5 CopyableAlias2;
+
+class X : public Copyable, public EmptyCopyable {
+  X(const X ) : Copyable(other) {}
+};
+
+class X2 : public Copyable2 {
+  X2(const X2 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor other than the copy constructor [misc-copy-constructor-init]
+  // CHECK-FIXES: X2(const X2 )  : Copyable2(other) {}
+};
+
+class X2_A : public Copyable2 {
+  X2_A(const X2_A &) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X2_A(const X2_A &) {}
+};
+
+class X3 : public Copyable, public Copyable2 {
+  X3(const X3 ) : Copyable(other) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X3(const X3 ) : Copyable(other) {}
+};
+
+class X4 : public Copyable {
+  X4(const X4 ) : Copyable() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X4(const X4 ) : Copyable(other) {}
+};
+
+class X5 : public Copyable3 {
+  X5(const X5 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X5(const X5 )  : Copyable3(other) {}
+};
+
+class X6 : public Copyable2, public Copyable3 {
+  X6(const X6 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X6(const X6 )  : Copyable2(other), Copyable3(other) {}
+};
+
+class X7 : public Copyable, public Copyable2 {
+  X7(const X7 ) : Copyable() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X7(const X7 ) : Copyable(other) {}
+};
+
+class X8 : public Copyable4 {
+  X8(const X8 ) : Copyable4(other) {}
+};
+
+class X9 : public Copyable4 {
+  X9(const X9 ) : Copyable4() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X9(const X9 ) : Copyable4(other) {}
+};
+
+class X10 : public Copyable4 {
+  X10(const X10 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X10(const X10 )  : Copyable4(other) {}
+};
+
+class X11 : public Copyable5 {
+  X11(const X11 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X11(const X11 )  : Copyable5(other) {}
+};
+
+class X12 : public CopyableAlias {
+  X12(const X12 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X12(const X12 ) {}
+};
+
+template 
+class X13 : T {
+  X13(const X13 ) {}
+};
+
+template class X13;
+template class X13;
+
+#define FROMMACRO\
+  class X14 : public Copyable2 { \
+X14(const X14 ) {} \
+  };
+
+FROMMACRO
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: calling a base constructor
+
+class X15 : public CopyableAlias2 {
+  X15(const X15 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X15(const X15 ) {}
+};
+
+class X16 : public NonCopyable {
+  X16(const X16 ) {}
+};
+
+class X17 : public NonCopyable2 {
+  X17(const X17 ) {}
+};
+
+class X18 : private Copyable {
+  X18(const X18 ) {}
+  // 

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 121885.
xazax.hun marked 3 inline comments as done.
xazax.hun added a comment.

- Fix review comments


https://reviews.llvm.org/D33722

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/CopyConstructorInitCheck.cpp
  clang-tidy/misc/CopyConstructorInitCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-copy-constructor-init.rst
  test/clang-tidy/misc-copy-constructor-init.cpp

Index: test/clang-tidy/misc-copy-constructor-init.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-copy-constructor-init.cpp
@@ -0,0 +1,188 @@
+// RUN: %check_clang_tidy %s misc-copy-constructor-init %t
+
+class NonCopyable {
+public:
+  NonCopyable() = default;
+  NonCopyable(const NonCopyable &) = delete;
+
+private:
+  int a;
+};
+
+class NonCopyable2 {
+public:
+  NonCopyable2() = default;
+
+private:
+  NonCopyable2(const NonCopyable2 &);
+  int a;
+};
+
+class Copyable {
+public:
+  Copyable() = default;
+  Copyable(const Copyable &) = default;
+
+private:
+  int a;
+};
+
+class Copyable2 {
+public:
+  Copyable2() = default;
+  Copyable2(const Copyable2 &) = default;
+
+private:
+  int a;
+};
+
+class Copyable3 : public Copyable {
+public:
+  Copyable3() = default;
+  Copyable3(const Copyable3 &) = default;
+};
+
+template 
+class Copyable4 {
+public:
+  Copyable4() = default;
+  Copyable4(const Copyable4 &) = default;
+
+private:
+  int a;
+};
+
+template 
+class Copyable5 {
+public:
+  Copyable5() = default;
+  Copyable5(const Copyable5 &) = default;
+
+private:
+  int a;
+};
+
+class EmptyCopyable {
+public:
+  EmptyCopyable() = default;
+  EmptyCopyable(const EmptyCopyable &) = default;
+};
+
+template 
+using CopyableAlias = Copyable5;
+
+typedef Copyable5 CopyableAlias2;
+
+class X : public Copyable, public EmptyCopyable {
+  X(const X ) : Copyable(other) {}
+};
+
+class X2 : public Copyable2 {
+  X2(const X2 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor other than the copy constructor [misc-copy-constructor-init]
+  // CHECK-FIXES: X2(const X2 )  : Copyable2(other) {}
+};
+
+class X2_A : public Copyable2 {
+  X2_A(const X2_A &) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X2_A(const X2_A &) {}
+};
+
+class X3 : public Copyable, public Copyable2 {
+  X3(const X3 ) : Copyable(other) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X3(const X3 ) : Copyable(other) {}
+};
+
+class X4 : public Copyable {
+  X4(const X4 ) : Copyable() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X4(const X4 ) : Copyable(other) {}
+};
+
+class X5 : public Copyable3 {
+  X5(const X5 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X5(const X5 )  : Copyable3(other) {}
+};
+
+class X6 : public Copyable2, public Copyable3 {
+  X6(const X6 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X6(const X6 )  : Copyable2(other), Copyable3(other) {}
+};
+
+class X7 : public Copyable, public Copyable2 {
+  X7(const X7 ) : Copyable() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X7(const X7 ) : Copyable(other) {}
+};
+
+class X8 : public Copyable4 {
+  X8(const X8 ) : Copyable4(other) {}
+};
+
+class X9 : public Copyable4 {
+  X9(const X9 ) : Copyable4() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X9(const X9 ) : Copyable4(other) {}
+};
+
+class X10 : public Copyable4 {
+  X10(const X10 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X10(const X10 )  : Copyable4(other) {}
+};
+
+class X11 : public Copyable5 {
+  X11(const X11 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X11(const X11 )  : Copyable5(other) {}
+};
+
+class X12 : public CopyableAlias {
+  X12(const X12 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X12(const X12 ) {}
+};
+
+template 
+class X13 : T {
+  X13(const X13 ) {}
+};
+
+template class X13;
+template class X13;
+
+#define FROMMACRO\
+  class X14 : public Copyable2 { \
+X14(const X14 ) {} \
+  };
+
+FROMMACRO
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: calling a base constructor
+
+class X15 : public CopyableAlias2 {
+  X15(const X15 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X15(const X15 ) {}
+};
+
+class X16 : public NonCopyable {
+  X16(const X16 ) {}
+};
+
+class X17 : public NonCopyable2 {
+  X17(const X17 ) {}
+};
+
+class X18 : private Copyable {
+  X18(const X18 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: 

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D33722#916540, @xazax.hun wrote:

> Also, bugprone might be a better module to put this?


I don't have strong opinions on misc vs bugprone (they're both effectively 
catch-alls for tidy checks, as best I can tell).


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D33722#916539, @xazax.hun wrote:

> In https://reviews.llvm.org/D33722#915440, @aaron.ballman wrote:
>
> > I disagree. We should not be changing code to be incorrect, requiring the 
> > user to find that next incorrectness only through an additional 
> > compilation. That is a rather poor user experience.
>
>
> The resulting code is not incorrect. We do not introduce undefined behavior.


Undefined behavior is not the only form of incorrectness. The resulting code is 
incorrect because the initalization order does not match the declaration order 
and relying on that behavior leads to bugs, which is why -Wreorder is on by 
default.

> But if we do not want to introduce new warnings either, I could skip the 
> fixits if there is an initializer already for any of the base classes. What 
> do you think?

I think that's reasonable behavior.




Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:38-40
+  if (!Class->field_empty())
+return false;
+  return true;

I'm not certain this function is all that useful -- it could be replaced with 
`Class->field_empty()` in the caller (even as a lambda if needed).



Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:69
+  (Ctor->getAccess() == AS_private || Ctor->isDeleted())) {
+NonCopyableBase = true;
+break;

What if the base class is inherited privately? e.g.,
```
struct Base {
  Base(const Base&) {}
};

struct Derived : private Base {
  Derived(const Derived &) {}
};
```



Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:109
+  } else {
+// We apply the missing ctros at the beginning of the initialization list.
+FixItLoc = (*Ctor->init_begin())->getSourceLocation();

ctros -> ctors



Comment at: docs/clang-tidy/checks/misc-copy-constructor-init.rst:4
+misc-copy-constructor-init
+=
+

The markdown here is wrong.



Comment at: docs/clang-tidy/checks/misc-copy-constructor-init.rst:6
+
+Finds copy constructors where the constructor don't call 
+the constructor of the base class.

don't -> doesn't


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Also, bugprone might be a better module to put this?


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D33722#915440, @aaron.ballman wrote:

> In the other cases, it is not clear that the re-lexed information should be 
> carried in the AST. In this case, I think it's pretty clear that the AST 
> should carry this information. Further, I don't know that the re-lexing is 
> correct (I pointed out an example that I think will be problematic) and 
> carrying the information in the AST would solve that more cleanly than trying 
> to reimplement the logic here.


Oh, you are right! Sorry for my oversight, I did not notice your example. I got 
a bit lost in all of the comments. Fortunately, I was able to get rid of the 
relexing, so this should be ok now.

>> I am not sure that we should complicate the fixit logic with the order. If 
>> -Wreorder has fixit hints, user should be able to get rid of the warning by 
>> applying that.
> 
> I disagree. We should not be changing code to be incorrect, requiring the 
> user to find that next incorrectness only through an additional compilation. 
> That is a rather poor user experience.

The resulting code is not incorrect. We do not introduce undefined behavior, 
nor change the meaning of the code. But if we do not want to introduce new 
warnings either, I could skip the fixits if there is an initializer already for 
any of the base classes. What do you think?


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 121698.
xazax.hun added a comment.

- Do not warn for NonCopyable bases
- Remove lexing


https://reviews.llvm.org/D33722

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/CopyConstructorInitCheck.cpp
  clang-tidy/misc/CopyConstructorInitCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-copy-constructor-init.rst
  test/clang-tidy/misc-copy-constructor-init.cpp

Index: test/clang-tidy/misc-copy-constructor-init.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-copy-constructor-init.cpp
@@ -0,0 +1,178 @@
+// RUN: %check_clang_tidy %s misc-copy-constructor-init %t
+
+class NonCopyable {
+public:
+  NonCopyable() = default;
+  NonCopyable(const NonCopyable &) = delete;
+
+private:
+  int a;
+};
+
+class NonCopyable2 {
+public:
+  NonCopyable2() = default;
+
+private:
+  NonCopyable2(const NonCopyable2 &);
+  int a;
+};
+
+class Copyable {
+public:
+  Copyable() = default;
+  Copyable(const Copyable &) = default;
+
+private:
+  int a;
+};
+
+class Copyable2 {
+public:
+  Copyable2() = default;
+  Copyable2(const Copyable2 &) = default;
+
+private:
+  int a;
+};
+
+class Copyable3 : public Copyable {
+public:
+  Copyable3() = default;
+  Copyable3(const Copyable3 &) = default;
+};
+
+template 
+class Copyable4 {
+public:
+  Copyable4() = default;
+  Copyable4(const Copyable4 &) = default;
+
+private:
+  int a;
+};
+
+template 
+class Copyable5 {
+public:
+  Copyable5() = default;
+  Copyable5(const Copyable5 &) = default;
+
+private:
+  int a;
+};
+
+class EmptyCopyable {
+public:
+  EmptyCopyable() = default;
+  EmptyCopyable(const EmptyCopyable &) = default;
+};
+
+template 
+using CopyableAlias = Copyable5;
+
+typedef Copyable5 CopyableAlias2;
+
+class X : public Copyable, public EmptyCopyable {
+  X(const X ) : Copyable(other) {}
+};
+
+class X2 : public Copyable2 {
+  X2(const X2 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor other than the copy constructor [misc-copy-constructor-init]
+  // CHECK-FIXES: X2(const X2 )  : Copyable2(other) {}
+};
+
+class X2_A : public Copyable2 {
+  X2_A(const X2_A &) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X2_A(const X2_A &) {}
+};
+
+class X3 : public Copyable, public Copyable2 {
+  X3(const X3 ) : Copyable(other) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X3(const X3 ) : Copyable2(other), Copyable(other) {}
+};
+
+class X4 : public Copyable {
+  X4(const X4 ) : Copyable() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X4(const X4 ) : Copyable(other) {}
+};
+
+class X5 : public Copyable3 {
+  X5(const X5 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X5(const X5 )  : Copyable3(other) {}
+};
+
+class X6 : public Copyable2, public Copyable3 {
+  X6(const X6 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X6(const X6 )  : Copyable2(other), Copyable3(other) {}
+};
+
+class X7 : public Copyable, public Copyable2 {
+  X7(const X7 ) : Copyable() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X7(const X7 ) : Copyable2(other), Copyable(other) {}
+};
+
+class X8 : public Copyable4 {
+  X8(const X8 ) : Copyable4(other) {}
+};
+
+class X9 : public Copyable4 {
+  X9(const X9 ) : Copyable4() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X9(const X9 ) : Copyable4(other) {}
+};
+
+class X10 : public Copyable4 {
+  X10(const X10 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X10(const X10 )  : Copyable4(other) {}
+};
+
+class X11 : public Copyable5 {
+  X11(const X11 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X11(const X11 )  : Copyable5(other) {}
+};
+
+class X12 : public CopyableAlias {
+  X12(const X12 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X12(const X12 ) {}
+};
+
+template 
+class X13 : T {
+  X13(const X13 ) {}
+};
+
+template class X13;
+template class X13;
+
+#define FROMMACRO\
+  class X14 : public Copyable2 { \
+X14(const X14 ) {} \
+  };
+
+FROMMACRO
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: calling a base constructor
+
+class X15 : public CopyableAlias2 {
+  X15(const X15 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X15(const X15 ) {}
+};
+
+class X16 : public NonCopyable {
+  X16(const X16 ) {}
+};
+
+class X17 : public NonCopyable2 {
+  X17(const X17 ) {}
+};
Index: docs/clang-tidy/checks/misc-copy-constructor-init.rst

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D33722#915134, @xazax.hun wrote:

> Two problems are not resolved. One is Aaron prefers to query some infor from 
> the AST instead of relexing. The second is providing base initializers in the 
> wrong order.
>  I think there are other checks that do relexing in some cases, this should 
> not be a blocker.


In the other cases, it is not clear that the re-lexed information should be 
carried in the AST. In this case, I think it's pretty clear that the AST should 
carry this information. Further, I don't know that the re-lexing is correct (I 
pointed out an example that I think will be problematic) and carrying the 
information in the AST would solve that more cleanly than trying to reimplement 
the logic here.

> I am not sure that we should complicate the fixit logic with the order. If 
> -Wreorder has fixit hints, user should be able to get rid of the warning by 
> applying that.

I disagree. We should not be changing code to be incorrect, requiring the user 
to find that next incorrectness only through an additional compilation. That is 
a rather poor user experience.


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked 5 inline comments as done.
xazax.hun added a comment.

Two problems are not resolved. One is Aaron prefers to query some infor from 
the AST instead of relexing. The second is providing base initializers in the 
wrong order.
I think there are other checks that do relexing in some cases, this should not 
be a blocker. I am not sure that we should complicate the fixit logic with the 
order. If -Wreorder has fixit hints, user should be able to get rid of the 
warning by applying that.


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 121477.
xazax.hun added a comment.

- Dominic said he no longer have time to continue with this patch, so I 
commandeered this revision
- Skip template instantiations
- Do not attempt fix macro expansions
- Do not attempt fix type aliases and typedef types
- Do not attempt to fix cases where the parameter is unnamed
- Use a more conservative approach and only warn for default constructors 
invoked by copy ctors. This could be refined in a followup patch.
- Skip the check for empty base classes
- Do not use matchers in the check callback
- Add tests for relevant changes
- Fix style issues in tests and docs
- Rebased for latest ToT


https://reviews.llvm.org/D33722

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/CopyConstructorInitCheck.cpp
  clang-tidy/misc/CopyConstructorInitCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-copy-constructor-init.rst
  test/clang-tidy/misc-copy-constructor-init.cpp

Index: test/clang-tidy/misc-copy-constructor-init.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-copy-constructor-init.cpp
@@ -0,0 +1,152 @@
+// RUN: %check_clang_tidy %s misc-copy-constructor-init %t
+
+class Copyable {
+public:
+  Copyable() = default;
+  Copyable(const Copyable &) = default;
+
+private:
+  int a;
+};
+
+class Copyable2 {
+public:
+  Copyable2() = default;
+  Copyable2(const Copyable2 &) = default;
+
+private:
+  int a;
+};
+
+class Copyable3 : public Copyable {
+public:
+  Copyable3() = default;
+  Copyable3(const Copyable3 &) = default;
+};
+
+template 
+class Copyable4 {
+public:
+  Copyable4() = default;
+  Copyable4(const Copyable4 &) = default;
+
+private:
+  int a;
+};
+
+template 
+class Copyable5 {
+public:
+  Copyable5() = default;
+  Copyable5(const Copyable5 &) = default;
+
+private:
+  int a;
+};
+
+class EmptyCopyable {
+public:
+  EmptyCopyable() = default;
+  EmptyCopyable(const EmptyCopyable &) = default;
+};
+
+template 
+using CopyableAlias = Copyable5;
+
+typedef Copyable5 CopyableAlias2;
+
+class X : public Copyable, public EmptyCopyable {
+  X(const X ) : Copyable(other) {}
+};
+
+class X2 : public Copyable2 {
+  X2(const X2 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor other than the copy constructor [misc-copy-constructor-init]
+  // CHECK-FIXES: X2(const X2 )  : Copyable2(other) {}
+};
+
+class X2_A : public Copyable2 {
+  X2_A(const X2_A &) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X2_A(const X2_A &) {}
+};
+
+class X3 : public Copyable, public Copyable2 {
+  X3(const X3 ) : Copyable(other) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X3(const X3 ) : Copyable2(other), Copyable(other) {}
+};
+
+class X4 : public Copyable {
+  X4(const X4 ) : Copyable() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X4(const X4 ) : Copyable(other) {}
+};
+
+class X5 : public Copyable3 {
+  X5(const X5 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X5(const X5 )  : Copyable3(other) {}
+};
+
+class X6 : public Copyable2, public Copyable3 {
+  X6(const X6 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X6(const X6 )  : Copyable2(other), Copyable3(other) {}
+};
+
+class X7 : public Copyable, public Copyable2 {
+  X7(const X7 ) : Copyable() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X7(const X7 ) : Copyable2(other), Copyable(other) {}
+};
+
+class X8 : public Copyable4 {
+  X8(const X8 ) : Copyable4(other) {}
+};
+
+class X9 : public Copyable4 {
+  X9(const X9 ) : Copyable4() {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X9(const X9 ) : Copyable4(other) {}
+};
+
+class X10 : public Copyable4 {
+  X10(const X10 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X10(const X10 )  : Copyable4(other) {}
+};
+
+class X11 : public Copyable5 {
+  X11(const X11 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X11(const X11 )  : Copyable5(other) {}
+};
+
+class X12 : public CopyableAlias {
+  X12(const X12 ) {}
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: calling a base constructor
+  // CHECK-FIXES: X12(const X12 ) {}
+};
+
+template 
+class X13 : T {
+  X13(const X13 ) {}
+};
+
+template class X13;
+template class X13;
+
+#define FROMMACRO\
+  class X14 : public Copyable2 { \
+X14(const X14 ) {} \
+  };
+
+FROMMACRO
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: calling a base constructor
+
+class X15 : public CopyableAlias2 {
+  X15(const X15 ) {}
+  // CHECK-MESSAGES: 

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:104
+
+  diag(Tok.getLocation(),
+   "calling an inherited constructor other than the copy constructor")

szdominik wrote:
> aaron.ballman wrote:
> > Insteaad of having to re-lex the physical source, can the AST should be 
> > modified to carry the information you need if it doesn't already have it? 
> > For instance, you can tell there is not initializer list by looking at 
> > `CXXConstructorDecl::getNumCtorInitializers()`.
> The getNumCtorInitializers method counts the generated initializers as well, 
> not just the manually written ones.
> My basic problem was that the AST's methods can't really distinguish the 
> generated and the manually written initializers, so it's quite complicated to 
> find the locations what we need. I found easier & more understandable if I 
> start reading the tokens.
This sounds like a deficiency with the AST that should be rectified rather than 
worked around. Going back to lexing the source can be very expensive (think 
about source files that live on a network drive, for instance) and is often 
tricky to get correct. For instance, it seems the lexing starts at the 
constructor declaration itself, so does a default argument to that copy 
constructor using `?:` cause issues? e.g., `S(const S&, int = 0 ? 1 : 2)`



Comment at: test/clang-tidy/misc-copy-constructor-init.cpp:27
+   // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: calling an inherited 
constructor other than the copy constructor [misc-copy-constructor-init]
+   // CHECK-FIXES: X3(const X3& other): Copyable2(other), Copyable(other) 
{};
+};

szdominik wrote:
> aaron.ballman wrote:
> > Don't we want the ctor-inits to be in the same order as the bases are 
> > specified?
> I think it's more readable if we put the FixIts to the beginning of the 
> expression - it's easier to check that everyting is correct & it's more 
> obvious what the changes are.
However, that then produces additional warnings because the ctor-inits are not 
in the canonical order (`-Wreorder`). See 
http://coliru.stacked-crooked.com/a/a9d77afe87618c13



Comment at: test/clang-tidy/misc-copy-constructor-init.cpp:9
+class X : public Copyable {
+   X(const X& other) : Copyable(other) {}
+   //Good code: the copy ctor call the ctor of the base class.

Please clang-format this file so it meets our usual formatting requirements.



Comment at: test/clang-tidy/misc-copy-constructor-init.cpp:19
+class X2 : public Copyable2 {
+   X2(const X2& other) {};
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited 
constructor other than the copy constructor [misc-copy-constructor-init]

Spurious semicolon.



Comment at: test/clang-tidy/misc-copy-constructor-init.cpp:25
+class X3 : public Copyable, public Copyable2 {
+   X3(const X3& other): Copyable(other) {};
+   // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: calling an inherited 
constructor other than the copy constructor [misc-copy-constructor-init]

Spurious semicolon (check the remainder of the file, this seems to be a common 
issue).


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Dominik Szabó via Phabricator via cfe-commits]

szdominik added inline comments.



Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:37
+
+  // We match here because we want one warning (and FixIt) for every ctor.
+  const auto Matches = match(

aaron.ballman wrote:
> Wouldn't registering this matcher achieve the same goal instead of needing to 
> re-match?
I wanted to create //only one// FixIt to every ctor - if I move the 
forEachCtorInitializer to the register part, it would warn us twice.



Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:104
+
+  diag(Tok.getLocation(),
+   "calling an inherited constructor other than the copy constructor")

aaron.ballman wrote:
> Insteaad of having to re-lex the physical source, can the AST should be 
> modified to carry the information you need if it doesn't already have it? For 
> instance, you can tell there is not initializer list by looking at 
> `CXXConstructorDecl::getNumCtorInitializers()`.
The getNumCtorInitializers method counts the generated initializers as well, 
not just the manually written ones.
My basic problem was that the AST's methods can't really distinguish the 
generated and the manually written initializers, so it's quite complicated to 
find the locations what we need. I found easier & more understandable if I 
start reading the tokens.



Comment at: test/clang-tidy/misc-copy-constructor-init.cpp:27
+   // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: calling an inherited 
constructor other than the copy constructor [misc-copy-constructor-init]
+   // CHECK-FIXES: X3(const X3& other): Copyable2(other), Copyable(other) 
{};
+};

aaron.ballman wrote:
> Don't we want the ctor-inits to be in the same order as the bases are 
> specified?
I think it's more readable if we put the FixIts to the beginning of the 
expression - it's easier to check that everyting is correct & it's more obvious 
what the changes are.


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Dominik Szabó via Phabricator via cfe-commits]

szdominik updated this revision to Diff 118617.
szdominik marked 4 inline comments as done.
szdominik added a comment.

Small changes after aaron.ballman's comments.


https://reviews.llvm.org/D33722

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/CopyConstructorInitCheck.cpp
  clang-tidy/misc/CopyConstructorInitCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-copy-constructor-init.rst
  test/clang-tidy/misc-copy-constructor-init.cpp

Index: test/clang-tidy/misc-copy-constructor-init.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-copy-constructor-init.cpp
@@ -0,0 +1,95 @@
+// RUN: %check_clang_tidy %s misc-copy-constructor-init %t
+
+class Copyable {
+	public:
+	Copyable() = default;
+	Copyable(const Copyable&) = default;
+};
+class X : public Copyable {
+	X(const X& other) : Copyable(other) {}
+	//Good code: the copy ctor call the ctor of the base class.
+};
+
+class Copyable2 {
+	public:
+	Copyable2() = default;
+	Copyable2(const Copyable2&) = default;
+};
+class X2 : public Copyable2 {
+	X2(const X2& other) {};
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X2(const X2& other) : Copyable2(other) {};
+};
+
+class X3 : public Copyable, public Copyable2 {
+	X3(const X3& other): Copyable(other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X3(const X3& other): Copyable2(other), Copyable(other) {};
+};
+
+class X4 : public Copyable {
+	X4(const X4& other): Copyable() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X4(const X4& other): Copyable(other) {};
+};
+
+class Copyable3 : public Copyable {
+	public:
+	Copyable3() = default;
+	Copyable3(const Copyable3&) = default;
+};
+class X5 : public Copyable3 {
+	X5(const X5& other) {};
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X5(const X5& other) : Copyable3(other) {};
+};
+
+class X6 : public Copyable2, public Copyable3 {
+	X6(const X6& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X6(const X6& other) : Copyable2(other), Copyable3(other) {};
+};
+
+class X7 : public Copyable, public Copyable2 {
+	X7(const X7& other): Copyable() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-MESSAGES: :[[@LINE-2]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X7(const X7& other): Copyable2(other), Copyable(other) {};
+};
+
+template 
+class Copyable4 {
+	public:
+	Copyable4() = default;
+	Copyable4(const Copyable4&) = default;
+};
+
+class X8 : public Copyable4 {
+	X8(const X8& other): Copyable4(other) {};
+	//Good code: the copy ctor call the ctor of the base class.
+};
+
+class X9 : public Copyable4 {
+	X9(const X9& other): Copyable4() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X9(const X9& other): Copyable4(other) {};
+};
+
+class X10 : public Copyable4 {
+	X10(const X10& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X10(const X10& other) : Copyable4(other) {};
+};
+
+template 
+class Copyable5 {
+	public:
+	Copyable5() = default;
+	Copyable5(const Copyable5&) = default;
+};
+
+class X11 : public Copyable5 {
+	X11(const X11& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X11(const X11& other) : Copyable5(other) {};
+};
Index: docs/clang-tidy/checks/misc-copy-constructor-init.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-copy-constructor-init.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - misc-copy-constructor-init
+
+misc-copy-constructor-init
+=
+
+Finds copy constructors where the constructor don't call 
+the constructor of the base class.
+
+.. code-block:: c++
+
+class Copyable {
+public:
+Copyable() = default;
+Copyable(const Copyable&) = default;
+};
+class X2 : public Copyable {
+X2(const X2& other) {}; // 

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

The run on llvm indicates that we don't want this to trigger if the base class 
doesn't have anything to copy (that is, no fields & a defaulted copy-ctor, or 
an empty copy-ctor).


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:24
+withInitializer(cxxConstructExpr(unless(hasDescendant(implicitCastExpr(
+.bind("cruct-expr")));
+

You pick a more readable name than `cruct-expr`, like `construct-expr`?



Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:37
+
+  // We match here because we want one warning (and FixIt) for every ctor.
+  const auto Matches = match(

Wouldn't registering this matcher achieve the same goal instead of needing to 
re-match?



Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:59
+  // We want to write in the FixIt the template arguments too.
+  if (const auto *Decl = dyn_cast(
+  Init->getBaseClass()->getAsCXXRecordDecl())) {

Please pick a name other than `Decl`, since that's a type name.



Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:80
+
+  auto  = Result.Context->getSourceManager();
+  SourceLocation StartLoc = Ctor->getLocation();

Please do not use `auto` here.



Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:95
+  if (Tok.is(tok::l_brace))
+FixItMsg += ": ";
+  FixItMsg += FixItInitList;

Space before the colon?



Comment at: clang-tidy/misc/CopyConstructorInitCheck.cpp:104
+
+  diag(Tok.getLocation(),
+   "calling an inherited constructor other than the copy constructor")

Insteaad of having to re-lex the physical source, can the AST should be 
modified to carry the information you need if it doesn't already have it? For 
instance, you can tell there is not initializer list by looking at 
`CXXConstructorDecl::getNumCtorInitializers()`.



Comment at: test/clang-tidy/misc-copy-constructor-init.cpp:27
+   // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: calling an inherited 
constructor other than the copy constructor [misc-copy-constructor-init]
+   // CHECK-FIXES: X3(const X3& other): Copyable2(other), Copyable(other) 
{};
+};

Don't we want the ctor-inits to be in the same order as the bases are specified?


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Dominik Szabó via Phabricator via cfe-commits]

szdominik added a comment.

There wasn't any update on this check lately - can I help to make it better?


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Dominik Szabó via Phabricator via cfe-commits]

szdominik updated this revision to Diff 109564.
szdominik marked 2 inline comments as done.
szdominik added a comment.
Herald added a subscriber: JDevlieghere.

Fixed check-fixes lines in test cases.
Updated matcher definition.


https://reviews.llvm.org/D33722

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/CopyConstructorInitCheck.cpp
  clang-tidy/misc/CopyConstructorInitCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-copy-constructor-init.rst
  test/clang-tidy/misc-copy-constructor-init.cpp

Index: test/clang-tidy/misc-copy-constructor-init.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-copy-constructor-init.cpp
@@ -0,0 +1,95 @@
+// RUN: %check_clang_tidy %s misc-copy-constructor-init %t
+
+class Copyable {
+	public:
+	Copyable() = default;
+	Copyable(const Copyable&) = default;
+};
+class X : public Copyable {
+	X(const X& other) : Copyable(other) {}
+	//Good code: the copy ctor call the ctor of the base class.
+};
+
+class Copyable2 {
+	public:
+	Copyable2() = default;
+	Copyable2(const Copyable2&) = default;
+};
+class X2 : public Copyable2 {
+	X2(const X2& other) {};
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X2(const X2& other) : Copyable2(other) {};
+};
+
+class X3 : public Copyable, public Copyable2 {
+	X3(const X3& other): Copyable(other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X3(const X3& other): Copyable2(other), Copyable(other) {};
+};
+
+class X4 : public Copyable {
+	X4(const X4& other): Copyable() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X4(const X4& other): Copyable(other) {};
+};
+
+class Copyable3 : public Copyable {
+	public:
+	Copyable3() = default;
+	Copyable3(const Copyable3&) = default;
+};
+class X5 : public Copyable3 {
+	X5(const X5& other) {};
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X5(const X5& other) : Copyable3(other) {};
+};
+
+class X6 : public Copyable2, public Copyable3 {
+	X6(const X6& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X6(const X6& other) : Copyable2(other), Copyable3(other) {};
+};
+
+class X7 : public Copyable, public Copyable2 {
+	X7(const X7& other): Copyable() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-MESSAGES: :[[@LINE-2]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X7(const X7& other): Copyable2(other), Copyable(other) {};
+};
+
+template 
+class Copyable4 {
+	public:
+	Copyable4() = default;
+	Copyable4(const Copyable4&) = default;
+};
+
+class X8 : public Copyable4 {
+	X8(const X8& other): Copyable4(other) {};
+	//Good code: the copy ctor call the ctor of the base class.
+};
+
+class X9 : public Copyable4 {
+	X9(const X9& other): Copyable4() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X9(const X9& other): Copyable4(other) {};
+};
+
+class X10 : public Copyable4 {
+	X10(const X10& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X10(const X10& other) : Copyable4(other) {};
+};
+
+template 
+class Copyable5 {
+	public:
+	Copyable5() = default;
+	Copyable5(const Copyable5&) = default;
+};
+
+class X11 : public Copyable5 {
+	X11(const X11& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: X11(const X11& other) : Copyable5(other) {};
+};
Index: docs/clang-tidy/checks/misc-copy-constructor-init.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-copy-constructor-init.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - misc-copy-constructor-init
+
+misc-copy-constructor-init
+=
+
+Finds copy constructors where the constructor don't call 
+the constructor of the base class.
+
+.. code-block:: c++
+
+class Copyable {
+public:
+Copyable() = default;
+Copyable(const Copyable&) = default;
+};
+class X2 : 

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Dominik Szabó via Phabricator via cfe-commits]

szdominik updated this revision to Diff 103385.
szdominik marked 4 inline comments as done.
szdominik added a comment.

Updated loop for searching the beginning of the initlist.


https://reviews.llvm.org/D33722

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/CopyConstructorInitCheck.cpp
  clang-tidy/misc/CopyConstructorInitCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-copy-constructor-init.rst
  test/clang-tidy/misc-copy-constructor-init.cpp

Index: test/clang-tidy/misc-copy-constructor-init.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-copy-constructor-init.cpp
@@ -0,0 +1,96 @@
+// RUN: %check_clang_tidy %s misc-copy-constructor-init %t
+
+class Copyable {
+	public:
+	Copyable() = default;
+	Copyable(const Copyable&) = default;
+};
+class X : public Copyable {
+	X(const X& other) : Copyable(other) {}
+	//Good code: the copy ctor call the ctor of the base class.
+};
+
+class Copyable2 {
+	public:
+	Copyable2() = default;
+	Copyable2(const Copyable2&) = default;
+};
+class X2 : public Copyable2 {
+	X2(const X2& other) {};
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: : Copyable2(other)
+};
+
+class X3 : public Copyable, public Copyable2 {
+	X3(const X3& other): Copyable(other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: Copyable2(other), 
+};
+
+class X4 : public Copyable {
+	X4(const X4& other): Copyable() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: other
+};
+
+class Copyable3 : public Copyable {
+	public:
+	Copyable3() = default;
+	Copyable3(const Copyable3&) = default;
+};
+class X5 : public Copyable3 {
+	X5(const X5& other) {};
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: : Copyable3(other)
+};
+
+class X6 : public Copyable2, public Copyable3 {
+	X6(const X6& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: : Copyable2(other), Copyable3(other)
+};
+
+class X7 : public Copyable, public Copyable2 {
+	X7(const X7& other): Copyable() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: other
+	// CHECK-MESSAGES: :[[@LINE-3]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: Copyable2(other), 
+};
+
+template 
+class Copyable4 {
+	public:
+	Copyable4() = default;
+	Copyable4(const Copyable4&) = default;
+};
+
+class X8 : public Copyable4 {
+	X8(const X8& other): Copyable4(other) {};
+	//Good code: the copy ctor call the ctor of the base class.
+};
+
+class X9 : public Copyable4 {
+	X9(const X9& other): Copyable4() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: other
+};
+
+class X10 : public Copyable4 {
+	X10(const X10& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: : Copyable4(other)
+};
+
+template 
+class Copyable5 {
+	public:
+	Copyable5() = default;
+	Copyable5(const Copyable5&) = default;
+};
+
+class X11 : public Copyable5 {
+	X11(const X11& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: : Copyable5(other)
+};
Index: docs/clang-tidy/checks/misc-copy-constructor-init.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-copy-constructor-init.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - misc-copy-constructor-init
+
+misc-copy-constructor-init
+=
+
+Finds copy constructors where the constructor don't call 
+the constructor of the base class.
+
+.. code-block:: c++
+
+class Copyable {
+public:
+Copyable() = default;
+Copyable(const Copyable&) = default;
+};
+class X2 : public Copyable {
+X2(const X2& other) {}; // Copyable(other) is missing
+};
+
+Also finds copy constructors where the constructor of 
+the base class don't have parameter. 
+
+.. code-block:: c++
+
+class X4 : public Copyable {
+X4(const X4& other): Copyable() {}; // other is 

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Dominik Szabó via Phabricator via cfe-commits]

szdominik added inline comments.



Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:92
+  // Loop until the beginning of the initialization list.
+  while (!Tok.is(tok::r_paren))
+Lex.LexFromRawLexer(Tok);

aaron.ballman wrote:
> szdominik wrote:
> > aaron.ballman wrote:
> > > This doesn't balance tokens. What about:
> > > ```
> > > struct S {
> > >  /* ... */
> > > };
> > > 
> > > struct T : S {
> > >   T(const T ) : S((1 + 2), O) {}
> > > };
> > > ```
> > The loop search only the first right paren in the line, which is the end of 
> > parameter list ( "...&0**)**" ), so, as I see, it doesn't interesting what 
> > happens after the colon.
> Let me try again. :-)
> ```
> struct S {
>  /* ... */
> };
> 
> struct T : S {
>   T(const T , int = (1 + 2)) : S((1 + 2), O) {}
> };
> ```
Ah, you're right. I'll figure out something... :)


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:92
+  // Loop until the beginning of the initialization list.
+  while (!Tok.is(tok::r_paren))
+Lex.LexFromRawLexer(Tok);

szdominik wrote:
> aaron.ballman wrote:
> > This doesn't balance tokens. What about:
> > ```
> > struct S {
> >  /* ... */
> > };
> > 
> > struct T : S {
> >   T(const T ) : S((1 + 2), O) {}
> > };
> > ```
> The loop search only the first right paren in the line, which is the end of 
> parameter list ( "...&0**)**" ), so, as I see, it doesn't interesting what 
> happens after the colon.
Let me try again. :-)
```
struct S {
 /* ... */
};

struct T : S {
  T(const T , int = (1 + 2)) : S((1 + 2), O) {}
};
```


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Dominik Szabó via Phabricator via cfe-commits]

szdominik added inline comments.



Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:92
+  // Loop until the beginning of the initialization list.
+  while (!Tok.is(tok::r_paren))
+Lex.LexFromRawLexer(Tok);

aaron.ballman wrote:
> This doesn't balance tokens. What about:
> ```
> struct S {
>  /* ... */
> };
> 
> struct T : S {
>   T(const T ) : S((1 + 2), O) {}
> };
> ```
The loop search only the first right paren in the line, which is the end of 
parameter list ( "...&0**)**" ), so, as I see, it doesn't interesting what 
happens after the colon.


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Dominik Szabó via Phabricator via cfe-commits]

szdominik updated this revision to Diff 102807.
szdominik marked 9 inline comments as done.
szdominik added a comment.

Rename check.
Hoisted matcher, changed warning message & nits.


https://reviews.llvm.org/D33722

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/CopyConstructorInitCheck.cpp
  clang-tidy/misc/CopyConstructorInitCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-copy-constructor-init.rst
  test/clang-tidy/misc-copy-constructor-init.cpp

Index: test/clang-tidy/misc-copy-constructor-init.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-copy-constructor-init.cpp
@@ -0,0 +1,96 @@
+// RUN: %check_clang_tidy %s misc-copy-constructor-init %t
+
+class Copyable {
+	public:
+	Copyable() = default;
+	Copyable(const Copyable&) = default;
+};
+class X : public Copyable {
+	X(const X& other) : Copyable(other) {}
+	//Good code: the copy ctor call the ctor of the base class.
+};
+
+class Copyable2 {
+	public:
+	Copyable2() = default;
+	Copyable2(const Copyable2&) = default;
+};
+class X2 : public Copyable2 {
+	X2(const X2& other) {};
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: : Copyable2(other)
+};
+
+class X3 : public Copyable, public Copyable2 {
+	X3(const X3& other): Copyable(other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: Copyable2(other), 
+};
+
+class X4 : public Copyable {
+	X4(const X4& other): Copyable() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: other
+};
+
+class Copyable3 : public Copyable {
+	public:
+	Copyable3() = default;
+	Copyable3(const Copyable3&) = default;
+};
+class X5 : public Copyable3 {
+	X5(const X5& other) {};
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: : Copyable3(other)
+};
+
+class X6 : public Copyable2, public Copyable3 {
+	X6(const X6& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: : Copyable2(other), Copyable3(other)
+};
+
+class X7 : public Copyable, public Copyable2 {
+	X7(const X7& other): Copyable() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: other
+	// CHECK-MESSAGES: :[[@LINE-3]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: Copyable2(other), 
+};
+
+template 
+class Copyable4 {
+	public:
+	Copyable4() = default;
+	Copyable4(const Copyable4&) = default;
+};
+
+class X8 : public Copyable4 {
+	X8(const X8& other): Copyable4(other) {};
+	//Good code: the copy ctor call the ctor of the base class.
+};
+
+class X9 : public Copyable4 {
+	X9(const X9& other): Copyable4() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: other
+};
+
+class X10 : public Copyable4 {
+	X10(const X10& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: : Copyable4(other)
+};
+
+template 
+class Copyable5 {
+	public:
+	Copyable5() = default;
+	Copyable5(const Copyable5&) = default;
+};
+
+class X11 : public Copyable5 {
+	X11(const X11& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: calling an inherited constructor other than the copy constructor [misc-copy-constructor-init]
+	// CHECK-FIXES: : Copyable5(other)
+};
Index: docs/clang-tidy/checks/misc-copy-constructor-init.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-copy-constructor-init.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - misc-copy-constructor-init
+
+misc-copy-constructor-init
+=
+
+Finds copy constructors where the constructor don't call 
+the constructor of the base class.
+
+.. code-block:: c++
+
+class Copyable {
+public:
+Copyable() = default;
+Copyable(const Copyable&) = default;
+};
+class X2 : public Copyable {
+X2(const X2& other) {}; // Copyable(other) is missing
+};
+
+Also finds copy constructors where the constructor of 
+the base class don't have parameter. 
+
+.. code-block:: c++
+
+class X4 : public Copyable {
+X4(const X4& other): Copyable() {}; // other 

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added a comment.

This check is similar to `misc-move-constructor-init`; could it have a similar 
name please.


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added inline comments.
This revision now requires changes to proceed.



Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:41
+  forEachConstructorInitializer(
+  cxxCtorInitializer(
+  isBaseInitializer(),

It seems like you can hoist out from the `cxxCtorInitializer()` onward and only 
write this code once rather than twice.



Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:48
+
+  std::string FixItInitList = "";
+  for (const auto  : Matches) {

No need for the initializer.



Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:53
+// Valid when the initializer is written manually (not generated).
+if ((Init->getSourceRange()).isValid()) {
+  const auto *CExpr = Match.getNodeAs("cruct-expr");

Spurious parens.



Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:55
+  const auto *CExpr = Match.getNodeAs("cruct-expr");
+  diag(CExpr->getLocEnd(), "Missing parameter in the base initializer!")
+  << FixItHint::CreateInsertion(

Diagnostics are not full sentences, should this should use a lowercase m and 
drop the exclamation mark. Also, it should be "argument" rather than parameter.

I think it might be more clearly rewritten as: `"calling an inherited 
constructor other than the copy constructor"`. Also, it would be helpful to 
point to which constructor *is* being called as a note.



Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:63
+  // We want to write in the FixIt the template arguments too.
+  if (auto *Decl = dyn_cast(
+  Init->getBaseClass()->getAsCXXRecordDecl())) {

`const auto *`



Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:67
+
+const auto Args = Decl->getTemplateArgs().asArray();
+for (const auto  : Args)

Do not use `auto` here.



Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:92
+  // Loop until the beginning of the initialization list.
+  while (!Tok.is(tok::r_paren))
+Lex.LexFromRawLexer(Tok);

This doesn't balance tokens. What about:
```
struct S {
 /* ... */
};

struct T : S {
  T(const T ) : S((1 + 2), O) {}
};
```



Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:96
+
+  std::string FixItMsg = "";
+  // There is not initialization list in this constructor.

No initializer needed.



Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:108-109
+
+  diag(Tok.getLocation(), "Copy constructor should call the copy "
+  "constructor of all copyable base class!")
+  << FixItHint::CreateInsertion(Tok.getLocation(), FixItMsg);

Similar comments about the diagnostic as above. I think the two diagnostics 
should really be the same string. At the end of the day, the check is to ensure 
that a derived copy constructor calls an inherited copy constructor. Whether 
the copy constructor calls an inherited default constructor, or some other kind 
of constructor, is largely immaterial because they're both wrong in the same 
way. I'd recommend using the diagnostic text from my comment above for both.


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Dominik Szabó via Phabricator via cfe-commits]

szdominik marked 2 inline comments as done.
szdominik added a comment.

Warnings of the check's run on llvm/clang codebase.

F3426875: undelegated-copy-of-base-classes-clangrun.txt 



https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

I would be interested in seeing the results of this check's run on LLVM+Clang 
code.


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: docs/ReleaseNotes.rst:83
+
+  Finds copy constructors where the ctor don't call the constructor of the 
base class.
+

I think will be good idea to replace ctor with constructor. Or re-phrase 
sentence to avoid repetition of word constructor.


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Dominik Szabó via Phabricator via cfe-commits]

szdominik updated this revision to Diff 100892.
szdominik added a comment.

Update with fixed docs.


https://reviews.llvm.org/D33722

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp
  clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-undelegated-copy-of-base-classes.rst
  test/clang-tidy/misc-undelegated-copy-of-base-classes.cpp

Index: test/clang-tidy/misc-undelegated-copy-of-base-classes.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-undelegated-copy-of-base-classes.cpp
@@ -0,0 +1,96 @@
+// RUN: %check_clang_tidy %s misc-undelegated-copy-of-base-classes %t
+
+class Copyable {
+	public:
+	Copyable() = default;
+	Copyable(const Copyable&) = default;
+};
+class X : public Copyable {
+	X(const X& other) : Copyable(other) {}
+	//Good code: the copy ctor call the ctor of the base class.
+};
+
+class Copyable2 {
+	public:
+	Copyable2() = default;
+	Copyable2(const Copyable2&) = default;
+};
+class X2 : public Copyable2 {
+	X2(const X2& other) {};
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: : Copyable2(other)
+};
+
+class X3 : public Copyable, public Copyable2 {
+	X3(const X3& other): Copyable(other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: Copyable2(other), 
+};
+
+class X4 : public Copyable {
+	X4(const X4& other): Copyable() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: Missing parameter in the base initializer! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: other
+};
+
+class Copyable3 : public Copyable {
+	public:
+	Copyable3() = default;
+	Copyable3(const Copyable3&) = default;
+};
+class X5 : public Copyable3 {
+	X5(const X5& other) {};
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: : Copyable3(other)
+};
+
+class X6 : public Copyable2, public Copyable3 {
+	X6(const X6& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: : Copyable2(other), Copyable3(other)
+};
+
+class X7 : public Copyable, public Copyable2 {
+	X7(const X7& other): Copyable() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: Missing parameter in the base initializer! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: other
+	// CHECK-MESSAGES: :[[@LINE-3]]:22: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: Copyable2(other), 
+};
+
+template 
+class Copyable4 {
+	public:
+	Copyable4() = default;
+	Copyable4(const Copyable4&) = default;
+};
+
+class X8 : public Copyable4 {
+	X8(const X8& other): Copyable4(other) {};
+	//Good code: the copy ctor call the ctor of the base class.
+};
+
+class X9 : public Copyable4 {
+	X9(const X9& other): Copyable4() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: Missing parameter in the base initializer! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: other
+};
+
+class X10 : public Copyable4 {
+	X10(const X10& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: : Copyable4(other)
+};
+
+template 
+class Copyable5 {
+	public:
+	Copyable5() = default;
+	Copyable5(const Copyable5&) = default;
+};
+
+class X11 : public Copyable5 {
+	X11(const X11& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: : Copyable5(other)
+};
Index: docs/clang-tidy/checks/misc-undelegated-copy-of-base-classes.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-undelegated-copy-of-base-classes.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - misc-undelegated-copy-of-base-classes
+
+misc-undelegated-copy-of-base-classes
+=
+
+Finds copy constructors where the constructor don't call 
+the constructor of the base class.
+
+.. code-block:: c++
+
+class Copyable {
+public:
+Copyable() = default;
+Copyable(const Copyable&) = default;
+};
+class X2 : public Copyable {
+X2(const X2& other) {}; // Copyable(other) is missing
+};
+
+Also finds copy constructors where the 

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

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




Comment at: docs/clang-tidy/checks/misc-undelegated-copy-of-base-classes.rst:29
+
+The checker suggests a FixItHint in every scenario including multiple 
+missing initializers and constructors with template argument.

checker -> check, FixItHint -> fix-it.



Comment at: docs/clang-tidy/checks/misc-undelegated-copy-of-base-classes.rst:31
+missing initializers and constructors with template argument.
\ No newline at end of file


Please add newline.


https://reviews.llvm.org/D33722



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

[Dominik Szabó via Phabricator via cfe-commits]

szdominik created this revision.
Herald added a subscriber: mgorny.

Finds copy constructors where the constructor don't call 
the constructor of the base class.

  class X : public Copyable {
  X(const X& other) {}; // Copyable(other) is missing
  };

Also finds copy constructors where the base initializer 
don't have parameter.


https://reviews.llvm.org/D33722

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp
  clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-undelegated-copy-of-base-classes.rst
  test/clang-tidy/misc-undelegated-copy-of-base-classes.cpp

Index: test/clang-tidy/misc-undelegated-copy-of-base-classes.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-undelegated-copy-of-base-classes.cpp
@@ -0,0 +1,96 @@
+// RUN: %check_clang_tidy %s misc-undelegated-copy-of-base-classes %t
+
+class Copyable {
+	public:
+	Copyable() = default;
+	Copyable(const Copyable&) = default;
+};
+class X : public Copyable {
+	X(const X& other) : Copyable(other) {}
+	//Good code: the copy ctor call the ctor of the base class.
+};
+
+class Copyable2 {
+	public:
+	Copyable2() = default;
+	Copyable2(const Copyable2&) = default;
+};
+class X2 : public Copyable2 {
+	X2(const X2& other) {};
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: : Copyable2(other)
+};
+
+class X3 : public Copyable, public Copyable2 {
+	X3(const X3& other): Copyable(other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:23: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: Copyable2(other), 
+};
+
+class X4 : public Copyable {
+	X4(const X4& other): Copyable() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: Missing parameter in the base initializer! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: other
+};
+
+class Copyable3 : public Copyable {
+	public:
+	Copyable3() = default;
+	Copyable3(const Copyable3&) = default;
+};
+class X5 : public Copyable3 {
+	X5(const X5& other) {};
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: : Copyable3(other)
+};
+
+class X6 : public Copyable2, public Copyable3 {
+	X6(const X6& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: : Copyable2(other), Copyable3(other)
+};
+
+class X7 : public Copyable, public Copyable2 {
+	X7(const X7& other): Copyable() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:32: warning: Missing parameter in the base initializer! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: other
+	// CHECK-MESSAGES: :[[@LINE-3]]:22: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: Copyable2(other), 
+};
+
+template 
+class Copyable4 {
+	public:
+	Copyable4() = default;
+	Copyable4(const Copyable4&) = default;
+};
+
+class X8 : public Copyable4 {
+	X8(const X8& other): Copyable4(other) {};
+	//Good code: the copy ctor call the ctor of the base class.
+};
+
+class X9 : public Copyable4 {
+	X9(const X9& other): Copyable4() {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:37: warning: Missing parameter in the base initializer! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: other
+};
+
+class X10 : public Copyable4 {
+	X10(const X10& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: : Copyable4(other)
+};
+
+template 
+class Copyable5 {
+	public:
+	Copyable5() = default;
+	Copyable5(const Copyable5&) = default;
+};
+
+class X11 : public Copyable5 {
+	X11(const X11& other) {};
+	// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: Copy constructor should call the copy constructor of all copyable base class! [misc-undelegated-copy-of-base-classes]
+	// CHECK-FIXES: : Copyable5(other)
+};
\ No newline at end of file
Index: docs/clang-tidy/checks/misc-undelegated-copy-of-base-classes.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-undelegated-copy-of-base-classes.rst
@@ -0,0 +1,30 @@
+.. title:: clang-tidy - misc-undelegated-copy-of-base-classes
+
+misc-undelegated-copy-of-base-classes
+=
+
+Finds copy constructors where the constructor don't call 
+the constructor of the base class.
+
+.. code-block:: c++
+
+class Copyable {
+