Page MenuHomePhabricator

[clang-tidy] Add cppcoreguidelines-comparison-operator
Needs ReviewPublic

Authored by nullptr.cpp on Mar 16 2021, 3:45 AM.

Details

Summary

Finds comparison operators(==, !=, <, <=, >, >=) which are not
noexcept, are defined as member functions, have parameters of different type
or pass parameters not by value for cheap to copy types and not by reference to
const for expensive to copy types.

Examples:

struct A {
  int Var;
  bool operator==(const A &a) const; // 'operator==' should not be member function
};
struct B;
bool operator==(const B &lh, const B &rh); // 'operator==' should be marked noexcept
struct C;
bool operator==(const C &lh, int rh) noexcept; // 'operator==' should have 2 parameters of the same type
struct C { // cheap to copy
int Var;
};

bool operator==(const C &lh, const C &rh) noexcept;
// 'lh' of type 'C' is cheap to copy, should be passed by value
// 'rh' of type 'C' is cheap to copy, should be passed by value
struct D { // expensive to copy
int Array[1024];
};

bool operator==(D lh, D rh) noexcept;
// 'lh' of type 'D' is expensive to copy, should be passed by reference to const
// 'rh' of type 'D' is expensive to copy, should be passed by reference to const

Options:

  • CheckParamPassing

    Boolean flag to warn when operators pass parameters not by value for cheap to copy types and not by reference to const for expensive to copy types. Default value is true.
  • ExpensiveToCopySize

    When parameters have a size greater than ExpensiveToCopySize bytes, treat them as expensive to copy types. Default value is 0, which means to use 2 * sizeof(void*) as the threshold.

The relevant rules in the C++ Core Guidelines are:

Diff Detail

Event Timeline

nullptr.cpp created this revision.Mar 16 2021, 3:45 AM
nullptr.cpp requested review of this revision.Mar 16 2021, 3:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2021, 3:45 AM
Eugene.Zelenko added inline comments.Mar 16 2021, 7:28 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp
28

Please use const auto * - type is spelled in same statement.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-comparison-operator.rst
11

Usually such text is placed at the end of documentation. See other checks.

njames93 added inline comments.Mar 16 2021, 10:35 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp
21–22

Is there a precedent to match spaceship operator?

49–53

We should likely only warn on the canonical declaration

struct A{
  bool operator==(const A&) const; // Warn here
};

bool A::operator==(const A&) const { // Don't warn here
  return true;
}
55–56

This code should not be executed when running in pre c++11 mode, as noexcept was added in c++11.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-comparison-operator.cpp
2

Once you rebase to trunk this test will fail without --fix-notes specified.

nullptr.cpp marked 6 inline comments as done.
nullptr.cpp edited the summary of this revision. (Show Details)

Modify according to suggestions

nullptr.cpp edited the summary of this revision. (Show Details)

Update

While we're here, I'm not sure if there is a cppcoreguideline for it, but parameters to comparison functions should be passed by const ref usually, or by value if they are cheap to copy. Maybe there's precedent to extend this to flag parameters that take non const reference, R value reference or by value if not cheap to copy(There's bound to be a function somewhere in clang tidy that determines if a type is a cheap copy)

clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp
48

It might just be better to return here if the matched result isn't the same as the canonical declaration.

55–56

I was saying the other checks can run in c++98 mode but then wrap this in an if block for c++11

nullptr.cpp added a comment.EditedMar 17 2021, 5:08 AM

While we're here, I'm not sure if there is a cppcoreguideline for it, but parameters to comparison functions should be passed by const ref usually, or by value if they are cheap to copy. Maybe there's precedent to extend this to flag parameters that take non const reference, R value reference or by value if not cheap to copy(There's bound to be a function somewhere in clang tidy that determines if a type is a cheap copy)

The relevant rule is F.16: For "in" parameters, pass cheaply-copied types by value and others by reference to const.
This should be done in another checker dedicated for parameter passing.

clang-tools-extra/clang-tidy/cppcoreguidelines/ComparisonOperatorCheck.cpp
48

Why only warn on the canonical declaration?
Clients need to find the definition and other declarations by themselves. I think it's better to warn about everything found here.

The relevant rule is F.16: For "in" parameters, pass cheaply-copied types by value and others by reference to const.
This should be done in another checker dedicated for parameter passing.

That's not exactly a relevant rule, That describes handling of In parameters. However, the point I'm making is we should enforce that parameters to comparison functions are In parameters.

nullptr.cpp edited the summary of this revision. (Show Details)
  • Warn on both declarations and definitions
  • Add check for parameter passing
  • Add options CheckParamPassing and ExpensiveToCopySize