Changing prefixing for optional UnitAttr accessors to follow LLVM convention.
Nice, overall looks good and reads nicer.
Why can't this just be getGetterName and these merged? Computational efficiency?
Should this just be isNofold() ? (not sure why attr post fix, not specific to your change though so just OOC)
Lets end with s/.'/; method will be removed 2023-06-23/
and could send a PR to https://github.com/llvm/mlir-www/blob/main/website/content/deprecation/_index.md to add it there too.
Could you tag all of these with same date so it is easier to find ?
One thing that's kind of popped up while reading the review is that this going to skew towards attributes with certain naming schemes, which needs to be documented. This is a bit because I feel that 'is' doesn't read well for a number of different cases.
Yes, the unit attribute check is unnecessary for operands, results and other types. Open to suggestions to improve this logic though.
Probably. Not sure where this specific instance of the nofold attribute accessor is defined/generated though.
There are other cases in addition to attribute names that start with "has" where the additional "is" prefix makes it read weird. Handling each case individually does not guarantee consistency and probably isn't feasible. It may be better to update the style guide, so the programmer can ensure proper naming conventions.
is is a verb. But this goes to River's point below: the prefix works better if one follows naming convention accounting for it.
For boolean accessors is and has are common conventions for prefixing boolean return functions. @lattner commented in the linked issue that in LLVM is is the prefix convention followed.
So 4 options here:
For (4) an entry is added to ODS style guide and folks update so that it reads more naturally.
It would yes, but there was opposition to enabling accessors to be customized previously (https://github.com/llvm/llvm-project/issues/51258). This is option (3) above though.
It is unclear how much option (3) benefits vs (2) e.g., with (2) you'd have "has_padding" or "is_continuous" in the printed IR rather than "padding" and "continuous", but no need to do anything in ODS or introduce new class for UnitAttr instances to instantiate. And I almost feel that is more readable. We could also do variant of (2) where it is required that there be a prefix in the Attribute ([a-z]*_ or perhaps just an allowed set), else it gets an is.