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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I like the adapter layer idea. However, I agree with @martong that it should be more 'abstract' ~ terse.
It's remarkable how compact the test is. Good job.



================
Comment at: clang/lib/AST/ASTImporter.cpp:8417-8418
+template <typename T> struct AttrArgImporter {
+  AttrArgImporter<T>(const AttrArgImporter<T> &) = delete;
+  AttrArgImporter<T>(AttrArgImporter<T> &&) = default;
+
----------------
What about the rest of the special member functions like assignment operators?

All the rest of the code mentions this template parameter using the `AT` name.  Could you please consolidate this? It would make it more readable.


================
Comment at: clang/lib/AST/ASTImporter.cpp:8425
+
+  const T &value() { return To; }
+};
----------------
So, the `value()` sometimes returns const ref, and other times it returns a mutable raw pointer...

I suspect, attribute constructors expect simple reference arguments and pointers for lists. And this is why it behaves like this. Am I right about this?


================
Comment at: clang/lib/AST/ASTImporter.cpp:8439
+      return;
+    } else {
+      To.reserve(ArraySize);
----------------
Please fix this.


================
Comment at: clang/lib/AST/ASTImporter.cpp:8460-8462
+  template <class AT>
+  AttrArgArrayImporter<AT>
+  importArrayArg(const llvm::iterator_range<AT *> &From, unsigned ArraySize) {
----------------
The name `AT` suggests to me that you expect the template type parameter to be a subtype of `class Attr`. However, I suspect it's not always the case.
For example in case of the `OwnershipAttr` the `args()` returns a sequence of `ParamIdx*` objects. So, in that sense, the `AT` name is not properly justified.

Restricting template parameter types makes the code cleaner, so I would suggest introducing a metafunction, that you could use in a `static_assert` that you could use to check this requirement as the first instruction in the function.

```lang=C++
template <typename T>
constexpr static bool AttrOrParamIdx = std::is_base_of<Attr, T>::value || std::is_same_v<T, ParamIdx>;

static_assert(AttrOrParamIdx<T>);
```


================
Comment at: clang/lib/AST/ASTImporter.cpp:8466
+
+  template <class T, typename... Arg>
+  Expected<Attr *> createImportedAttr(const T *FromAttr, Arg &&...ImportedArg) {
----------------
I think you should be consistently using `typename` or `class`. I'm generally in favor of using `typename` though.


================
Comment at: clang/lib/AST/ASTImporter.cpp:8467
+  template <class T, typename... Arg>
+  Expected<Attr *> createImportedAttr(const T *FromAttr, Arg &&...ImportedArg) {
+    const IdentifierInfo *ToAttrName;
----------------
So, this accepts universal references, shouldn't we `std::forward` when we consume them?
If not, why do you use `&&` ?


================
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());
----------------
balazske wrote:
> 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).
Why don't you initialize the variables directly? Same for the other variables. This way they could be const as well.


================
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)
----------------
balazske wrote:
> 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.)
What about having two implementations. One for any T that has `args()` and one for anything else and using SFINAE choosing the right one.
In this context, we should have the appropriate dynamic type, so by using template type deduction on `From` could achieve this dispatch.

Internally it could do the `AI.importArrayArg(From->args(), From->args_size()).value()` for the `args()` case.
WDYT?


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:6414
+    // Verify that dump does not crash because invalid data.
+    ToD->dump();
+
----------------
Should we dump to the `stderr`?


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:6442-6443
+  void importAttr(const char *Code, AT *&FromAttr, AT *&ToAttr) {
+    static_assert(std::is_base_of<Attr, AT>::value, "AT should be an Attr");
+    static_assert(std::is_base_of<Decl, DT>::value, "DT should be a Decl");
+
----------------
<3 More!


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:6467-6468
+  template <class T>
+  void checkImportVariadicArg(const llvm::iterator_range<T **> &From,
+                              const llvm::iterator_range<T **> &To) {
+    for (auto FromI = From.begin(), ToI = To.begin(); FromI != From.end();
----------------
An iterator range is just a pair of iterators, am I right?
We could still pass them by value.


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:6549-6551
+      R"(
+      void test(int A1, int A2) __attribute__((assert_capability(A1, A2)));
+      )",
----------------
You could probably use the regular `"void test(int A1, int A2) __attribute__((assert_capability(A1, A2)));"` string literal. It would still fit in the line and would consume only a single line.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109608/new/

https://reviews.llvm.org/D109608