Closes #5325: Switch BinOp.chpl and OperatorMsg.chpl to using enums #5326
Conversation
1RyanK
changed the title
1RyanK
force-pushed
the
5325-switch_binop_chpl_and_operatormsg_chpl_to_using_enums
branch
2 times, most recently
from
7520d65 to
748ed5f
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5326 +/- ##
========================================
Coverage ? 100.00%
========================================
Files ? 4
Lines ? 63
Branches ? 0
========================================
Hits ? 63
Misses ? 0
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1RyanK
force-pushed
the
5325-switch_binop_chpl_and_operatormsg_chpl_to_using_enums
branch
from
748ed5f to
b2cb094
Compare
| } | ||
| select op { | ||
| when "//" { // floordiv | ||
| when Operator.FloorDiv { // floordiv |
There was a problem hiding this comment.
comment is not necessary anymore thanks to self-documenting code!
| visted = true; | ||
| } | ||
| when "%" { // modulo " <- quote is workaround for syntax highlighter bug | ||
| when Operator.Mod { // modulo " <- quote is workaround for syntax highlighter bug |
There was a problem hiding this comment.
ditto. I am almost certain the workaround is no longer necessary as you removed the quote, too.
| } | ||
| when "**=" { l.a **= r.a; } | ||
| when "%=" { | ||
| when OpEq.Ee { l.a **= r.a; } |
There was a problem hiding this comment.
This is probably the most cryptic name to me.
There was a problem hiding this comment.
The idea was "exponent-equals," I already have "Pe" for "plus-equals," so I didn't want to put it as "pow-equals." I guess I could do "Poe" for "pow-equals" in the same way I have "Moe" for "mod-equals."
There was a problem hiding this comment.
I see. I don't think this is a single point that needs changing. See my other comment for naming change suggestions.
| enum OpEq { | ||
| Invalid, | ||
| Pe, Me, Te, De, Fde, Moe, Ee, Oe, Ae, Xe, Sle, Sre | ||
| } |
There was a problem hiding this comment.
I have some notes here:
- Is the separation of enums well-motivated? From a systems perspective, everything is an operator. Whether it is a compound operator that modifies LHS doesn't seem very relevant to me.
- If we are separating enums, and using qualified names like
Operator.Add, andOpEq.Pe, we can keep the enum values identical. i.e.Operator.Add, andOpEq.Add. Though I don't like this idea. - Instead, regardless of whether we use the same or separate enums, I prefer
AddandAddE(orAdde). As per Chapel style guidelines, they should beoperator.addandoperator.addE. (both the enum and the values are ought to be snakeCase). I don't necessarily think we should enforce that here, but just noting. As it stands, one operator is referring to the operation, the other is referring to the operator. This requires more mental energy from the maintainer. - If you want to avoid qualifying enums' values (i.e.
Operator.Addvs justAdd), you canuse Operator. Verbosity didn't bother me here at all, but just wanted to make sure that you are aware of that option.
4 participants
Switched all of the operator usage to enums. It may look like a lot of code but just about everything is converting to enums.
Closes #5325: Switch BinOp.chpl and OperatorMsg.chpl to using enums