Page MenuHomePhabricator

[clang-tidy] New: checker misc-unconventional-assign-operator replacing misc-assign-operator-signature
ClosedPublic

Authored by baloghadamsoftware on Mar 18 2016, 7:12 AM.

Details

Diff Detail

Event Timeline

baloghadamsoftware retitled this revision from to [clang-tidy] New: checker misc-assign-operator-return.
baloghadamsoftware updated this object.
baloghadamsoftware added reviewers: alexfh, hokein.

I think will be reasonable to merge misc-assign-operator-return and misc-assign-operator-signature into one check.

My first thought was also to extend existing checker misc-assign-operator-signature and rename it to just misc-assign-operator. However, there is little benefit doing this: the two checkers check different locations, one checks the signature while the other one of the return statements. Signature is always one, return statements can be multiple per function. So the checker function itself would consist of one branching statement and report different errors for different locations in the two branches. Only some matcher expressions can be common. Of course, if such a merge is beneficial from the user's perspective, then we should do it. Thoughts, opinions?

As user I could tell that it's much easier to have one check which will check all nuances of particular language construction then multiple checks.

Merged into misc-assign-operator-signature and thus renamed to misc-assign-operator

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added inline comments.
clang-tidy/misc/AssignOperatorCheck.cpp
74 ↗(On Diff #51554)

This can be lowered into the if statement in place of Name.

clang-tidy/misc/AssignOperatorCheck.h
19 ↗(On Diff #51554)

s/assign/assignment.

test/clang-tidy/misc-assign-operator.cpp
15 ↗(On Diff #51554)

This seems at odds with the core guidelines for cppcoreguidelines-c-copy-assignment-signature.

Thank you for your comments, but they are not related to my changes. These lines were present in the original file and I did not change them.

aaron.ballman edited edge metadata.Mar 29 2016, 5:27 AM

Thank you for your comments, but they are not related to my changes. These lines were present in the original file and I did not change them.

Ah, good to know (hard to tell context in Phab sometimes). Can you fix up the trivial changes, and leave the C++ core guideline behavior for now?

baloghadamsoftware edited edge metadata.

Requested fixes done (not related to the changes).

aaron.ballman accepted this revision.Mar 29 2016, 7:59 AM
aaron.ballman edited edge metadata.

Aside from the copy-pasta, looks good.

clang-tidy/misc/AssignOperatorCheck.cpp
74 ↗(On Diff #51906)

This doesn't look like it will compile. ;-) You can fix as part of the commit (does not require additional review).

This revision is now accepted and ready to land.Mar 29 2016, 7:59 AM
baloghadamsoftware edited edge metadata.

Faulty line deleted.

baloghadamsoftware marked an inline comment as done.Mar 29 2016, 11:54 PM
baloghadamsoftware added inline comments.
clang-tidy/misc/AssignOperatorCheck.cpp
75 ↗(On Diff #52023)

Oh, sorry, I mixed up things yesterday: started compilation, went to a meeting and then forgot that I did not check compilation results.

alexfh edited reviewers, added: sbenza; removed: hokein.Mar 30 2016, 2:55 AM
alexfh edited edge metadata.Mar 30 2016, 3:07 AM

Adding the original author in case he has something to say here.

clang-tidy/misc/AssignOperatorCheck.h
24 ↗(On Diff #52023)

Please add:

/// For the user-facing documentation see:
/// http://clang.llvm.org/extra/clang-tidy/checks/misc-assign-operator.html
clang-tidy/misc/MiscTidyModule.cpp
58

Check names usually contain hints at the class of problems they detect (e.g. "misc-inaccurate-erase" or "misc-macro-repeated-side-effects"), while misc-assign-operator doesn't serve this purpose and is overall a bit too vague (there's nothing wrong with assign operators per se).

I wonder whether a more specific name would be more helpful to the users. I'm thinking of something like misc-assign-operator-conventions. WDYT?

docs/clang-tidy/checks/misc-assign-operator.rst
13 ↗(On Diff #52023)

nit: Please add a trailing period.

baloghadamsoftware marked an inline comment as done.Mar 30 2016, 3:59 AM
baloghadamsoftware added inline comments.
clang-tidy/misc/MiscTidyModule.cpp
58

I have no better idea either. Should I rename it or let us wait for other potential reviewers proposing another name?

What about NonIdiomaticAddignOperator or UnchainableAssignOperator?

Unchainable is not enough: the original checker (which was extended) also checks for parameters and other qualifiers such as const or virtual.

What about NonIdiomaticAddignOperator or UnchainableAssignOperator?

Yep, "unchainable" doesn't cover all problems the check detects. misc-non-idiomatic-assign-operator seems good though. I'd wait for the original author to chime in before making the change.

What about NonIdiomaticAddignOperator or UnchainableAssignOperator?

Yep, "unchainable" doesn't cover all problems the check detects. misc-non-idiomatic-assign-operator seems good though. I'd wait for the original author to chime in before making the change.

This doesn't check for idiomatic assignment, unfortunately. For instance, it allows T &operator=(T) which is a copy assignment, but not generally considered an idiomatic one. (Similar for allowing volatile-qualified parameters.) If we want to go with such a check, I would not be opposed to it, but we should definitely discuss what "idiomatic" means.

What about NonIdiomaticAddignOperator or UnchainableAssignOperator?

Yep, "unchainable" doesn't cover all problems the check detects. misc-non-idiomatic-assign-operator seems good though. I'd wait for the original author to chime in before making the change.

This doesn't check for idiomatic assignment, unfortunately. For instance, it allows T &operator=(T) which is a copy assignment, but not generally considered an idiomatic one. (Similar for allowing volatile-qualified parameters.) If we want to go with such a check, I would not be opposed to it, but we should definitely discuss what "idiomatic" means.

IMHO T &operator=(T) can be idiomatic e.g. when one uses copy and swap idiom or T &operator=(S) where S is a type that can be copied efficiently.

What about NonIdiomaticAddignOperator or UnchainableAssignOperator?

Yep, "unchainable" doesn't cover all problems the check detects. misc-non-idiomatic-assign-operator seems good though. I'd wait for the original author to chime in before making the change.

This doesn't check for idiomatic assignment, unfortunately. For instance, it allows T &operator=(T) which is a copy assignment, but not generally considered an idiomatic one. (Similar for allowing volatile-qualified parameters.) If we want to go with such a check, I would not be opposed to it, but we should definitely discuss what "idiomatic" means.

Maybe you like misc-assign-operator-conventions more? ;)

Actually, there was nothing wrong with assign operator signatures per se either although the original name of the checker was AssignOperatorSignature. The only change here is that it does not check the signature only anymore, but also the body (if present).

Actually, there was nothing wrong with assign operator signatures per se either although the original name of the checker was AssignOperatorSignature. The only change here is that it does not check the signature only anymore, but also the body (if present).

Maybe the old check name should have been misc-unconventional-assign-operator-signature or something like that, but even the old name limited the scope enough to make it easy to guess about the possible issues it flags. However, further expanding the scope makes the name even less informative. There are just too many things that could be wrong with assignment operators. If/when we add a check for another bug-prone pattern related to assignment operators, the name misc-assign-operator could be broad enough to cover this hypothetical new check as well. This will inevitably lead to confusion.

misc-unconventional-assign-operator?

Please summarize this check in docs/ReleaseNotes.rst.

There is a pending review that will stub this out automatically, but it hasn't been approved yet....

Please summarize this check in docs/ReleaseNotes.rst.

OK, I will do it after the name is decided.

This comment was removed by baloghadamsoftware.
sbenza added inline comments.Apr 1 2016, 6:46 AM
clang-tidy/misc/AssignOperatorCheck.cpp
63 ↗(On Diff #52023)

I dislike these uses of hasAnscestor. They are kind of slow.
But more importantly, they break with nested functions/types.
This particular example is not checking that the return statement is from the assignment operator, only that it is within it. For example, it would match a lambda.
I think this would trip the check:

F& operator=(const F& o) {
  std::copy_if(o.begin(), o.end(), begin(), [](V v) { return v > 0; });
  return *this;
}
69 ↗(On Diff #52023)

Move this closer to where it is used.

70 ↗(On Diff #52023)

Move this into the if () statement.

clang-tidy/misc/AssignOperatorCheck.h
19 ↗(On Diff #52023)

This does not talk about the return statement, only the return type.

test/clang-tidy/misc-assign-operator.cpp
16 ↗(On Diff #52023)

This is a very common C++98 way of implementing copy-and-swap with copy elision support.
You do: T& operator=(T t) { swap(t); return *this; }
And it will avoid the copy if the argument is already a temporary due to copy elision on the caller.

69 ↗(On Diff #52023)

This case is not a bad return statement, so it should not be in this class.

aaron.ballman added inline comments.Apr 1 2016, 6:52 AM
test/clang-tidy/misc-assign-operator.cpp
16 ↗(On Diff #52023)

I wasn't arguing that it wasn't useful, but this check is also registered as cppcoreguidelines-c-copy-assignment-signature, and so we need to make sure that we aren't breaking that check. Basically, this can be resolved by looking at the spelling of the check and deciding whether to diagnose this particular case or not and add appropriate tests. (We do this for a few checks shared with CERT rules as well.)

clang-tidy/misc/AssignOperatorCheck.cpp
63 ↗(On Diff #52023)

I can change it to hasDescendant if it is faster, but it does not solve the lambda problem. No solution for that comes to my mind with the existing matchers. Maybe a new matcher hasDescendantStatement could help which only traverses statements down the AST. Is this the right way to go?

And what about the final name?

sbenza added inline comments.Apr 5 2016, 8:13 AM
clang-tidy/misc/AssignOperatorCheck.cpp
63 ↗(On Diff #52023)

hasDescendant has the same problem has hasAnscestor.
I think the best is to write a matcher like:

AST_MATCHER_P(ReturnStmt, forFunction, internal::Matcher<FunctionDecl>, InnerMatcher) {
  ...
}

In there we can find the right FunctionDecl that encloses the return statement and apply the matcher.
This matcher seems like a good candidate to add to ASTMatchers.h

clang-tidy/misc/AssignOperatorCheck.cpp
63 ↗(On Diff #52023)

Maybe I am wrong, but your proposal also seems a bottom-up matcher which is slow. That is the reason I proposed a top-down matcher, e.g. hasDescendantStatement or something like this which is top-down and only traverses statements so it does not search in a lambda which is a declaration.

sbenza added inline comments.Apr 7 2016, 10:51 AM
clang-tidy/misc/AssignOperatorCheck.cpp
63 ↗(On Diff #52023)

hasAnscestor is slow because it is way too general. There are tons of virtual function calls, cache lookups, memory allocations, etc. And the search will not stop until it find a match or runs out of anscestors. This means it will go all the way to the translationUnitDecl before it figures out that there are no matches.
Every parent will be run through the matcher.

What I propose is a simple matcher that:

  1. Finds the enclosing FunctionDecl for a statement.
  2. Runs the matcher on it, if there is an enclosing function.

(1) can be done without any virtual function call and with no/minimal memory allocations.
(2) will be done only once on the found node.

clang-tidy/misc/AssignOperatorCheck.cpp
63 ↗(On Diff #52023)

I did something like your proposal and it is working, but before uploading a patch I have a question. There are some nodes, even statement having multiple parents, probably due to template instantiations. Is it enough if we chose the first parent here (thus going up only to the template itself) or should we do a depth-first (better to say height-search here, since we are not searching downwards in a tree but upwards in a DAG) search here and go up to every parent? hasAncestor uses breadth-first search which is out of the question here.

misc-unconventional-assign-operator, misc-assign-operator-conventions, misc-non-idiomatic-assign-operator or something else then?

sbenza added inline comments.Apr 13 2016, 12:30 PM
clang-tidy/misc/AssignOperatorCheck.cpp
63 ↗(On Diff #52023)

Yes, with templates you can have multiple parents.
I think you can use any of the parents, but it would be unspecified which FunctionDecl you get.
Maybe step (1) can return a list of FunctionDecls instead.
In this particular case you might need the particular template instantiation because the return type might be a dependent type.

baloghadamsoftware retitled this revision from [clang-tidy] New: checker misc-assign-operator-return to [clang-tidy] New: checker misc-unconventional-assign-operator replacing misc-assign-operator-signature.
baloghadamsoftware edited edge metadata.

Issues fixed and checker renamed.

alexfh requested changes to this revision.Apr 26 2016, 2:50 AM
alexfh edited edge metadata.

There still seem to be a few comments from sbenza and myself that need to be resolved.

This revision now requires changes to proceed.Apr 26 2016, 2:50 AM

Wait, I probably was looking at an older diff somehow. After returning to the page I don't see any unresolved comments. Reviewing...

alexfh accepted this revision.Apr 26 2016, 3:37 AM
alexfh edited edge metadata.

No objections from me, but please wait for Samuel's review.

This revision is now accepted and ready to land.Apr 26 2016, 3:37 AM
sbenza added inline comments.Apr 26 2016, 9:08 AM
clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
69 ↗(On Diff #54486)

couldn't this be folded into the Messages array below?

clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
69 ↗(On Diff #54486)

There would be no benefit of it since we use the location of RetStmt in this message instead of Method.

sbenza accepted this revision.Apr 27 2016, 7:46 AM
sbenza edited edge metadata.
sbenza added inline comments.
clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
69 ↗(On Diff #54486)

Right. So it can't really be merged. That's fine.

I see this one is accepted, but the prerequisite is not reviewed yet (after the update). Without that this one should not be merged into the code because it will not compile.

xazax.hun closed this revision.May 4 2016, 5:08 AM