[PATCH] D32751: [ASTImporter] Support new kinds of declarations (mostly Using*)

[Peter Szecsi via Phabricator via cfe-commits]

szepet added a comment.

Some nits added, other than these it looks good to me.  Thank you!
Just more one question, I can see 3 different cases how the import returns are handled:

- if(!ToDecl) return nullptr;
- if(!ToDecl && FromDecl) return nullptr;
- no handling: ObjectXY::Create(...Import(FromDecl))

My question is the following: which cases require a check and which decls can be imported without checking the return value of the **import **function?
(Yepp, it could be asked in more general about the Importer, since things like this would be great to follow a convention. I have found some cases where it was not obivous to me which way to check. )



================
Comment at: lib/AST/ASTImporter.cpp:2993
+      return nullptr;
+  }
+
----------------
nit: As I see these cases typically handled in the way:

```
FrPattern = .;
ToPattern = ..;
if(FrPattern && !ToPattern)
```
Just to avoid the nested ifstmt.


================
Comment at: lib/AST/ASTImporter.cpp:3000
+    else
+      // FIXME: We return a nullptr here but the definition is already created
+      // and available with lookups. How to fix this?..
----------------
I dont see the problem with moving these up , collect nad investigate them in a smallset before the Create function, then adding them to the created ToUsing decl. It could be done as a follow up patch, dont want to mark it as a blocking issue.


================
Comment at: lib/AST/ASTImporter.cpp:3042
+      return nullptr;
+  }
+
----------------
the same nit as above


================
Comment at: lib/AST/ASTImporter.cpp:3043
+  }
+
+  LexicalDC->addDeclInternal(ToShadow);
----------------
Does not this causes the same FIXME problem as above?


https://reviews.llvm.org/D32751