[PATCH] D59665: Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

Fri Mar 22 03:08:43 PDT 2019

martong added a comment.

In D59665#1438911 <https://reviews.llvm.org/D59665#1438911>, @a_sidorin wrote:

> Hi Shafik,
>
> Honestly, I was always wondering what does HandleNameConflict actually do. Its implementation in ASTImporter is trivial and I don't see any of its overrides in LLDB code too. Why do we check its result to be non-empty is a question to me as well. And the more I look at it (and the bug you pointed in it), the more I think there is something wrong with it. Maybe it is better to just remove it at all? I hope LLDB developers have more knowledge about it. Shafik, Davide @davide , do you know its actual purpose?

As to my best knowlege, HandleNameConflict is not overridden in LLDB (@davide , @shafik please correct my if I am wrong).
However, I would not remove it, I'd rather fix the problems we have around it.
I think via HandleNameConflict we could implement different strategies about how to handle ODR errors and perhaps that was the original intention to use it for. With CTU (and probably with LLDB too) this could be really helpful: Recently I have discovered that during the CTU analysis of Xerces there are real ODR errors on a typedef, but it would be nice if we could just keep on going with the inlining of a certain function. So, I see three possible different strategies to handle name conflicts:

1. Conservative. In case of name conflict propagate the error. (The function will not be inlined because of a typedef ODR error.)
2. Liberal. In case of name conflict create a new node with the same name and do not propagate the error. (This may give erroneous results if a lookup is performed, but most SA checkers does not do lookup)
3. Rename. In case of name conflict create a new node with a different name (e.g. with a prefix of the TU's name). Do not propagate the error. (This would result the most sane AST, but this is perhaps the most difficult to implement, plus a mapping between old and new names has to be given.)

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

https://reviews.llvm.org/D59665