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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
