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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to