Fix building errors - #2893
Fix building errors#2893
Conversation
dosubot
Bot
added
the
size:L
WalkthroughA comprehensive field rename across multiple language bindings and implementations, renaming Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Areas requiring attention:
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 @csukuangfj, 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 addresses and resolves building errors by refactoring the Highlights
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 pull request primarily refactors the codebase by renaming pinyinDict to lexicon across various files and languages, including Dart, Python, Go, C++, Swift, and JavaScript. This change improves clarity and consistency. The updates have been applied correctly and consistently throughout the project. Additionally, an example in python-api-examples/offline-zeroshot-tts.py has been updated to use a newer model, and some minor style improvements have been made. The changes are well-executed and I don't see any issues.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sherpa-onnx/c-api/cxx-api.cc (1)
154-157: Fix loop variable in multi‑stream decode to avoid UBThe loop uses
for (int32_t i = 0; i != n; ++n)instead of incrementingi, which can overrun thestreamsvector or loop indefinitely.This should be:
- for (int32_t i = 0; i != n; ++n) { + for (int32_t i = 0; i != n; ++i) { streams[i] = ss[i].Get(); }There is an identical pattern later in
KeywordSpotter::Decode(const OfflineStream *ss, int32_t n)that should be corrected the same way.
🧹 Nitpick comments (2)
python-api-examples/offline-zeroshot-tts.py (1)
13-30: Zipvoice example: lexicon flag and config wiring are consistent
- The usage example, download commands, and CLI invocation all use
--zipvoice-lexiconand point to.../lexicon.txtin the newdistill-int8-zh-en-emiliamodel tree (lines 13–30).add_zipvoice_args()defines--zipvoice-lexiconwith a matching help string (lines 73–77).OfflineTtsZipvoiceModelConfigis constructed withlexicon=args.zipvoice_lexicon, aligning the example script with the new API surface (lines 236–247).- Minor style nit: Ruff flags an ambiguous fullwidth comma in the docstring (RUF002). If you care about lint cleanliness, replace any
，with a standard ASCII,in comments/usage text.Functionally, this script matches the renamed Zipvoice field and updated model bundle paths.
Also applies to: 43-78, 234-247, 272-291
flutter/sherpa_onnx/lib/src/tts.dart (1)
195-252: Dart ZipVoice config:lexiconrename and FFI wiring look correct
OfflineTtsZipVoiceModelConfigcleanly introduceslexicon:
- Included in the ctor with a default of
''(lines 195–207).- Parsed from
json['lexicon']infromJson, emitted as'lexicon'intoJson, and shown intoString()(lines 209–241).- Stored as a
final String lexiconfield (lines 242–252).- The FFI bridge sets
c.ref.model.zipvoice.lexiconfromconfig.model.zipvoice.lexicon.toNativeUtf8()and later frees it withcalloc.free(c.ref.model.zipvoice.lexicon)(lines 404–409, 430–436, 455–458), mirroring how other lexicon fields (VITS, Matcha, Kokoro) are handled.This keeps the Dart config, JSON, and native interop in sync with the new
lexiconfield, with no apparent leaks or mismatches. Just be aware that old JSON using apinyinDictkey will no longer populate this field.Also applies to: 404-436, 430-458
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
flutter/sherpa_onnx/lib/src/sherpa_onnx_bindings.dart(1 hunks)flutter/sherpa_onnx/lib/src/tts.dart(7 hunks)python-api-examples/offline-zeroshot-tts.py(5 hunks)scripts/go/sherpa_onnx.go(3 hunks)sherpa-onnx/c-api/c-api.cc(2 hunks)sherpa-onnx/c-api/c-api.h(1 hunks)sherpa-onnx/c-api/cxx-api.cc(1 hunks)sherpa-onnx/c-api/cxx-api.h(1 hunks)sherpa-onnx/csrc/offline-tts-zipvoice-model-config.cc(0 hunks)sherpa-onnx/csrc/offline-tts-zipvoice-model-config.h(1 hunks)sherpa-onnx/python/csrc/offline-tts-zipvoice-model-config.cc(1 hunks)swift-api-examples/SherpaOnnx.swift(2 hunks)wasm/tts/sherpa-onnx-tts.js(4 hunks)wasm/tts/sherpa-onnx-wasm-main-tts.cc(1 hunks)
💤 Files with no reviewable changes (1)
- sherpa-onnx/csrc/offline-tts-zipvoice-model-config.cc
🧰 Additional context used
🧬 Code graph analysis (4)
sherpa-onnx/c-api/c-api.cc (1)
sherpa-onnx/csrc/offline-stream.cc (2)
r(257-257)r(257-257)
scripts/go/sherpa_onnx.go (3)
sherpa-onnx/c-api/cxx-api.h (1)
OfflineOmnilingualAsrCtcModelConfig(271-273)sherpa-onnx/csrc/lexicon.cc (4)
Lexicon(105-122)Lexicon(125-145)Lexicon(402-405)Lexicon(409-412)sherpa-onnx/csrc/lexicon.h (1)
Lexicon(20-62)
wasm/tts/sherpa-onnx-tts.js (1)
wasm/asr/sherpa-onnx-asr.js (17)
lexiconLen(374-374)n(99-99)n(133-133)n(157-157)n(173-173)n(189-189)n(647-647)n(678-678)n(695-695)n(712-712)n(729-729)n(746-746)n(763-763)n(785-785)n(829-829)n(876-877)n(921-921)
swift-api-examples/SherpaOnnx.swift (1)
sherpa-onnx/csrc/kokoro-multi-lang-lexicon.cc (2)
lexicon(470-481)lexicon(470-470)
🪛 Ruff (0.14.8)
python-api-examples/offline-zeroshot-tts.py
29-29: Docstring contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF002)
🔇 Additional comments (13)
wasm/tts/sherpa-onnx-wasm-main-tts.cc (1)
76-83: Zipvoice lexicon print matches renamed fieldThe new
lexiconline is consistent with other model configs and the underlyingzipvoice->lexiconfield; no issues.sherpa-onnx/c-api/c-api.cc (2)
706-713: ys_log_probs export is correctly guarded and ownedAllocating
r->ys_log_probsonly whenresult.ys_log_probsis non-empty and matchesr->count, and setting it tonullptrotherwise, lines up with the struct definition and destructor and avoids OOB or double-free.
1243-1256: Zipvoice lexicon wiring in OfflineTtsConfig is consistentMapping
config->model.zipvoice.lexiconintotts_config.model.zipvoice.lexiconwithSHERPA_ONNX_OR(..., "")matches the other TTS models’ lexicon handling and the C API struct layout.sherpa-onnx/python/csrc/offline-tts-zipvoice-model-config.cc (1)
18-36: Python binding now exposeslexiconcleanlyRenaming the ctor parameter and
.def_readwritemember tolexiconaligns this pybind surface with the underlying C++ config and other language bindings; no issues.sherpa-onnx/c-api/cxx-api.h (1)
429-441: C++ Zipvoice model config field rename is layout‑safe
OfflineTtsZipvoiceModelConfignow usesstd::string lexiconin the expected slot betweendata_dirand the float fields, matching the C API struct and other TTS configs. No ABI/layout concerns.sherpa-onnx/c-api/cxx-api.cc (1)
416-423: Zipvoice lexicon propagation into C API looks correctAssigning
config.model.zipvoice.lexicon.c_str()intoc.model.zipvoice.lexiconkeeps the C++ wrapper in sync with the C struct and new field name; the whole Zipvoice block is now consistently configured.sherpa-onnx/c-api/c-api.h (1)
1069-1080: C API Zipvoice struct now exposeslexiconconsistentlyAdding
const char *lexiconbetweendata_dirand the float fields matches the internal config and all bindings; struct size and field order remain compatible with prior layouts.flutter/sherpa_onnx/lib/src/sherpa_onnx_bindings.dart (1)
217-236: Dart FFI ZipVoice config stays layout‑aligned with C APIThe new
lexiconpointer is placed betweendataDirand the four@Float()fields, matching the nativeSherpaOnnxOfflineTtsZipvoiceModelConfiglayout; this keeps the struct ABI‑compatible while reflecting the renamed field.If these bindings are not auto‑generated, double‑check against
SherpaOnnxOfflineTtsZipvoiceModelConfiginc-api.hafter any future changes to avoid drift.scripts/go/sherpa_onnx.go (2)
474-505: Offline omnilingual ASR config is wired correctly end-to-endThe new
Omnilingual OfflineOmnilingualAsrCtcModelConfigfield is properly threaded through:
- Added to
OfflineModelConfig(line 487) so it’s part of the public Go config.newCOfflineRecognizerConfigpopulatesc.model_config.omnilingual.modelfromconfig.ModelConfig.Omnilingual.Model(line 591).freeCOfflineRecognizerConfigfreesc.model_config.omnilingual.modelsymmetrically (lines 745–748).This matches the existing patterns for other offline model configs and should be safe from leaks or missed initialization.
Also applies to: 547-592, 628-748
962-975: Zipvoicelexiconfield is correctly added to Go config and C bridgeFor the Zipvoice TTS model:
OfflineTtsZipvoiceModelConfiggains aLexicon stringfield with a clear comment (lines 962–974).NewOfflineTtsnow setsc.model.zipvoice.lexiconfromconfig.Model.Zipvoice.Lexiconand defersC.freeon the allocated C string (lines 1151–1152).This keeps the Go API aligned with the C++
OfflineTtsZipvoiceModelConfiglexicon field and follows the same allocation/free pattern as the existing VITS/Matcha/Kokoro lexicon fields. No correctness or lifetime issues apparent.Also applies to: 1151-1157
wasm/tts/sherpa-onnx-tts.js (1)
265-276: ZipVoice WASM config: lexicon field and struct layout look consistent
lexiconLenis computed and included in the total buffer sizen(lines 272, 275).- Lexicon is written after
dataDirinto the contiguous buffer, and the string offset is advanced accordingly (lines 299–301).- Pointer layout (
len = 10 * 4) correctly reflects 6 string pointers (tokens, textModel, flowMatchingModel, vocoder, dataDir, lexicon) followed by 4 floats (featScale, tShift, targetRMS, guidanceScale), with the lexicon pointer atptr + 20(lines 279–324).- Default
offlineTtsZipVoiceModelConfignow includeslexicon: ''so callers that omit it won’t hitundefinedaccess (lines 377–390).This matches the expected C struct shape for Zipvoice and should fix the previous mismatch without introducing alignment or bounds issues.
Also applies to: 299-301, 318-325, 377-390
sherpa-onnx/csrc/offline-tts-zipvoice-model-config.h (1)
15-47: C++ Zipvoice model config now cleanly exposeslexicon
- The struct member
lexicon(line 22) replaces the oldpinyin_dictand is included in both the ctor parameters and initializer list (lines 31–46).- Parameter ordering matches member ordering:
tokens, text_model, flow_matching_model, vocoder, data_dir, lexicon, feat_scale, t_shift, target_rms, guidance_scale, preserving ABI layout.- Float defaults in the ctor match the in-class member defaults, so existing call sites that relied on defaults remain valid.
The rename is internally consistent and should just require downstream users to access
lexiconinstead ofpinyin_dict.swift-api-examples/SherpaOnnx.swift (1)
930-954: Swift Zipvoice helper correctly switched tolexicon
- The helper’s signature now exposes
lexicon: String = ""instead of the previous pinyin dict parameter (line 936).- The C-bridge call forwards this via the named
lexicon:field (line 948), keeping the argument order unchanged for the other parameters.This keeps the Swift convenience API aligned with the C++/C
lexiconfield without introducing ABI or call-site issues.
1 participant
Summary by CodeRabbit
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.