-
Notifications
You must be signed in to change notification settings - Fork 145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding simplification attribute to non-constructor-based equalities #250
Adding simplification attribute to non-constructor-based equalities #250
Conversation
@@ -42,6 +42,7 @@ | |||
public static final KLabel SetItem = KLabel("SetItem"); | |||
public static final KLabel DotMap = KLabel(".Map"); | |||
public static final KLabel List = KLabel("_List_"); | |||
public static final KLabel DotSet = KLabel(".Map"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.Set
or .Map
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, the order of these constants makes little sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this! And abput the order, I agree, but it's not like it was very ordered before :-)
d2585c9
to
38055a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable to me, but I'd like someone with more kframework experience to approve.
@@ -0,0 +1,56 @@ | |||
package org.kframework.compile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the other files have copyright statements.
def constructorBased(m: Module, term: K): Boolean = term match { | ||
case kapp: KApply => immutable(kapp.klist.items).forall(constructorOnly(m)) | ||
case t: KAs => constructorBased(m, t.pattern) | ||
case other => true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment explaining why returning true here works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really rather we didn't add to the amount of scala we are using in the project unnecessarily as it's already a source of problems on occasion
@dwightguth what is your exact suggestion here? |
I mean my suggestion is to write ConstructorChecks in Java. |
0b3196b
to
7f8f117
Compare
@dwightguth I've rewritten it in Java. Please re-review. |
6d252ea
to
7367e6c
Compare
7367e6c
to
0614211
Compare
Fixes #203 --------- Co-authored-by: devops <devops@runtimeverification.com>
Fixes #203 --------- Co-authored-by: devops <devops@runtimeverification.com>
No description provided.