[PATCH] D95536: [clang][sema] Note decl location on missing member

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D95536#2568252 <https://reviews.llvm.org/D95536#2568252>, @tbaeder wrote:

> In D95536#2557197 <https://reviews.llvm.org/D95536#2557197>, @aaron.ballman wrote:
>
>> Hmm... I feel like the diagnostic should already be sufficient to locate the originating location of the class or namespace and the note is adding a bit more (almost, but not quite) noise,
>
> I guess this makes sense when you're talking about https://godbolt.org/z/1Yo3Pj, but I don't understand how I would know where `OsType` is defined in https://godbolt.org/ in a larger code base (e.g. when using `OsType` in clang, with it being defined in llvm).

I think that the name in the diagnostic should be sufficient for you to track that down using the editor of your choice (including super simple editors like notepad), such as by using a simple search. With that example, when I search for "enum OsType" in my editor, there's only one hit in the code base. Even in slightly more complex cases where you try to make things look ambiguous, the diagnostic text does a pretty good job of making it clear roughly where to look: https://godbolt.org/z/41nPnd

>> Anonymous namespaces:
>>
>>   namespace foo {
>>   namespace {
>>     void func();
>>   }
>>   }
>>   
>>   void bar() {
>>     foo::blarg(); // Should point to 'foo'?
>>   }
>
> Seems to work:
>
>   ./enum.cpp:56:8: error: no member named 'blarg' in namespace 'foo'
>     foo::blarg(); // Should point to 'foo'?
>     ~~~~~^
>   ./enum.cpp:49:11: note: namespace 'foo' declared here
>   namespace foo {
>             ^~~
>
> You list a few more interesting corner cases however. I'm not sure if I want to pursue this patch further as it is already quite ugly because it's touching all those tests. Or if it would be better to implement a note that lists all enum members (up to a certain threshold?), but just for enums.

I'd be worried that would add even more output that would largely be noise in the common case. My personal preference in this case would be to try to improve the existing diagnostics to be more clear as to what's referenced rather than adding an extra note. However, I won't block the patch based solely on my personal preference if you'd like to continue to pursue this -- I leave the final decision to @rsmith in that case.


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

https://reviews.llvm.org/D95536