balazske added inline comments.
================
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)
----------------
martong wrote:
> What's the reason of moving this hunk?
This is moved into the public section. Other such functions are public already.
================
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());
----------------
martong wrote:
> Why can't we use `importChecked` here?
For `Identifier*` no error is possible and `importChecked` does not work (could
be added to have uniform code).
================
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)
----------------
martong wrote:
> 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?
It should be possible to pass every kind of needed arguments (after it is
imported) to the constructor of the newly created `Attr` object. The problem is
solved here by the `AttrArgImporter` that imports the object and can store it
in a temporary value until it is passed to the constructor. The temporary value
is important if an array is imported. To import the array the size must be
passed to the array importer before, and the size must be passed to the
constructor of the `Attr` too, therefore it exists 2 times in the code. An
`AttrArgImporter` can provide only one value to the constructor of the new
`Attr` object. (Probably other solution is possible with `std::tuple` and
`std::apply` but not sure if it is better.)
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
