[PATCH] D94067: [clang][ASTImporter] Fix a possible assertion failure `NeedsInjectedClassNameType(Decl)'.

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked an inline comment as done.
balazske added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:2901
+          // Skip the declaration if injected type is already set.
+          if (isa<InjectedClassNameType>(RI->getTypeForDecl()))
+            continue;
----------------
shafik wrote:
> balazske wrote:
> > shafik wrote:
> > > Is this to fix the bug or is this for efficiency sake?
> > This is not needed for the fix, it was used in the first version of the fix (still only for efficiency). In the current form this looks like unrelated change (the old fix included other code at the same location) so I am not against removing this part (but add it in a separate change).
> Yes, please if we can split the two changes that would be great.
I checked if this "skip" is needed, but it turned out (at least on this test file) that in all cases  (1060 total) the `InjectedClassNameType` is already there, so this loop would do nothing useful if the skip is added. And the same for the code in `if (Injected)` branch, the type is already `InjectedClassNameType`. I do not plan to make a new change for this issue, but it needs investigation to check if this change of the type is needed at all (if not, and why not, if yes, is the "skip" useful). Probably a FIXME could be added here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94067