https://github.com/AaronBallman commented:
Thank you for working on this, and sorry for the delayed review (holiday schedules are hard). In general, I think we should be doing this with tablegen from the start. With the current patch, we have to update a few switch statements for the attribute to be checked properly and I'm worried people will just continue to do ad hoc solutions here instead of doing the tablegen work, especially as the list grows longer (it's easier to do the tablegen work when there's 3 attributes than when there's 40; and we have ~550 attributes in total so we know the ad hoc solution won't scale). And when I think about the fact that a lot of this is keyed off the attribute *kind* rather than the spelling, I get worried that the current approach isn't really the direction we should be going. The trouble is: * Same attribute kind doesn't mean same attribute; we have attributes with a single semantic `Attr` subclass but multiple spellings that mean different things. * We also have the situation where attributes share the same spelling but are different semantic attributes. I think this situation is okay though because those attributes should always be target-specific and I think structural equivalence across targets is handled elsewhere anyway. * Different attribute arguments don't mean the two declarations aren't structurally equivalent. e.g., `[[deprecated]]` with different messages aren't *structurally* different, are they? But even with availability as in this PR, is a version of `10` structurally different from `10.0` different from `10.0.0`? There's also `AvailabilityAttr::equivalentPlatformNames()`, etc. Would you be amenable to doing the tablegen approach instead? I was thinking of a design like: * If there's no other marking in the attribute definition, then a strict equivalence is checked (same kind, same spelling, identical arguments (so `0` and `0 + 0` are structurally different). * Otherwise, there can be a custom comparator implemented explicitly via a `code` field with a signature along the lines of `bool (const T *, const T *)`. But the design is flexible if you've got other ideas, too. https://github.com/llvm/llvm-project/pull/168769 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
