fix(StringTools): edge cases in split_up, remove_quotes, append_codepoint, escape_detect#1364
Merged
Conversation
…quotes, append_codepoint, escape_detect - split_up no longer drops a non-delimiter character following a closing quote/bracket; it only skips the next position when it is an actual delimiter. - remove_quotes(vector) skips empty strings to avoid front()/back() UB reachable from config sections with consecutive separators (e.g. [a..b]). - append_codepoint throws on code points above 0x10FFFF instead of silently emitting nothing; surrogate error message now refers to invalid code points rather than UTF-8. - escape_detect returns early when the trigger char is at offset 0, avoiding an unsigned wraparound that searched the whole string. Assisted-by: ClaudeCode:claude-opus-4-8
The previous fix for character loss after a closing quote/bracket resumed parsing from the very next character. When that character is itself a quote/bracket opening character (e.g. a third backtick in ```++), this re-opened a fresh, potentially unterminated, quoted sequence that swallowed following delimiters and merged separate tokens into one. That changed command-line parsing of some fuzz-corpus inputs and broke the FuzzFailTest round-trip invariant (app_roundtrip_custom index 14). Narrow the fix so an ordinary trailing character is still retained, but a quote/bracket opening character immediately after a close is skipped like the delimiter case, matching the original well-behaved splitting. Assisted-by: ClaudeCode:claude-opus-4.8
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
🤖 AI text below 🤖
Part of #1357
Fixes four verified string-handling edge cases in
include/CLI/impl/StringTools_inl.hpp, all confirmed against the current code.1.
split_updropped the character after a closing quote/bracketWhen a token started with a bracket/quote, the code did
output.push_back(str.substr(0, end + 1))then unconditionallystr = str.substr(end + 2), assumingstr[end + 1]was a delimiter. For e.g.split_up("\"abc\"def")thedvanished. Now we only skip positionend + 1when it is an actual delimiter (the localfind_wspredicate); otherwise we resume fromend + 1, so the remainder starts the next token and no characters are lost. New testsSplitUp: TrailingAfterQuote,TrailingAfterBracket, andDelimiterAfterQuoteintests/HelpersTest.cpp.2.
remove_quotes(std::vector<std::string>&)calledfront()/back()on empty strings (UB)The App parse path erases empty strings first, but
generate_parents(Config_inl.hpp) does not:split_up(section, '.')on a section like[a..b]yields empty segments that reachedarg.front()on"". Fixed by skipping empty strings in the loop. New testStringBased: EmptySectionSegmentintests/ConfigFileTest.cppparses[a..b]and asserts it does not crash and still finds the value.3.
append_codepointsilently emitted nothing for code points above 0x10FFFFA
\Uescape computing a code >= 0x110000 fell off the end of the if/else ladder and disappeared, unlike the surrogate case which throws. Added a finalelsethat throwsstd::invalid_argument. Also corrected the surrogate error message (was "are not valid UTF-8" -> "are not valid code points", since these are invalid code points, not bad UTF-8). New assertions inStringTools: unicode_literalscover\U00110000,\UFFFFFFFF, and the largest valid point\U0010FFFF.4.
escape_detectunsigned wraparound at offset 0When the trigger char (
=/:) is the first character,str.find_last_of("-/ \"'\", offset - 1)passednposand searched the whole string (including characters after the trigger), potentially matching a later-//and wrongly rewriting the leading char. Now returnsoffset + 1early whenoffset == 0. New testStringTools: escape_detect`.Test change (old lossy behavior)
AppTest.cppOneStringEqualVersionSingleStringQuotedEscapedCharactershad a stray trailing"after a closing backtick-literal:-m=`"quoted\n string"`". That unbalanced quote was previously silently swallowed by thesplit_upbug (item 1). With the fix it is preserved as a token and rejected as an unexpected argument. The trailing"is malformed input, so I removed it from the test input; the meaningful assertions are unchanged.All tests pass: full
ctestis 24/24 green (HelpersTest, ConfigFileTest, StringParseTest, AppTest included).🤖 Generated with Claude Code