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

[Gabor Marton via Phabricator via cfe-commits]

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