-
-
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 all 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 |
---|---|---|
|
@@ -1641,6 +1641,23 @@ proc isTupleRecursive*(t: PType): bool = | |
var cycleDetector = initIntSet() | ||
isTupleRecursive(t, cycleDetector) | ||
|
||
proc isRecursivePointer(typ: PType, isPointer: bool): bool = | ||
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.
Why the
isPointer
here? Isn't that the proc's job to decide?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.
AFAICT this is only used for recursing, the exported overload without the
isPointer
argument is right below this oneThere 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 thought about is. Initially I had a version with
value is SomePointer and value.isRecursive
. But it didn't work out because of distinct types. A distinct ref not being a ref anymore really hurts here. NeitherisNil
nor== nil
work on a distinct ref. My next thought was: "I only really care about cyclic types when the type is also a pointer type, value types can't be cyclic by definition". So I move the "distinct" resolution to the compiler because it was eventually simpler, and probably also a faster implementation.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 don't understand this explanation. Rename the
isPointer
parameter to something that makes it obvious.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 function is recursive. And
isPointer
is just the state of the recursive fold on the type tree. After all we don't care about the result ofcanFormAcycle
isn't a pointer type after all.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 would it help if I rename this proc to
isPointerAux
?