-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Cleanups 20180401 #7458
Cleanups 20180401 #7458
Conversation
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.
Nice!
var c: TTraversalClosure | ||
var p = newProc(nil, m) | ||
result = "Marker_" & getTypeName(m, origTyp, sig) | ||
var typ = origTyp.skipTypes(abstractInst) | ||
if typ.kind == tyOpt: typ = optLowering(typ) | ||
|
||
case reason | ||
of tiNew: c.visitorFrmt = "#nimGCvisit((void*)$1, op);$n" | ||
else: assert false |
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 know very little about this code, but I have suspicion that removing this case statement is a bad idea. New reasons could be added in the future, and it would be good to get an error for them.
Oh, I see you've actually removed the enum.
compiler/vmgen.nim
Outdated
@@ -1767,13 +1767,6 @@ proc gen(c: PCtx; n: PNode; dest: var TDest; flags: TGenFlags = {}) = | |||
of skConst: | |||
let constVal = if s.ast != nil: s.ast else: s.typ.n | |||
gen(c, constVal, dest) | |||
of skEnumField: |
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.
How is that a code cleanup? Don't touch this.
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.
mainly because it looks like we never get here - afaict skEnumField
gets converted to an int node in semfold.nim
leading to this branch never being taken.. can add it back, though lit looks pretty dead.
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.
It's more robust to not assume the frontend did this transformation to the underlying integer value. Don't remove this code.
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.
sure, added a comment instead.
fyi, the js backend doesn't handle it either and there's a number of other sk...
cases that get the internalError
treatment here through an else branch.
random code readability fixes & dead code cleanups