martong added a reviewer: teemperor.
martong added a comment.
Thanks, nice work! I like the direction, however, I'd like to ask comments from
@teemperor .
================
Comment at: clang/lib/AST/ASTImporter.cpp:656-671
+ // Helper for chaining together multiple imports. If an error is detected,
+ // subsequent imports will return default constructed nodes, so that
failure
+ // can be detected with a single conditional branch after a sequence of
+ // imports.
+ template <typename T> T importChecked(Error &Err, const T &From) {
+ // Don't attempt to import nodes if we hit an error earlier.
+ if (Err)
----------------
What's the reason of moving this hunk?
================
Comment at: clang/lib/AST/ASTImporter.cpp:8473-8474
+
+ ToAttrName = Importer.Import(FromAttr->getAttrName());
+ ToScopeName = Importer.Import(FromAttr->getScopeName());
+ ToAttrRange = NImporter.importChecked(Err, FromAttr->getRange());
----------------
Why can't we use `importChecked` here?
================
Comment at: clang/lib/AST/ASTImporter.cpp:8485-8486
+ T *ToAttr = T::Create(Importer.getToContext(), ImportedArg..., ToI);
+ // T *ToAttr = new (Importer.getToContext()) T(Importer.getToContext(),
ToI,
+ // ImportedArg...);
+
----------------
Could you please remove this comment?
================
Comment at: clang/lib/AST/ASTImporter.cpp:8547-8548
+ Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(
+ From, AI.importArrayArg(From->args(), From->args_size()).value(),
+ From->args_size());
+ if (ToAttrOrErr)
----------------
Could we hide these arguments?
I mean we probably need a higher abstraction, something like
```
Expected<Attr *> ToAttrOrErr = AI.createImportedAttr(From);
```
should be sufficient, isn't it. We do want to import all arguments of all kind
of attributes, don't we?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D109608/new/
https://reviews.llvm.org/D109608
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
