[PATCH] D61749: [clang-tidy] initial version of readability-static-const-method

[Matthias Gehre via Phabricator via cfe-commits]

mgehre added a comment.

I have thought about splitting the check into separate `static` & `const`, but kept it together because
the analysis for both cases is very similar, i.e. finding the usage of `this`.

Also, both checks share many preconditions, e.g. not applying them to virtual methods or methods
on class with a template base class, not applying them to constructor, not applying them to operators, etc
(and those preconditons are not shared with D54943 <https://reviews.llvm.org/D54943>).

I had a look at D54943 <https://reviews.llvm.org/D54943>. Nice change! It will be very valuable to have the `const` analysis for variables!
Note, though, that the hard part of of making methods
`const` is not the detailed usage analysis as its done by the `ExprMutationAnalyzer`. Instead, the hard part
is to avoid false-positives that make the check unusable.
I had a version of this check that made a very detailed analysis just like `ExprMutationAnalyzer`
(restricted to `this`). But then most of the functions it flagged as `const`
(technically correct) on the LLVM code base we would not want to make const (logically). Examples:

  class S {
    // Technically const (the pointer Pimp itself is not modified), but logically should not be const
    //  because we want to consider *Pimp as part of the data.
    void setFlag() {
      *Pimp = 4;
    }
  
    int* Pimp;
  };



  class S {
    void modify() {
       // Technically const because we call a const method
      // "pointer unique_ptr::operator->() const;"
      // but logically not const.
      U->modify();
    }
  
    std::unique_ptr<Object> U;
  };



  // Real-world code, see clang::ObjCInterfaceDecl.
  class DataPattern {
    int& data() const;
  public:
  
    int& get() { // Must not be const, even though it only calls const methods
      return data();
    }
  
    const int& get() const {
      return const_cast<DataPattern *>(this)->get();
    }
  
    void set() { // Must not be const, even though it only calls const methods
      data() = 42;
    }
  };

and many more.

So the hard thing for the `const` check was to actually remove the sophisticated analysis I had, and come up with
something much simpler that would have no false positive (otherwise users will just disable the check) while
still finding something. I fear that the same could happen when we apply D54943 <https://reviews.llvm.org/D54943> to `this` directly. It might
be easier to keep those two cases separate.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61749/new/

https://reviews.llvm.org/D61749