[PATCH] D128715: [clang-tidy] Fix confusable identifiers interaction with DeclContext

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D128715#3619650 <https://reviews.llvm.org/D128715#3619650>, @serge-sans-paille wrote:

> @aaron.ballman  : I agree with most of your suggestion except the one below that I annotated accordingly
>
>   struct Base {
>     virtual void O0();
>   
>   private:
>     void II();
>   };
>   
>   struct Derived : Base {
>     void OO(); // We should warn about this
>     void I1();   // I think we should warn about this too, because implementation of, say, `O0` could refer to `I1` wich would be confusable with `Il`
>   };
>   
>   void I1(); // we should probably warn here too, becaus ein the implementation of, says, `O0`, if we refer to `Il`, it's confusable with this `I1` too

I don't think we should diagnose `Derived::I1()` because `Base::II()` is private, so `I1` can never be validly confused with `II` (either from within the class context or externally).

The free function case is actually interesting but not because of confusable identifiers. If you call `I1()` within the context of `Derived`, are you trying to call the member function (you will) or the free function (you won't)? But I don't think it should be warned as a confusable identifier in terms of Unicode confusables because calling the member function outside of the class context requires an object while calling the free function does not, while calling it within the class is already confused because of name hiding.

Does that change your opinion at all?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128715