a_sidorin added a comment.
Herald added a subscriber: Szelethus.
Hi Balasz,
It's going to take some time to review the whole patch.
================
Comment at: lib/AST/ASTImporter.cpp:194
+ // FIXME: This should be the final code.
+ //auto ToOrErr = Importer.Import(From);
+ //if (ToOrErr) {
----------------
Do I understand correctly that we have to use the upper variant because
ASTImporter API is unchanged?
================
Comment at: lib/AST/ASTImporter.cpp:417
- bool ImportDefinition(RecordDecl *From, RecordDecl *To,
- ImportDefinitionKind Kind = IDK_Default);
- bool ImportDefinition(VarDecl *From, VarDecl *To,
- ImportDefinitionKind Kind = IDK_Default);
- bool ImportDefinition(EnumDecl *From, EnumDecl *To,
- ImportDefinitionKind Kind = IDK_Default);
- bool ImportDefinition(ObjCInterfaceDecl *From, ObjCInterfaceDecl *To,
- ImportDefinitionKind Kind = IDK_Default);
- bool ImportDefinition(ObjCProtocolDecl *From, ObjCProtocolDecl *To,
- ImportDefinitionKind Kind = IDK_Default);
- TemplateParameterList *ImportTemplateParameterList(
+ Error ImportDefinition(
+ RecordDecl *From, RecordDecl *To,
----------------
balazske wrote:
> a_sidorin wrote:
> > The changes for argument indentation look redundant. Is it a result of
> > clang-format?
> clang-format aligns start of new argument lines to the `(` character above.
> Indentations are not always consistent in this file, I can reformat the whole
> file with clang-format.
> (But I do not like the formatting like
> ```
> Error ImportDeclParts(NamedDecl *D, DeclContext *&DC, DeclContext
> *&LexicalDC,
> DeclarationName &Name, NamedDecl *&ToD,
> SourceLocation &Loc);
> ```
> if there is a `LongNamedClass::LongReturnType
> LongNamedClass::LongNamedFunction`.)
>
No need to reformat the whole file. As I see, the indentation changes are not
breaking and too wide to roll them back so we can leave them as-is.
================
Comment at: lib/AST/ASTImporter.cpp:1087
- QualType ClassType = Importer.Import(QualType(T->getClass(), 0));
- return Importer.getToContext().getMemberPointerType(ToPointeeType,
- ClassType.getTypePtr());
+ ExpectedType ClassTypeOrErr = import(QualType(T->getClass(), 0));
+ if (!ClassTypeOrErr)
----------------
Nice catch!
================
Comment at: lib/AST/ASTImporter.cpp:1658
}
+ llvm::SmallVector<Decl*, 8> ImportedDecls;
+ for (auto *From : FromDC->decls()) {
----------------
Space before '*' is needed.
================
Comment at: lib/AST/ASTImporter.cpp:1663
+ // Ignore the error, continue with next Decl.
+ consumeError(ImportedOrErr.takeError());
+ }
----------------
Silent fail doesn't look correct in case of structures. Should we add a FIXME?
================
Comment at: lib/AST/ASTImporter.cpp:1953
- return false;
+Expected<TemplateArgument>
+ASTNodeImporter::ImportTemplateArgument(const TemplateArgument &From) {
----------------
Should we add a FIXME for removing this method?
================
Comment at: lib/AST/ASTImporter.cpp:2266
+ ToD, D, Importer.getToContext(), DC, ToNamespaceLoc, ToAliasLoc,
+ ToIdentifier,ToQualifierLoc, ToTargetNameLoc, ToNamespace))
return ToD;
----------------
Space after comma is lost.
================
Comment at: lib/AST/ASTImporter.cpp:2656
}
+ if (IsFriendTemplate)
+ continue;
----------------
This move looks like an additional functional change. Could you please explain
the reason?
================
Comment at: lib/AST/ASTImporter.cpp:2683
+ continue;
+ } else if (isa<ValueDecl>(Found))
+ continue;
----------------
Same here.
================
Comment at: lib/AST/ASTImporter.cpp:2706
CXXRecordDecl *D2CXX = nullptr;
- if (auto *DCXX = dyn_cast<CXXRecordDecl>(D)) {
+ if (auto *DCXX = llvm::dyn_cast<CXXRecordDecl>(D)) {
if (DCXX->isLambda()) {
----------------
We usually don't qualify casts.
================
Comment at: lib/AST/ASTImporter.cpp:4136
return ToProto;
}
----------------
// Stopped here.
Repository:
rC Clang
https://reviews.llvm.org/D51633
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
