-
Notifications
You must be signed in to change notification settings - Fork 161
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
Fix scope_depth
to return 0 for root scope
#424
Conversation
Codecov Report
@@ Coverage Diff @@
## master #424 +/- ##
==========================================
+ Coverage 64.87% 64.96% +0.09%
==========================================
Files 52 52
Lines 8256 8278 +22
==========================================
+ Hits 5356 5378 +22
Misses 2900 2900
Continue to review full report at Codecov.
|
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 left a few comments. Also thanks for adding tests!
fn unchecked_as_ref(x: *const ScopeRaw) -> &ScopeRaw { | ||
// SAFETY: `current.parent` necessarily lives longer than `current`. | ||
this = current.parent.map(|x| unsafe { &*x }); | ||
unsafe { &*x } | ||
} |
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.
Although this is a scoped function, I believe it technically still needs to be unsafe
. In that case, it might just be better to inline unsafe { &*x }
let mut depth = 0; | ||
let mut next = cx.raw.parent.map(unchecked_as_ref); | ||
|
||
while let Some(current) = next { | ||
next = current.parent.map(unchecked_as_ref); | ||
depth += 1; | ||
} | ||
depth |
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 have another idea of an implementation that might be simpler:
while let Some(next) = current.parent.map(unsafe { &*x }) { ... }
Hi 👋,
Just a quick fix so that the
scope_depth
function returns0
for the root scope as stated by the comment -I also added some tests for the function too.
This function is only ever used in this repository for the following SSR test:
sycamore/packages/sycamore/tests/ssr/main.rs
Lines 120 to 136 in 487877f
that relies solely on the fact that depth is incremented on child scopes.