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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
