[PATCH] D73362: [mlir] EnumsGen: dissociate string form of integer enum from C++ symbol name

Mon Jan 27 01:47:40 PST 2020

ftynse added a comment.
Herald added a reviewer: rriddle.

In D73362#1839215 <https://reviews.llvm.org/D73362#1839215>, @antiagainst wrote:

> Thanks Alex for adding this! Have you considered adding an additional parameter (defaulting to C++ enumerant symbol) for the string representation instead of duplicating all the cases?

I'm not sure I quite understand. I did add an additional parameter to `EnumAttrCaseInfo` and it is defaulted to C++ enumerant symbol for `StrEnumAttrCase` and for `I32EnumAttrCase`/`I64EnumAttrCase` that are currently in use. It made more sense to me to have this value at the top-level class exactly because it could be useful for other kinds of attributes. I could not have `IntEnumAttrCaseBase` that inherits from `IntStrEnumAttrCaseBase` for the purposes of defaulting the string representation to C++ enumerant since, in this case, I32EnumAttr and I64EnumAttr would have to be duplicated instead. They accept I32/64StrEnumAttrCase so can also accept the derived I32/64EnumAttrCase now, but it would not be correct if the defaulting happened before the type is specified.

>   I can see this is also useful for BitEnumAttrCase and StrEnumAttrCase so it ends up we need to duplicate all the cases. Besides, the name of `IntStrEnumCase` can be quite confusing, given we have `IntEnumCase` and `StrEnumCase` (and in the future may have `StrStrEnumCase`/`BitStrEnumCase`).

How about AnnotatedIntEnumCase ?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73362/new/

https://reviews.llvm.org/D73362