Page MenuHomePhabricator

[ASTImporter] Fix importing OperatorDelete from CXXConstructorDecl

Authored by teemperor on Jan 13 2019, 2:07 PM.



Shafik found out that importing a CXXConstructorDecl will create a translation unit that
causes Clang's CodeGen to crash. The reason for that is that we don't copy the OperatorDelete
from the CXXConstructorDecl when importing. This patch fixes it and adds a test case for that.

Diff Detail

rC Clang

Event Timeline

teemperor created this revision.Jan 13 2019, 2:07 PM

Didn't really upstream any non-trivial ASTImporter patches yet, so please point out any style errors.

Hi Rafael,
The change looks mostly fine but I have some comments inline.


It's better to move this code to VisitFunctionDecl to keep all related stuff together.


Moving this code to VisitFunctionDecl will also allow us not to create a dtor if this import fails.

Thank you, this looks good but perhaps some refactoring would help improve the change.


I am not sure I agree. Having everything in VisitFunctionDecl makes this code harder to maintain.


At what point in VisitFunctionDecl would you expect this change to bail out of? A possible refactor would be to move this code to a separate function and then call that function at that point in VisitFunctionDecl. This would prevent VisitFunctionDecl from becoming even larger and I think address your concern.

Hi Shafik,
Please find my answers inline.


That's true, but I think it's better to put FunctionDecl refactoring as a separate commit - it would be bigger than just putting some related stuff to the original function. We have the same problem with VisitRecordDecl, but, unfortunately, I still had no time to rearrange this stuff.


Just before line 3096, where a new dtor decl is being created.
If correct import of operator delete is required for successful import of dtor decl, it's better to check if it can be imported before a new dtor decl is created. So, it's better to put operator delete import before the creation of a new declaration which is located in VisitFunctionDecl(). Otherwise, we will be able to create a declaration both invalid and available by AST lookup (because DeclContexts are already set).
Please correct me if I'm wrong.

teemperor updated this revision to Diff 182718.Jan 20 2019, 3:00 PM
  • Moved code into the VisitFunctionDecl function.

I don't mind if we clean up that function in a later commit. Thanks for the feedback!

a_sidorin accepted this revision.Jan 21 2019, 2:47 PM

LGTM, thank you!
I'm sorry if the change I requested doesn't look good, but I think the correctness is our priority.

This revision is now accepted and ready to land.Jan 21 2019, 2:47 PM
martong accepted this revision.Jan 22 2019, 8:50 AM

The updated version looks good to me! Thank you!

This revision was automatically updated to reflect the committed changes.