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
Test string pointers validity and explain safety #12
Test string pointers validity and explain safety #12
Conversation
cb856a5
to
e1a46cb
Compare
Codecov Report
@@ Coverage Diff @@
## master #12 +/- ##
=======================================
Coverage 94.43% 94.43%
=======================================
Files 2 2
Lines 413 413
=======================================
Hits 390 390
Misses 23 23 Continue to review full report at Codecov.
|
b8c1ed3
to
e2b943c
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.
Thanks for the huge comment describing the safety guarantees of the unsafe code parts.
I am not sure whether it should be a normal //
comment of a doc comment (and where to put it then).
Also it was sometimes hard to read due to some writing mistakes here and there. Please fix them and we can merge.
Rest looks fine.
I used normal |
e2b943c
to
da8b0f3
Compare
Fixed some sentences, but I won't notice some kind of mistakes and unnatural English, because it's not my native tongue... |
Closing this as outdated. |
wrt #10 (comment).
Added
StringInterner::assert_internal_str_refs_validity()
and helper functionStringInterner::max_capacity()
, that are enabled only in test bulid.This detects dangling pointer and overlooked interned strings, and panics if something wrong.
Using this assertion in tests, it will be possible to detect dangling pointers once it appears.
It will be hard to test every possible case, so I added a test for typical case in which the pointer can be modified or invalidated: when interning new string (and specifically when reallocation happens), and when being cloned.
It is
tests::internal_str_refs_validity::intern_reallocate_clone()
.Additionally, I added a comment to explain why the uses of
InternalStrRef
are safe, for future contributors and reviewers.