Repository navigation
Replace deprecated std::istrstream with std::istringstream - #3214
Conversation
dosubot
Bot
added
the
size:L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (23)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (14)
📝 WalkthroughWalkthroughThis PR replaces deprecated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Summary of ChangesHello @suykerbuyk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request systematically updates the codebase to use Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a great pull request that addresses the use of the deprecated std::istrstream. The changes are consistent across all files, and the detailed PR description is very helpful in understanding the reasoning behind the changes and their safety. I have one suggestion to improve the robustness of the C-API.
| bool is_ok = false; | ||
|
|
||
| std::istrstream is(data, n); | ||
| std::istringstream is(std::string(data, n)); |
There was a problem hiding this comment.
Constructing a std::string from a null pointer with a non-zero size is undefined behavior. Since this is a C-API, it would be safer to add a check to ensure data is not null when n > 0 before this line is executed, for instance at the beginning of the function.
There was a problem hiding this comment.
Good catch. The original istrstream(data, n) had the same null-pointer UB, but since we're touching this line anyway, I've added an early if (\!data || n <= 0) return nullptr; guard at the top of the function. Updated in the force-push.
std::istrstream (from <strstream>) has been deprecated since C++98 and produces -Wdeprecated-declarations warnings on GCC 14+ and Clang 18+. Replace all 37 usages across 23 files with std::istringstream (from <sstream>). The zero-copy property of istrstream is not needed at any of these call sites — all source buffers outlive the stream and the data sizes are small (symbol tables, lexicons, WAV chunks, FST rules). For files that already include <sstream> (symbol-table.cc, lexicon.cc, audio-tagging-label-file.cc, piper-phonemize-lexicon.cc, offline-tts-character-frontend.cc, matcha-tts-lexicon.cc, homophone-replacer.cc, melo-tts-lexicon.cc, character-lexicon.cc, kokoro-multi-lang-lexicon.cc), the redundant <strstream> include is simply removed.
cbb629c to
77d6773
Compare
…pstream PR Bump sherpa-onnx from v1.10.39 (73 releases behind) to v1.12.26 via the suykerbuyk/sherpa-onnx fork's fix/replace-deprecated-istrstream branch, which replaces all 37 deprecated std::istrstream usages across 23 sherpa-onnx source files with std::istringstream. Upstream PR: k2-fsa/sherpa-onnx#3214 CMakeLists.txt changes: - GIT_TAG v1.10.39 → fork branch fix/replace-deprecated-istrstream - Removed CMAKE_MODULE_PATH workaround (fixed upstream in v1.12.26 — sherpa-onnx now uses CMAKE_CURRENT_SOURCE_DIR for module paths) - Removed include_directories workaround (fixed upstream — sherpa-onnx now uses PROJECT_SOURCE_DIR for include paths) - Replaced manual FetchContent_Populate + add_subdirectory with FetchContent_MakeAvailable(sherpa-onnx) since workarounds are gone - Kept CMAKE_POLICY_VERSION_MINIMUM 3.5 (still needed for transitive dep kaldi-native-fbank) Result: zero build warnings from sherpa-onnx (previously 10 istrstream deprecation warnings). All 134 unit tests pass (522 assertions). Once upstream merges the PR, switch back to k2-fsa/sherpa-onnx with the release tag containing the fix.
…mit upstream PR #3215 Switch sherpa-onnx GIT_TAG from fix/replace-deprecated-istrstream to fix/gcc15-deprecation-warnings, a combined fork branch that includes both the istrstream fix (PR #3214) and the new wstring_convert fix (PR #3215). The wstring_convert fix replaces all std::wstring_convert / codecvt_utf8 usage across 6 sherpa-onnx source files with manual UTF-8 codec functions implementing RFC 3629. Changes were made on the fork at /tmp/sherpa-onnx on branch fix/replace-deprecated-wstring-convert, then cherry-picked onto the combined branch. Files changed in sherpa-onnx fork (PR #3215): text-utils.h — Add Utf32ToUtf8(char32_t) overload text-utils.cc — Remove <codecvt>, rewrite 4 functions kokoro-multi-lang-lexicon.cc — Use text-utils functions piper-phonemize-lexicon.cc — Use text-utils functions offline-tts-character-frontend.cc — Use text-utils functions matcha-tts-lexicon.cc — Remove local Utf32ToUtf8 (now shared) Files changed in recmeet: CMakeLists.txt — GIT_TAG → fix/gcc15-deprecation-warnings Result: zero C++ deprecation warnings from sherpa-onnx when building recmeet with GCC 15.2. All 134 unit tests pass (522 assertions). Upstream PRs: k2-fsa/sherpa-onnx#3214 (istrstream → istringstream) k2-fsa/sherpa-onnx#3215 (wstring_convert → manual UTF-8 codec)
csukuangfj
left a comment
csukuangfj
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
2 participants
Summary
std::istrstream(from<strstream>) has been deprecated since C++98 and produces-Wdeprecated-declarationswarnings on GCC 14+ and Clang 18+. This PR replaces all 37 usages across 23 files withstd::istringstream(from<sstream>).Changes
std::istrstream is(buf.data(), buf.size())→std::istringstream is(std::string(buf.data(), buf.size()))new std::istrstream(buf.data(), buf.size())→new std::istringstream(std::string(buf.data(), buf.size()))<sstream>, the redundant<strstream>include is simply removedoffline-tts-pocket-impl.hwhich usedistrstreamwithout including<strstream>(transitive include),<sstream>was added directlyoffline-tts-zipvoice-impl.hwhich includes<strstream>but never usesistrstream, replaced with<sstream>Safety
The zero-copy property of
istrstreamis not needed at any of these call sites:buffromReadFile,datafrom C API) outlive the stream usageistringstreamis actually safer — the stream owns its data, eliminating any dangling-pointer riskFiles changed (23)
c-api/c-api.ccsymbol-table.cc,lexicon.cc,offline-recognizer-impl.cc,online-recognizer-impl.cc,audio-tagging-label-file.cc,keyword-spotter-transducer-impl.h,piper-phonemize-lexicon.ccoffline-tts-matcha-impl.h,offline-tts-vits-impl.h,offline-tts-character-frontend.cc,offline-tts-kokoro-impl.h,offline-tts-kitten-impl.h,offline-tts-pocket-impl.h,offline-tts-zipvoice-impl.h,matcha-tts-lexicon.cc,melo-tts-lexicon.cc,character-lexicon.cc,kokoro-multi-lang-lexicon.cc,homophone-replacer.ccrknn/keyword-spotter-transducer-rknn-impl.hjni/common.h,jni/wave-reader.ccNo functional or behavioral changes.
Summary by CodeRabbit
Bug Fixes
Chores