Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement the rest of the FTCS marks #14341
Implement the rest of the FTCS marks #14341
Changes from 17 commits
58b5d99
63ffcc0
f1a96c4
a1b9dac
a3ede80
b3c523c
b02d222
43875fd
609e354
2317192
045ceaf
5c33a61
0e184f1
302c99e
0bcb6ed
ba9e1c0
887f16a
9257c1f
c70afe7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 believe that using indices would be better than using
.back()
and adding.insert()
.Rust exposes this problem much more obviously, as you can't "borrow" foreign data easily there without introducing lifetime issues. A common solution is to instead save a numeric index (or similar) of where the data is stored at. In this case you could store a
size_t _currentPromptMarkIndex
which stores the index of the mark inside_scrollMarks
, so that you can safely retrieve it from_scrollMarks
at a later time.It's basically exactly like the
_currentPrompt
pointer since it's still sort of a (relative) memory location, but unlike it, it's much easier to prove that it doesn't reference invalid data (by checking that_currentPromptMarkIndex < .size()
). This way you don't need toinsert()
user marks at the beginning either, allowing you to remove this boolean parameter. Such a numeric index doesn't fix any "stateful" bugs of course, but it does prevent memory lifetime issues, as was the case with the raw pointer.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 just realized that the two positions were for a selection!)
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 am moderately horrified about this actually -- why is this better than
UINT_MAX
or whatever? Do we really want to use-1
to indicate.. uh, something? But, if we really want-1
shouldn't it be signed?