-
-
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
refactoring and yak shaving. #13687
refactoring and yak shaving. #13687
Changes from 33 commits
712cb76
94d4f79
7cede93
70a75ad
2d3f84b
0b82257
eaed2e3
d1b53da
6296db1
05f7fee
abf3f1a
08e8b80
20515e6
15a0c2e
f6f5ded
61d3807
8e2d6c5
5c2954f
c93544c
150b9d6
da70460
8a71755
2e0369a
6ae9f9c
f4ab177
5c5bbfe
0726d1f
675f4b2
ae73bb4
267cd65
6ddb8e5
d71bb0c
9b3ede6
bd9694e
b83dea3
d778c35
5586a09
e8e3c2c
590e225
99fd44d
a9fb475
dc4b8a8
59c15eb
15e449e
5b53c8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -593,7 +593,10 @@ proc typeToString(typ: PType, prefer: TPreferedDesc = preferName): string = | |
of tyUncheckedArray: | ||
result = "UncheckedArray[" & typeToString(t[0]) & ']' | ||
of tySequence: | ||
result = "seq[" & typeToString(t[0]) & ']' | ||
Araq marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if t.sym != nil and prefer != preferResolved: | ||
result = t.sym.name.s | ||
else: | ||
result = "seq[" & typeToString(t[0]) & ']' | ||
of tyOpt: | ||
result = "opt[" & typeToString(t[0]) & ']' | ||
of tyOrdinal: | ||
|
@@ -1623,7 +1626,7 @@ proc isTupleRecursive(t: PType, cycleDetector: var IntSet): bool = | |
if isTupleRecursive(t[i], cycleDetectorCopy): | ||
return true | ||
of tyAlias, tyRef, tyPtr, tyGenericInst, tyVar, tyLent, tySink, | ||
tyArray, tyUncheckedArray, tySequence: | ||
tyArray, tyUncheckedArray, tySequence, tyDistinct: | ||
return isTupleRecursive(t.lastSon, cycleDetector) | ||
else: | ||
return false | ||
|
@@ -1632,6 +1635,23 @@ proc isTupleRecursive*(t: PType): bool = | |
var cycleDetector = initIntSet() | ||
isTupleRecursive(t, cycleDetector) | ||
|
||
proc isRecursivePointer(typ: PType, isPointer: bool): bool = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAICT this is only used for recursing, the exported overload without the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about is. Initially I had a version with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand this explanation. Rename the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is recursive. And There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Araq would it help if I rename this proc to |
||
if typ == nil: | ||
return false | ||
case typ.kind | ||
of tyObject, tyTuple: | ||
return isPointer and canFormAcycle(typ) | ||
of tyRef, tyPtr: | ||
return isRecursivePointer(typ.lastSon, isPointer = true) | ||
of tyAlias, tyGenericInst, tyVar, tyLent, tySink, tyArray, tyUncheckedArray, tySequence, tyDistinct: | ||
return isRecursivePointer(typ.lastSon, isPointer) | ||
else: | ||
return false | ||
|
||
proc isRecursivePointer*(t: PType): bool = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can remove this overload and just defined: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, but I prefer it the way it is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @timotheecour The explanation for why I prefer it this way is, I don't want the |
||
isRecursivePointer(t, false) | ||
|
||
|
||
proc isException*(t: PType): bool = | ||
# check if `y` is object type and it inherits from Exception | ||
assert(t != nil) | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,11 +179,7 @@ proc `[]=`*[T](c: var CritBitTree[T], key: string, val: T) = | |
template get[T](c: CritBitTree[T], key: string): T = | ||
let n = rawGet(c, key) | ||
if n == nil: | ||
when compiles($key): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the other version much better than relying on the new "fact" that "everything has a dollar operator" (which isn't true in your PR either btw, callback types cannot be stringified). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid point with the callback type. As I said, this |
||
raise newException(KeyError, "key not found: " & $key) | ||
else: | ||
raise newException(KeyError, "key not found") | ||
|
||
raise newException(KeyError, "key not found: " & $key) | ||
n.val | ||
|
||
proc `[]`*[T](c: CritBitTree[T], key: string): T {.inline.} = | ||
|
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.
use a template or macro to reduce repetitions
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.
No thanks, I prefer repetition here.
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.builtinTypeSubstitution(($value.kind)[2..^1].toLowerAscii)
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.
@Araq, I know about this shorter code. But I really don't like unnecessary string allocations. Your version allocates two intermediate temporary strings, where my version creates none. Did you take that into account?
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.
you can cache these into a RT (or even CT) table and avoid allocs as well as code duplication.
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.
Yeah I did, allocations for debug code are irrelevant.