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