[ASTImporter] ParmVarDecl default argument initialization

[szepet]

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.

[a.sidorin]

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]

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]

additional small test case added

[a.sidorin]

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

lib/AST/ASTImporter.cpp

3760

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

if (FromExpr && !ToExpr)
  return nullptr;

3764

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

[szepet]

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

[a.sidorin]

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

lib/AST/ASTImporter.cpp

Decl *ASTNodeImporter::VisitParmVarDecl(ParmVarDecl *D) {
   // Import the location of this declaration.
   SourceLocation Loc = Importer.Import(D->getLocation());
   
   // Import the parameter's type.
   QualType T = Importer.Import(D->getType());
   if (T.isNull())
     return nullptr;
 
+  // Import the default argument.
+  Expr* DefaultArg = Importer.Import(D->getDefaultArg());
+
   // Create the imported parameter.
   TypeSourceInfo *TInfo = Importer.Import(D->getTypeSourceInfo());
   ParmVarDecl *ToParm = ParmVarDecl::Create(Importer.getToContext(), DC,
                                      Importer.Import(D->getInnerLocStart()),
                                             Loc, Name.getAsIdentifierInfo(),
                                             T, TInfo, D->getStorageClass(),
-                                            /*FIXME: Default argument*/nullptr);
+                                            DefaultArg);
   ToParm->setHasInheritedDefaultArg(D->hasInheritedDefaultArg());
 
   if (D->isUsed())
     ToParm->setIsUsed();
 
   return Importer.Imported(D, ToParm);
 }
 
 Decl *ASTNodeImporter::VisitObjCMethodDecl(ObjCMethodDecl *D) {
   // Import the major distinguishing characteristics of a method.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
   SourceLocation Loc;
   NamedDecl *ToD;
   if (ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
     return nullptr;
   if (ToD)
     return ToD;