Page MenuHomePhabricator

[clang-tidy] Add checker for undelegated copy of base classes
ClosedPublic

Authored by xazax.hun on May 31 2017, 6:28 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
docs/clang-tidy/checks/misc-undelegated-copy-of-base-classes.rst
31 ↗(On Diff #100858)

Please add newline.

Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Update with fixed docs.

docs/ReleaseNotes.rst
83 ↗(On Diff #100892)

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

alexfh edited edge metadata.Jun 8 2017, 1:00 AM

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

szdominik marked 2 inline comments as done.Jun 10 2017, 12:45 AM

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

aaron.ballman requested changes to this revision.Jun 14 2017, 4:25 PM
aaron.ballman added inline comments.
clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp
41 ↗(On Diff #100892)

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

48 ↗(On Diff #100892)

No need for the initializer.

53 ↗(On Diff #100892)

Spurious parens.

55 ↗(On Diff #100892)

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.

63 ↗(On Diff #100892)

const auto *

67 ↗(On Diff #100892)

Do not use auto here.

92 ↗(On Diff #100892)

This doesn't balance tokens. What about:

struct S {
 /* ... */
};

struct T : S {
  T(const T &O) : S((1 + 2), O) {}
};
96 ↗(On Diff #100892)

No initializer needed.

108–109 ↗(On Diff #100892)

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.

This revision now requires changes to proceed.Jun 14 2017, 4:25 PM

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

szdominik updated this revision to Diff 102807.Jun 16 2017, 5:16 AM
szdominik edited edge metadata.
szdominik marked 9 inline comments as done.

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

szdominik added inline comments.Jun 16 2017, 5:19 AM
clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp
92 ↗(On Diff #100892)

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.

aaron.ballman added inline comments.Jun 16 2017, 5:26 AM
clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp
92 ↗(On Diff #100892)

Let me try again. :-)

struct S {
 /* ... */
};

struct T : S {
  T(const T &O, int = (1 + 2)) : S((1 + 2), O) {}
};
szdominik added inline comments.Jun 16 2017, 5:32 AM
clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp
92 ↗(On Diff #100892)

Ah, you're right. I'll figure out something... :)

szdominik updated this revision to Diff 103385.Jun 21 2017, 8:06 AM
szdominik marked 4 inline comments as done.

Updated loop for searching the beginning of the initlist.

xazax.hun added inline comments.Jul 31 2017, 6:52 AM
clang-tidy/misc/CopyConstructorInitCheck.cpp
22 ↗(On Diff #103385)

Wouldn't something like:

static auto ctorInit = cxxCtorInitializer(....);

work?

I would prefer that rather than a new AST_MATCHER.

And later you could use ctorInit without parens.

test/clang-tidy/misc-copy-constructor-init.cpp
21 ↗(On Diff #103385)

These check-fixes lines should contain the whole line after the fixit is applied.

szdominik updated this revision to Diff 109564.Aug 3 2017, 8:14 AM
szdominik marked 2 inline comments as done.

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

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

aaron.ballman added inline comments.Sep 20 2017, 8:36 AM
clang-tidy/misc/CopyConstructorInitCheck.cpp
24 ↗(On Diff #109564)

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

37 ↗(On Diff #109564)

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

59 ↗(On Diff #109564)

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

80 ↗(On Diff #109564)

Please do not use auto here.

95 ↗(On Diff #109564)

Space before the colon?

104 ↗(On Diff #109564)

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().

test/clang-tidy/misc-copy-constructor-init.cpp
27 ↗(On Diff #109564)

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

klimek added a subscriber: klimek.Sep 22 2017, 6:06 AM

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).

szdominik updated this revision to Diff 118617.Oct 11 2017, 7:43 AM
szdominik marked 4 inline comments as done.

Small changes after aaron.ballman's comments.

clang-tidy/misc/CopyConstructorInitCheck.cpp
37 ↗(On Diff #109564)

I wanted to create only one FixIt to every ctor - if I move the forEachCtorInitializer to the register part, it would warn us twice.

104 ↗(On Diff #109564)

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.

test/clang-tidy/misc-copy-constructor-init.cpp
27 ↗(On Diff #109564)

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.

aaron.ballman added inline comments.Oct 15 2017, 7:11 AM
clang-tidy/misc/CopyConstructorInitCheck.cpp
104 ↗(On Diff #109564)

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)

test/clang-tidy/misc-copy-constructor-init.cpp
9 ↗(On Diff #118617)

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

19 ↗(On Diff #118617)

Spurious semicolon.

25 ↗(On Diff #118617)

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

27 ↗(On Diff #109564)

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

xazax.hun commandeered this revision.Nov 3 2017, 7:44 AM
xazax.hun added a reviewer: szdominik.
xazax.hun updated this revision to Diff 121477.Nov 3 2017, 7:49 AM
  • 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
xazax.hun marked 5 inline comments as done.Nov 3 2017, 7:54 AM

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.

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.

xazax.hun updated this revision to Diff 121698.Nov 6 2017, 1:41 AM
  • Do not warn for NonCopyable bases
  • Remove lexing
xazax.hun added a comment.EditedNov 6 2017, 1:45 AM

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. 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?

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

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.

clang-tidy/misc/CopyConstructorInitCheck.cpp
38–40 ↗(On Diff #121698)

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).

69 ↗(On Diff #121698)

What if the base class is inherited privately? e.g.,

struct Base {
  Base(const Base&) {}
};

struct Derived : private Base {
  Derived(const Derived &) {}
};
109 ↗(On Diff #121698)

ctros -> ctors

docs/clang-tidy/checks/misc-copy-constructor-init.rst
4 ↗(On Diff #121698)

The markdown here is wrong.

6 ↗(On Diff #121698)

don't -> doesn't

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).

xazax.hun updated this revision to Diff 121885.Nov 7 2017, 6:26 AM
xazax.hun marked 3 inline comments as done.
  • Fix review comments
xazax.hun updated this revision to Diff 121886.Nov 7 2017, 6:29 AM
xazax.hun marked 2 inline comments as done.
  • Fix doc comments that I overlooked earlier

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?

clang-tidy/misc/CopyConstructorInitCheck.cpp
69 ↗(On Diff #121698)

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.

aaron.ballman added inline comments.Nov 9 2017, 7:45 PM
clang-tidy/misc/CopyConstructorInitCheck.cpp
69 ↗(On Diff #121698)

Good, thank you for adding the test (and I agree, we want to warn in that case).

xazax.hun marked 8 inline comments as done.Nov 16 2017, 1:18 AM
aaron.ballman accepted this revision.Nov 16 2017, 5:57 AM

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.

clang-tidy/misc/CopyConstructorInitCheck.cpp
61 ↗(On Diff #121886)

Please don't use auto here.

docs/clang-tidy/checks/misc-copy-constructor-init.rst
29 ↗(On Diff #121886)

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

This revision is now accepted and ready to land.Nov 16 2017, 5:57 AM
This revision was automatically updated to reflect the committed changes.