Page MenuHomePhabricator

[clang-tidy] New checker bugprone-incomplete-comparison-operator
AbandonedPublic

Authored by kallehuttunen on Mar 7 2019, 10:46 AM.

Details

Summary

The checker detects comparison operators that don't access all fields of the compared types.

Diff Detail

Event Timeline

kallehuttunen created this revision.Mar 7 2019, 10:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2019, 10:46 AM

Thank you for working on this!

This looks questionable to me.
Is this based on some coding standard?
Are there any numbers on the false-positive rate of theck?

The checker gives quite many warnings on LLVM code base. For example, running it for lib/Transforms/Scalar:

/home/kalle/llvm/lib/Transforms/Scalar/GVN.cpp:119:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator==(const Expression &other) const {
  ^
/home/kalle/llvm/lib/Transforms/Scalar/GVN.cpp:119:3: note: commutative not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVN.cpp:119:3: note: other.commutative not accessed

/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator==(const CHIArg &A) { return VN == A.VN; }
  ^
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: note: Dest not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: note: I not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: note: A.Dest not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: note: A.I not accessed

/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator!=(const CHIArg &A) { return !(*this == A); }
  ^
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: note: Dest not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: note: I not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: note: A.Dest not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: note: A.I not accessed

/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator>(const SinkingInstructionCandidate &Other) const {
  ^
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: NumBlocks not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: NumInstructions not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: NumPHIs not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: NumMemoryInsts not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Blocks not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Other.NumBlocks not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Other.NumInstructions not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Other.NumPHIs not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Other.NumMemoryInsts not accessed
/home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Other.Blocks not accessed

/home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator<(const BCEAtom &O) const {
  ^
/home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: note: GEP not accessed
/home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: note: LoadI not accessed
/home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: note: O.GEP not accessed
/home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: note: O.LoadI not accessed

/home/kalle/llvm/lib/Transforms/Scalar/NewGVN.cpp:436:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator==(const Expression &Other) const {
  ^
/home/kalle/llvm/lib/Transforms/Scalar/NewGVN.cpp:436:3: note: Other.Opcode not accessed
/home/kalle/llvm/lib/Transforms/Scalar/NewGVN.cpp:436:3: note: Other.HashVal not accessed

/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:206:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  friend LLVM_ATTRIBUTE_UNUSED bool operator<(const Slice &LHS,
  ^
/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:206:3: note: LHS.EndOffset not accessed
/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:206:3: note: LHS.UseAndIsSplittable not accessed

/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:210:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  friend LLVM_ATTRIBUTE_UNUSED bool operator<(uint64_t LHSOffset,
  ^
/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:210:3: note: RHS.EndOffset not accessed
/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:210:3: note: RHS.UseAndIsSplittable not accessed

/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:582:3: warning: incomplete comparison operator [bugprone-incomplete-comparison-operator]
  bool operator==(const partition_iterator &RHS) const {
  ^
/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:582:3: note: MaxSplitSliceEndOffset not accessed
/home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:582:3: note: RHS.MaxSplitSliceEndOffset not accessed

Running the checker on 100k SLOC proprietary code base I'm working with resulted in finding 5 bugs and 1 false positive (the code had a comment that one member was not accessed on purpose).

I'm anticipating that the checker can produce lots of false positives in some code bases because of operator<. That's why I'm making the checked operators configurable from the start.

Thank you for working on this!

This looks questionable to me.
Is this based on some coding standard?
Are there any numbers on the false-positive rate of theck?

It's not based on any coding standard, but is rather an attempt to eliminate one class of bugs (forgetting to update comparison operator when adding struct members) that I've encountered several times.

lebedev.ri added inline comments.Mar 7 2019, 11:08 AM
clang-tidy/bugprone/IncompleteComparisonOperatorCheck.cpp
242–243

These should point to the actual field decl location in the class

MyDeveloperDay added a subscriber: MyDeveloperDay.EditedMar 7 2019, 11:20 AM

I definately been burnt by not handling all the cases, but imagine a string class

class mystring
{
     const char *ptr;
     int size;
     int numallocated;
}

to compare the strings I wouldn't want to compare all the fields?

I think this could lead to a lot of false positives

@lebedev.ri @kallehuttunen what do you think of putting this into context of the proposal (i believe its being standardized with c++20) of operator==() = default and the spaceship-operator.
This check could serve as a basis to modernize the operator== to the default if appropriate and warn/diagnose in the other cases.
WDYT?

Eugene.Zelenko removed a project: Restricted Project.Mar 7 2019, 12:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2019, 12:35 PM
Eugene.Zelenko added inline comments.
clang-tidy/bugprone/IncompleteComparisonOperatorCheck.cpp
197

Please don't use auto where return type is not obvious.

I'm definitely worried about the false positive rate for this check -- from a quick look at the diagnostics from LLVM, every one of them is a false positive (though perhaps I've missed something; it was a very quick look). Requiring users to explicitly use a declaration within the operator seems like it could get onerous for complex classes that support the notion of equality from a few members as opposed to all members.

Also, can you add some tests for classes that inherit members, perhaps even private ones? e.g.,

struct Base {
  int Member;
};

struct Derived : Base {
  int OtherMember;

  bool operator==(const Derived &RHS) const { return OtherMember == RHS.OtherMember; }
};

struct Derived2 : private Base {
  int OtherMember;

  bool operator==(const Derived2 &RHS) const { return OtherMember == RHS.OtherMember; }
};

Another good test is for scenarios where you cannot compare all of the members, such as:

struct Interesting {
  bool operator==(const Interesting &Other) const = delete;
};

struct S {
  int Member;
  Interesting OtherMember;

  bool operator==(const S &Other) const { return Member == Other.Member; }
};

I found this checker to be useful in the code base I initially developed it for, but the usage of comparison operators there is pretty much limited to comparing simple aggregate types. It's true that this checker can produce lots of false positives, maybe too much to be included to clang-tidy proper.

@JonasToth 's proposal is interesting. Overall I think the "modernize-use-defaulted-spaceship-op" check would be much more complex than this one, but I guess the FieldAccessVisitor could be used as a building block.

Another idea that came to my mind would be to enable this check only for annotated types. So warning for missing field access would be only given for types that have for example [[clang::annotate("value type")]] annotation. Possibly other kinds of checks could be also developed for types that have the given annotation.

Another idea that came to my mind would be to enable this check only for annotated types. So warning for missing field access would be only given for types that have for example [[clang::annotate("value type")]] annotation. Possibly other kinds of checks could be also developed for types that have the given annotation.

I don't think annotate would be a good choice because the primary purpose of that one is to pass that attribute information down to the backend and using it for this purpose feels a bit hackish. However, we could always add a new attribute if needed, but I'm not convinced an attribute is the right approach either (but then again, I'm also lacking information I'm sure).

What other kinds of checks do you have in mind and what are the semantics of the attribute you're thinking of?

Another idea that came to my mind would be to enable this check only for annotated types. So warning for missing field access would be only given for types that have for example [[clang::annotate("value type")]] annotation. Possibly other kinds of checks could be also developed for types that have the given annotation.

I don't think annotate would be a good choice because the primary purpose of that one is to pass that attribute information down to the backend and using it for this purpose feels a bit hackish. However, we could always add a new attribute if needed, but I'm not convinced an attribute is the right approach either (but then again, I'm also lacking information I'm sure).

What other kinds of checks do you have in mind and what are the semantics of the attribute you're thinking of?

I was thinking about something along the lines of having an attribute to mark types to be a "value type" as defined in Herb's metaclasses proposal (chapter 3.5 in https://herbsutter.files.wordpress.com/2017/07/p0707r1.pdf). There could be then checks for having default ctor, no virtual functions etc in addition to this check for those types. But then again now that I think about it, a "string" class is an counterexample for this: it can be a value (regular) type, but when comparing instances you don't want to access the "capacity" member.

kallehuttunen abandoned this revision.Mar 28 2019, 8:10 AM