Add open enums and enum methods with tests - #631
Conversation
| return b == null; | ||
| } | ||
| CLASS_QUERY, VARIANT_QUERY => { | ||
| if (EnumType.?(tt)) { |
There was a problem hiding this comment.
From this it's clear to me that enums need to be desugared to variants earlier in a more general way. Claude has just smeared special cases from front to middle to backend. There shouldn't be any need to alter the evaluation of operators like this.
There was a problem hiding this comment.
I'm willing to work on simplifying such things. I'm not 100% clear on the desugaring you're hoping for, though. I can see that enums are kind of like variants that have no variant fields. Of course enums have their own "fields", that are implemented as arrays indexed by tag. If the two are to be unified into a single whole, we need some place to hang both kinds of fields. Also while enum class structure mirrors variant class structure (in the case of no generic parameters), enums themselves are single values (or have unique single values associated with them) while variant cases are types (unless they have no fields, in which case they have the same "ambiguity" as enums). If you have a little more guidance I would be more confident revising code ...
There was a problem hiding this comment.
Ah, forgive me. It used to be that enums were immediately desugared to a single class declaration. At some point I split off the implementation entirely. If open enums were desugared into open variants instead (i.e. adding a class per enum case), then enum method dispatch would just be variant dispatch. Enum fields would still be implemented via field arrays and use VariantGetTag as the index. Unboxing will just eliminate the overhead of boxing enum values.
There was a problem hiding this comment.
I get the idea but am foggy as to the particulars. Specifically I wonder about the field arrays, and maybe also enumsets. Is the desugaring proposed to happen in VstSsaGen, so that enums are mostly washed away once we have SSA, but the distinction is more visible in parsing (of course) and verification? This would make sense in that, once verified, enums could be mapped down to other structures (variants, field arrays) and marked to be unboxed (always). I'd have to get more deeply into it to see where issues might come up with enumsets, but again, suitable use of VariantGetTag would get the raw numeric values required. So, I could try to combine processing in verification (and maybe parts of parsing), but strive to eliminate separate processing once in SSA.
| } | ||
| def newIrClass(ctype: Type, superClass: IrClass, decl: VstCompound) -> IrClass { | ||
| var ic = IrBuilder.new(ctype, superClass).buildClass(decl); | ||
| var builder = IrBuilder.new(ctype, superClass); |
There was a problem hiding this comment.
I don't know why it feels the need to add the IR module to the builder now.
There was a problem hiding this comment.
Apparently the field and this code are dead, so I'm removing it.
| if (rc.isUnboxed()) { | ||
| // move flattened data type receiver to function sig | ||
| ftype = Function.prependParamTypes(rc.variantNorm.sub, ftype); | ||
| } else if (EnumType.?(rc.oldType)) { |
There was a problem hiding this comment.
If enums are properly desugared to variants, this will just fall out of the normal unboxing.
There was a problem hiding this comment.
isUnboxed means that normalization did the unboxing - but enums are implicitly already unboxed, so the test doesn't work for them. Unifying these more would be a bigger change. Is that how you would prefer to go?
| rm.norm.flags |= IrFlag.M_OVERRIDE; | ||
| sm.norm.flags |= IrFlag.M_OVERRIDDEN; | ||
| } | ||
| // For enum methods, M_OVERRIDDEN must be transferred from original |
There was a problem hiding this comment.
Ultimately this appears necessary because enums don't use regular vtables so their overrides are processed differently. That don't use regular vtables because individual enum cases are not types and thus don't have a corresponding RaClass, etc. Again, is this deeper change one you would prefer to happen?
| setMtable(l.head, rv); | ||
| } | ||
| } | ||
| def layoutEnumMtable(rv: RaVirtual, rm: RaMethod, rc: RaClass) { |
There was a problem hiding this comment.
More replications with special cases from our AI friends. As this is very tricky, we definitely don't want to do this again.
There was a problem hiding this comment.
Part of the same effects from enum cases not having an RaClass and thus not lining up with the existing dispatch mechanisms.
…s variants and enums more similarly
…id improperly identifying 0 tag with null
2 participants
This combines the open enums work into a single commit. There were few minor conflicts, which have been worked out. Passes all CI locally.