This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix false positives involving type aliases in `misc-unconventional-assign-operator` check
ClosedPublic

Authored by fwolff on Nov 18 2021, 2:13 PM.

Details

Summary

clang-tidy currently reports false positives even for simple cases such as:

struct S {
    using X = S;
    X &operator=(const X&) { return *this; }
};

This is due to the fact that the misc-unconventional-assign-operator check fails to look at the canonical types. This patch fixes this behavior.

Diff Detail

Event Timeline

fwolff created this revision.Nov 18 2021, 2:13 PM
fwolff requested review of this revision.Nov 18 2021, 2:13 PM
This revision is now accepted and ready to land.Nov 18 2021, 2:51 PM

Thanks for the quick review! Can you also commit it for me? You can use name and email as in commit 738e7f1231949ec248c1d8d154783338215613d1. Thank you!

Thanks for the quick review! Can you also commit it for me? You can use name and email as in commit 738e7f1231949ec248c1d8d154783338215613d1. Thank you!

Sure np, I will give some time for other reviewers to chime in though :)

But FYI in case you are interested in contributing more, you can get your own license to kill here: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

whisperity added inline comments.Nov 26 2021, 5:03 AM
clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator.cpp
151

This is a no-warn due to the parameter being a completely unrelated type, right? Might worth a comment. I don't see at first glance why a warning should not happen here.

152

What about Alias3<TypeAlias::Alias>& operator =(const Alias1&) { return *this; }? That should trigger a warning as it is an unrelated type, right?

fwolff updated this revision to Diff 391850.Dec 4 2021, 11:39 AM
fwolff marked 2 inline comments as done.Dec 4 2021, 11:45 AM

Thanks for your comments @whisperity. I think I've addressed them, could you have another look?

clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator.cpp
151

Exactly. I've added a comment in the TypeAlias struct above, because that one comes first. I've also updated the documentation for this check to make this clearer.

152

Yes, I've added a test for this (but with double as the argument type, because const Alias1& gives a warning:

warning: 'const' qualifier on reference type 'TemplateTypeAlias::Alias1' (aka 'TemplateTypeAlias<T> &') has no effect [-Wignored-qualifiers]
fwolff marked 2 inline comments as done.Dec 4 2021, 11:45 AM
Quuxplusone added inline comments.
clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator.cpp
151

This is a no-warn due to the parameter being a completely unrelated type, right?

No. If that were the case, this would be a terrible test case, full of irrelevant cruft. :)

Once you look past all the levels of type aliases, this is equivalent to

template<class T>
struct TTA {
    TTA<T>& operator=(int) { return *this; }
};

and so no warning is given because it's correct code.

aaron.ballman accepted this revision.Dec 8 2021, 8:36 AM

LGTM aside from some small nits.

clang-tools-extra/docs/clang-tidy/checks/misc-unconventional-assign-operator.rst
12 ↗(On Diff #391850)
clang-tools-extra/test/clang-tidy/checkers/misc-unconventional-assign-operator.cpp
131

Can you also add tests which use typedef instead of using? (They should just work as-is)

fwolff marked 3 inline comments as done.Jan 17 2022, 12:15 PM