[PATCH] D86169: Initial support for letting plugins perform custom parsing of attribute arguments.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D86169#2236457 <https://reviews.llvm.org/D86169#2236457>, @jonathan.protzenko wrote:

> Thanks for the review!
>
> Regarding Declarator: I briefly mentioned something related in the details section... actually maybe you can tell me what's the best approach here. I was thinking of just passing in a void* and expecting plugins to do
>
>   if (const Declarator *D = isa_and_nonnull<Declarator>(Context))
>
> This would allow a future-proof API where we can, in the future, pass in more classes for the "Context" argument (not just a Declarator), relying on clients doing dynamic casts. If I understood https://llvm.org/docs/ProgrammersManual.html#isa this `void*` cast would work?

I don't think `isa<>` would work well with a `void *` because the LLVM casting infrastructure works by eventually trying to call a static function on a class, and that static function is given a pointer value which it often calls a member function on. If the types are all correct, then things should work out okay, but I'd not want to design a plugin feature around that unsafe of an interface. In fact, I think it would require the user to use C-style casts because you cannot form a reference to a `void *`.

> My only hesitation is I'm not sure about the right technical device to use for this polymorphism pattern, as I've seen a couple related things in the clang code base:
>
> - I've seen a pattern where there's a wrapper class that has a Kind() method and then allows downcasting based on the results of the Kind (seems overkill here)
> - I've also seen references to `opaque_ptr` somewhere with a `get<T>` method... (maybe these are helpers to do what I'm doing better?)
>
> If you can confirm the void* is the right way to go (or point me to a suitable pattern I can take inspiration from), I'll submit a revised patch, along with the extra spelling argument (good point, thanks!).

I don't think `void *` is the right way to go and I'm hoping we can devise a way to not require the entity being attributed to be passed when parsing the attribute. For instance, some attributes are parsed before you know what you're attaching them to: `[[foobar]] <whatever else>;` could be a statement attribute or a declaration attribute, depending on what `<whatever else>` is. Another example is: `int f [[foobar]] <whatever else>;` where the attribute could be appertaining to a function (if `<whatever else>` was `()`) or a variable (if `<whatever else>` was ` = 12`). My intuition is that we're going to need to wait until we know what AST node we're dealing with before asking a plugin to decide whether an attribute appertains to it or not. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86169