String index fixes and cleanups #1937
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
After some discussion on the developers list, I agreed that having a negative index feature is nice, but the extra overhead (54 bytes of program space even when not using it) is not worth it. I've removed the patch that added support for this, leaving just the related (but independently useful) bugfix and cleanup patches. I've added two more bugfixes related to invalid strings as well. |
matthijskooijman
changed the title
matthijskooijman
added
Component: Core
This check already happens in the remove(unsigned int, unsigned int) method that is caled, so there is no need to also check this here.
Previously, if you passed in a very big index and/or count, the `index + count` could overflow, making the count be used as-is instead of being truncated (causing the string to be updated wrongly and potentially writing to arbitrary memory locations). We can rewrite the comparison to use `len - index` instead. Since we know that index < len, we are sure this subtraction does not overflow, regardless of what values of index and count we pass in. As an added bonus, the `len - index` value already needed be calculated inside the if, so this saves a few instructions in the generated code. To illustrate this problem, consider this code: String foo = "foo"; Serial.println(foo.length()); // Prints 3 foo.remove(1, 65535); // Should remove all but first character Serial.println(foo.length()); // Prints 4 without this patch Not shown in this is example is that some arbitrary memory is written as well.
Previously, this method calculated the length of the string from the given index onwards. However, the other remove() method called already contains code for this calculation, which is used when the count passed in is too big. This means we can just pass in a very big count that is guaranteed to point past the end of the string, shrinking the remove method by a few bytes.
matthijskooijman
force-pushed
the
stringindex
branch
from
6595f2f to
8463ae3
Compare
When checking the `left` argument, it previously allowed having left == len. However, this means the substring starts one past the last character in the string and should return the empty string. In practice, this already worked correctly, because buffer[len] contains the trailing nul, so it would (re)assign the empty string to `out`. However, fixing this check makes it a bit more logical, and prevents a fairly unlikely out-of-buffer write (to address 0x0) when calling substring on an invalidated String: String bar = (char*)NULL; bar.substring(0, 0);
matthijskooijman
force-pushed
the
stringindex
branch
from
8463ae3 to
04dba1e
Compare
|
I've rebased this commit, dropped two commits (of which the second reverted the first - apparently I removed a check in the first commit, thinking it wasn't needed and then re-added it later not realizing I removed it in an earlier commit) and slightly changed the last commit to fix the same (unlikely) problem in a different way. I also added some examples of code that would break in two of the commit messages. |
|
@ArduinoBot build this please |
|
Merged build finished. Test PASSed. |
cmaglie
added a commit
that referenced
this pull request
Sep 12, 2014
String index fixes and cleanups
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This pullrequest adds support for using negative indices in Strings to index a String from the end instead of the start. This changes all methods that accept indices:
operator[],charAt,setCharAt,indexOf,lastIndexOf,substringandremove.I also added some fixes and simplifications I ran across while implementing the negative indices.