balazske added inline comments.
================
Comment at: clang/lib/AST/ASTImporter.cpp:8425
+
+ const T &value() { return To; }
+};
----------------
steakhal wrote:
> 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?
The data returned by `value` is the one that is passed to the attribute
constructor. It is in exact the same form, and for arrays this is a simple
pointer to the array data. The array in the attribute has a size parameter too,
this can be passed separately from this object.
One such "importer" is created for every attribute argument even if it is of an
array type. For the array size no such object has to be created, only for the
array data.
================
Comment at: clang/lib/AST/ASTImporter.cpp:8460-8462
+ template <class AT>
+ AttrArgArrayImporter<AT>
+ importArrayArg(const llvm::iterator_range<AT *> &From, unsigned ArraySize) {
----------------
steakhal wrote:
> 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>);
> ```
In this case `AT` is the element type of the array attribute argument, mostly
`Expr *` but can be any other. The `AT` should mean "Argument Type". Now is is
renamed to `T` because it is the only parameter, and every other place `T` is
used too. I think it is not needed to make a static assert for every possible
attribute argument type. The class can be still used incorrectly and
theoretically attribute argument can be of any type, so the assert adds not
much value.
================
Comment at: clang/lib/AST/ASTImporter.cpp:8466
+
+ template <class T, typename... Arg>
+ Expected<Attr *> createImportedAttr(const T *FromAttr, Arg &&...ImportedArg)
{
----------------
steakhal wrote:
> I think you should be consistently using `typename` or `class`. I'm generally
> in favor of using `typename` though.
Here the first parameter is really a class, the second not.
================
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)
----------------
steakhal wrote:
> 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?
Probably we can have a specialization for exactly the case when the attribute
has one parameter called "Args" that is of an array type. We need to check at
how many attributes this is the case, probably it is only used at the thread
safety part (relatively) many times.
================
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();
----------------
steakhal wrote:
> An iterator range is just a pair of iterators, am I right?
> We could still pass them by value.
Probably enough to pass by value but this should be not worse than that.
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
