Add esc option to muse::strings join and split utils#74
Conversation
📝 WalkthroughWalkthroughThis PR extends the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/global/stringutils.cpp`:
- Around line 88-101: The loop that checks s.compare(i, sep.size(), sep) must be
guarded against an empty separator to avoid zero-length matches and size_t
underflow; update the code around the esc/sep handling (the branch using
variables esc and sep) to either assert(!sep.empty()) or add a runtime guard
such as if (!sep.empty() && s.compare(i, sep.size(), sep) == 0) so the separator
branch is skipped when sep is empty; apply the same protection to the
corresponding split function (the split/delim comparison) to prevent infinite
loops.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b10b3ec-86f8-44a1-80b7-51e4bcda1ddd
📒 Files selected for processing (4)
framework/global/stringutils.cppframework/global/stringutils.hframework/global/tests/CMakeLists.txtframework/global/tests/stringutils_tests.cpp
saintmatthieu
force-pushed
the
string-join-and-split-fixes
branch
from
22a0a50 to
2bcf179
Compare
| } | ||
|
|
||
| void muse::strings::split(const std::string& str, std::vector<std::string>& out, const std::string& delim) | ||
| void muse::strings::split(const std::string& str, std::vector<std::string>& out, const std::string& delim, |
There was a problem hiding this comment.
Maybe it would be better to make two functions - keep the old "simple" one as is, and add a new one with a new parameter and logic?
There was a problem hiding this comment.
I have force-pushed a change that reduces the diff. Now you can see clearly that if the utils are called without the escape overload, it will pick the previous, simpler implementation.
That said, I can now confidently say that we do not need the simpler implementation at all anymore. join_handle_escape and split_handle_escape are generalizations. If they are used even when the escape argument is empty, the unit tests still pass. I also took some time debugging the machine-generating algorithm to gain an intuitive understanding. It's not trivial but not too complicated either.
saintmatthieu
force-pushed
the
string-join-and-split-fixes
branch
from
2bcf179 to
7f8bce9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/global/tests/stringutils_tests.cpp`:
- Line 145: Remove the redundant size assertion EXPECT_EQ(out.size(), 5u); from
the test in stringutils_tests.cpp because the subsequent EXPECT_EQ(out, parts)
already verifies content and size; leave the equality assertion using out and
parts (or only keep EXPECT_EQ(out, parts)) so the test remains clear and
non-duplicative.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b4a9462-c070-4cc6-b921-721974cc8241
📒 Files selected for processing (4)
framework/global/stringutils.cppframework/global/stringutils.hframework/global/tests/CMakeLists.txtframework/global/tests/stringutils_tests.cpp
|
|
||
| std::vector<std::string> out; | ||
| strings::split(joined, out, "_", "\\"); | ||
| EXPECT_EQ(out.size(), 5u); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Redundant size assertion.
Line 146 already verifies out == parts, which implies size equality. The explicit size check on line 145 is redundant.
♻️ Simplify by removing redundant assertion
std::vector<std::string> out;
strings::split(joined, out, "_", "\\");
- EXPECT_EQ(out.size(), 5u);
EXPECT_EQ(out, parts);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EXPECT_EQ(out.size(), 5u); | |
| std::vector<std::string> out; | |
| strings::split(joined, out, "_", "\\"); | |
| EXPECT_EQ(out, parts); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/global/tests/stringutils_tests.cpp` at line 145, Remove the
redundant size assertion EXPECT_EQ(out.size(), 5u); from the test in
stringutils_tests.cpp because the subsequent EXPECT_EQ(out, parts) already
verifies content and size; leave the equality assertion using out and parts (or
only keep EXPECT_EQ(out, parts)) so the test remains clear and non-duplicative.
2 participants
The split and join utils from
muse::stringscan't be used by Audacity's effect-ID resolve logic as it stands, because it does not handle the presence of the escape characters in one of the constituents.Build configuration
audacity: audacity/audacity/master
audacity platforms: linux_x64
musescore: musescore/MuseScore/main
musescore platforms: linux_x64