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