[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