This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] misc-assign-operator-signature checker checks return value of all assign operators
ClosedPublic

Authored by baloghadamsoftware on Mar 18 2016, 5:04 AM.

Diff Detail

Event Timeline

baloghadamsoftware retitled this revision from to [clang-tidy] misc-assign-operator-signature checker checks return value of all assign operators.
baloghadamsoftware updated this object.
baloghadamsoftware added reviewers: alexfh, hokein.
hokein accepted this revision.Mar 21 2016, 6:05 AM
hokein edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Mar 21 2016, 6:05 AM

I don't think this is a safe assumption to make -- for instance, DSLs tend to have operator overload return types that aren't necessarily the same as the class type. Also, it's quite common for older code (pre C++11) to return void and make these functions private (with no definition) as an early form of deleting the operator.

How many additional diagnostics does this modification produce over some large code bases (particularly ones with DSLs if you can narrow the test field a bit)?

Also, it's quite common for older code (pre C++11) to return void and make these functions private (with no definition) as an early form of deleting the operator.

This checker will not warn on private methods.

About EDSLs, I think they are not the common case. In case it is important, it might make sense to make this configurable, but I think the minority of the C++ projects have EDSLs so it make more sense to warn on those cases by default. What do you think?

Also, it's quite common for older code (pre C++11) to return void and make these functions private (with no definition) as an early form of deleting the operator.

This checker will not warn on private methods.

Good deal.

About EDSLs, I think they are not the common case. In case it is important, it might make sense to make this configurable, but I think the minority of the C++ projects have EDSLs so it make more sense to warn on those cases by default. What do you think?

That's why I am wondering about how much this increases the false positive rate. I don't think DSLs are going to be a huge problem, but if it turns out they are, then a configuration option makes sense. If we don't see a large increase, then no need to waste the effort on an option.

Also, this appears to be changing the behavior of the cppcoreguidelines-c-copy-assignment-signature alias. If the core guidelines don't prohibit this behavior under that rule, we should not trigger the diagnostic if the user enabled the check via cppcoreguidelines-c-copy-assignment-signature instead of misc-assign-operator-signature. Check out MoveConstructorInitCheck as an example of how we have handled this before.

etienneb added inline comments.
clang-tidy/misc/AssignOperatorSignatureCheck.cpp
48

nit: could you +2 the line to be consistent with the rest of the code

baloghadamsoftware edited edge metadata.

Check for non copy and move assign operators made optional.

According to cpp core guidelines any assignment operator should return *this.
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-assignment-op

So in case this check is activated by using an alias it should not be a special case.

Oh, I was searching in the C++ Core Guidlines, but at the wrong place because I did not find it. So I will change this option to be enabled by default. DSL users who do not follow this rule for the non copy and non move assign operators can disable it.

alexfh edited edge metadata.Mar 23 2016, 3:57 PM

Oh, I was searching in the C++ Core Guidlines, but at the wrong place because I did not find it. So I will change this option to be enabled by default. DSL users who do not follow this rule for the non copy and non move assign operators can disable it.

I'd go without the option until we find a use case for it in real code.

clang-tidy/misc/AssignOperatorSignatureCheck.cpp
48

Just do whatever clang-format -style=llvm does.

baloghadamsoftware edited edge metadata.

Reverted to the original (accepted) version.

Code reformatted using: clang-format -style="LLVM"

alexfh accepted this revision.Mar 24 2016, 2:42 AM
alexfh edited edge metadata.

LG. Tell me if you need someone to commit the patch for you.

This revision was automatically updated to reflect the committed changes.