aaron.ballman added a comment. In D89743#2406017 <https://reviews.llvm.org/D89743#2406017>, @sammccall wrote:
> In D89743#2363201 <https://reviews.llvm.org/D89743#2363201>, @aaron.ballman > wrote: > >> In D89743#2360258 <https://reviews.llvm.org/D89743#2360258>, @sammccall >> wrote: >> >>> (while this is useful to you, let's keep discussing, but I'm also happy to >>> stop and land this with your preferred API/semantics - just LMK if changes >>> are needed) >> >> Let's hold off on landing for just a little bit. I don't think changes are >> needed yet, but the discussion may change my opinion and we might as well >> avoid churn. However, if you need to land part of this while we discuss >> other parts, LMK. > > Sorry I haven't had more to say on this. I would like to land at least the > attr-in-DynTypedNode part, as this unblocks some stuff unrelated to matchers > in clangd (someone just filed a dupe bug, which reminded me...) That part LGTM and is fine to land. > Happy to cut out all of the matcher part, or land just attr() without any > attr matchers, or all of it, or make another round of changes... I think the `attr()` matcher is definitely reasonable as it stands and can be landed as well. >>> In D89743#2360032 <https://reviews.llvm.org/D89743#2360032>, @aaron.ballman >>> wrote: >>> >>>> >> >> My mental model is that there is no declaration of an attribute (in the >> usual sense, because users cannot specify their own attributes without >> changing the compiler), and so there's not a referential model like there >> are with a DeclRefExpr that refers back to a specific declaration. Instead, >> to me, an attribute is a bit more like a builtin type -- you may have >> multiple ways to spell the type (`char` vs `signed char` or `unsigned char`, >> `long int` vs `long`, etc), but it's the same type under the hood. > > Ah, this also makes sense! Again, the wrinkle is IIUC `attr::Kind` is like a > builtin type, while `Attr` is more like VectorType - it specifies a builtin > type, but also some other stuff. Yeah, that's a fair point. >> However, that brings up an interesting observation: you can't do >> `hasType(hasName("long"))` and instead have to do >> `hasType(asString("long"))`. I don't recall the background about why there >> is `asString()` for matching string types vs `hasName()` -- you wouldn't >> happen to remember that context, would you? > > That was before my time I'm afraid. > It seems the implementation checks whether QualType::getAsString() exactly > matches the argument. `asString("long")` matches a type specified as `long` > or `long int`. But `asString("long int")` doesn't match anything! (Sugar > types muddle things a bit but don't fundamentally change this). > My guess is this implementation was just a question of practicality: parsing > the matcher argument as a type isn't feasible, but having the node producing > a single string to compare to is easy. And it generalizes to complicated > template types. That's my guess as well -- also, I wasn't aware of the `long` vs `long int` behavioral difference with `asString()`, that's.. neat. > It's reasonable to think of these as doing different things `hasName` is > querying a property, while `asString` is doing a specialized comparison of > the whole thing. > I'm not *sure* that's why the different names where chosen though. And the > fact that `hasName` allows qualifiers definitely feels a bit bolted on here. > >> For instance, is the correct pattern to allow `attr(asString("aligned"))` to >> map back to an `AlignedAttr` regardless of how the attribute was spelled in >> source, similar to how `asString("long")` matches `long int`? > > If we're following precedents (not sure we have to): > > - per your mental model, `asString` would pick one fixed name for each > attribute kind - which may not be the one used! > - ... but also `asString` should roughly be "node as string" which logically > includes arguments and should use the right name (since the node records it) > - `hasName` should match against the `name` attribute of Attr, possibly with > optional qualifiers > - ... but there's no precedent for rejecting synonymous names per se, and > this may be confusing behavior > > So I don't think either of these are a *great* fit, assuming we think > "attribute named XYZ or any synonym" is the most important operation (which I > agree with). I agree that this is the most common use-case. > I can't think of a pithy name, but I also think it's OK to be a bit verbose > and explicit here - attributes aren't the most commonly used bit of the AST. > >> Maybe `attr(equivalentAttrName("..."))` or so, possibly constrasting to >> `attr(exactAttrName("..."))`? I think this design is the most flexible and will be the most understandable in the long-run because it makes it clear which name-matching behavior is being used. But do we want to stick `attr` in the name of this to limit it to just attributes or do we think this is a generalized concept that should apply to matching other names as well? e.g., `hasType(equivalentName("long"))` to match either `long`, `long int` or `signed long int` and `hasType(exactName(`long int`))` to only match `long int` and not the other forms? Or do you think this is a bridge too far because it would be reasonable to expect `hasType(equivalentName("long"))` to match typedef names because those are equivalent (or perhaps more problematically, would the user expect `hasType(equivalentName("struct foo<bar>::baz<quux>"))` to match as an equivalent name for the parameter type (spelled with a `using` declaration) in `void foo(blah b)`)? (I think I've just about talked myself into keeping `attr` in the name so we don't try to use this for other kinds of identifiers with equivalent names.) Go ahead and land the dyn node bits and the `attr` matcher. If you feel like being on the hook for doing the other matchers that we're discussing in another review, then yay, but don't feel obligated (though I appreciate the design discussion!). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89743/new/ https://reviews.llvm.org/D89743 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
