[PATCH] D109608: [clang][ASTImporter] Generic attribute import handling (first step).

[Balázs Kéri via Phabricator via cfe-commits]

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