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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
