[ODS] StructAttr arguments backwards

[lattner]

Bugzilla Link	51909
Version	unspecified
OS	All
CC	@jpienaar,@River707

Extended Description

You define a StructAttr in ODS like this:

def OutputFileAttr : StructAttr<"OutputFileAttr", HWDialect, [

but you define an AttrDef in the opposite order:

def VerbatimParameterValueAttr : AttrDef<HWDialect, "VerbatimParameterValue"> {

It seems like StructAttr is backwards. Also, neither AttrDef nor StructAttr seem to have much documentation.

-Chris

[lattner]

The accessor naming topic is pretty separable from the StructAttr specific things in this PR so I moved it to llvm/llvm-bugzilla-archive#51916

[lattner]

This has long been a bug in my mind. MLIR is currently has this as dissonance between the LLVM style and something else. I'm not sure where the "void fooAttr(newValue)" style of setters came from, even the Google style uses "set": https://google.github.io/styleguide/cppguide.html#Function_Names

I recognize that changing this at this point would be very disruptive, but I think it would be totally fine to have (e.g.) an ODS-dialect level "accessor style" property that could be set to "LLVM, Google, old-mlir" and the default could be old-mlir.

For the dialects I maintain, I'd very happily move them to LLVM style to get get/set consistently, and I'm sure that some google clients would prefer to have "set_thing" for consistency with their other code.

[jpienaar]

That may be, but it is less consistent with the rest of MLIR generators (attributes, operands, regions, op adaptors). That is fine to consider, but I'd say that is orthogonal to the order of StructAttr here (which seems good to change) and would be a more invasive change to ensure consistency [making it opt-in, as previously discussed, could allow for a simple migration path here]

[lattner]

getAttr/setAttr are way nicer (more consistent with LLVM) than fooAttr() and fooAttr(newValue).

[jpienaar]

Re #​3: AttrDef is the one inconsistent wrt prepending get and changing case compared to the rest of the generators.

[lattner]

StructAttr also doesn't convert getters to the right case and prefix with a 'get' name like AttrDef does. This makes the name accessor be thing.name() instead of the nicer and more consistent thing.getName() that AttrDef provides.

More differences are illuminated by this patch moving a StructAttr to an AttrDef: https://github.com/llvm/circt/commit/9d1a362dfd22c992aeb4375ae36b8a23cf6fe1c6

[lattner]

... and the ::get method on the StructAttr has its MLIRContext member last instead of first like the rest of the builders.

[lattner]

Also, the string passed to AttrDef gets an implicit "Attr" suffix, but StructAttr doesn't.

[lattner]

assigned to @joker-eph