Page MenuHomePhabricator

[ASTImporter] ParmVarDecl default argument initialization
ClosedPublic

Authored by szepet on Feb 6 2017, 5:10 PM.

Details

Summary

It was left as a FIXME but I do not exactly see why.
This simple solution worked in every case I experienced (small test cases and even on greater files).
If there is a more complex problem about this initialization please let me know.

Diff Detail

Event Timeline

szepet created this revision.Feb 6 2017, 5:10 PM
a.sidorin edited edge metadata.Feb 7 2017, 8:27 AM

Hello Peter,
Thank you for working on this. Unfortunately, this patch is only a partial solution. Setting the initializer for ParmVarDecl is a bit more complicated because internally Init in ParmVarDecl is a large union; there is not simple initializer only, but different cases are handled. You can see the example of how it can be done on https://github.com/haoNoQ/clang/blob/summary-ipa-draft/lib/AST/ASTImporter.cpp#L4523. Hope this will be helpful. If you update your patch with adapting these changes, I will be happy to review it.
Also, I'd like to see some tests showing that the initializer is imported correctly. ASTImporter tests are placed into tests/ASTMerge. You can see some examples here.

szepet updated this revision to Diff 87666.Feb 8 2017, 8:34 AM

Hello Aleksei,
thank you for the review and the advice!

I have adapted the linked changes and checked if there is anything else to set. Removed the the asserts since It is possible that the importer can not import the expression, but it will give a diag message to indicate it (whenever Complain is true). So in this version of the code in case we can not import the defaultarg then it will be set to nullptr.

Added a small test case where the given CXXDefaultArgExpr is imported. Without this change it fails when creating the expression since the given ParmVarDecl's default argument is null and it will be dereferenced. So that ensures us tat the parmVarDecls defarg is set.
I'm not sure how to test the other 2 cases (uninstantiated and unparsed args).

szepet added a subscriber: dkrupp.Feb 9 2017, 5:11 AM
szepet updated this revision to Diff 87937.Feb 9 2017, 6:38 PM

additional small test case added

Looks very promising overall. I also like to see the tests here. There are some comments below.

lib/AST/ASTImporter.cpp
3896

If Expr * being imported can be null, then we can check the result in such manner:

if (FromExpr && !ToExpr)
  return nullptr;
3900

It's better to use consistent Expr * over the patch (with * sticked to the variable name).

szepet updated this revision to Diff 88180.Feb 13 2017, 5:14 AM
szepet marked 2 inline comments as done.

Use one ToExpr and FromExpr in the patch.
Check for unsuccessful import added.

a.sidorin accepted this revision.Feb 14 2017, 9:18 AM

This looks good to me. If there will be no any other review comments, I will commit. Thank you!

This revision is now accepted and ready to land.Feb 14 2017, 9:18 AM
This revision was automatically updated to reflect the committed changes.