Refactor Pascal API - #3160
Refactor Pascal API#3160
Conversation
dosubot
Bot
added
the
size:L
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 focuses on refining the Pascal API for Sherpa-Onnx by introducing 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
|
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdded const qualifiers to several public array parameters and replaced element-wise sample copies with Move-based memory transfers; added nil/zero-length guards and minor input validation across resampling, waveform acceptance, denoising, diarization, TTS, and read/write wave helpers. Public API names remain unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![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 provides a nice refactoring of the Pascal API. The changes to use const for array parameters and replace manual copy loops with Move are great for performance. The added nil checks for pointers returned from the C-API improve the robustness of the code. I've found a couple of places where nil checks are still missing and could lead to runtime errors. Please see my detailed comments.
| P := SherpaOnnxVoiceActivityDetectorFront(Self.Handle); | ||
| Result.Start := P^.Start; |
There was a problem hiding this comment.
The pointer P returned from SherpaOnnxVoiceActivityDetectorFront can be nil, for example, if the VAD buffer is empty. Dereferencing it on the next line without a nil check will lead to an access violation. Please add a check for nil before using P.
P := SherpaOnnxVoiceActivityDetectorFront(Self.Handle);
if P = nil then
begin
Result := Default(TSherpaOnnxSpeechSegment);
Exit;
end;
Result.Start := P^.Start;
| P := SherpaOnnxLinearResamplerResample(Self.Handle, Samples, N, Ord(Flush)); | ||
| SetLength(Result, P^.N); |
There was a problem hiding this comment.
The pointer P returned from SherpaOnnxLinearResamplerResample might be nil on error. While the current C++ implementation seems to always return a valid pointer, it's safer and more consistent with other C-API wrappers in this file to add a nil check before dereferencing P.
P := SherpaOnnxLinearResamplerResample(Self.Handle, Samples, N, Ord(Flush));
if P = nil then
Exit;
SetLength(Result, P^.N);
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sherpa-onnx/pascal-api/sherpa_onnx.pas (2)
856-869:⚠️ Potential issue | 🟡 MinorMissing nil check on
P, inconsistent with other methods in this PR.All other methods refactored in this PR (
Generate,Run,ReadWave) now guard against a nil return from the C API before dereferencing. ThisResampleoverload dereferencesPon line 863 without checking, and would crash ifSherpaOnnxLinearResamplerResampleever returns nil.Proposed fix for consistency
Result := Default(TSherpaOnnxSamplesArray); P := SherpaOnnxLinearResamplerResample(Self.Handle, Samples, N, Ord(Flush)); + + if P = nil then + Exit; + SetLength(Result, P^.N); if P^.N > 0 then
2433-2445:⚠️ Potential issue | 🟠 MajorBounds validation does not reject negative
OffsetorN, allowing out-of-bounds memory access.The check on line 2435 only validates the upper bound. A negative
Offset(e.g.,Offset = -1, N = 1) satisfiesOffset + N <= Length(Samples)but causespcfloat(Samples) + Offsetto point before the array buffer. Similarly, a negativeNpasses the check and is forwarded to the C function.Proposed fix
procedure TSherpaOnnxVoiceActivityDetector.AcceptWaveform(const Samples: array of Single; Offset: Integer; N: Integer); begin - if Offset + N > Length(Samples) then + if (Offset < 0) or (N < 0) or (Offset + N > Length(Samples)) then begin WriteLn(Format('Invalid arguments!. Array length: %d, Offset: %d, N: %d', [Length(Samples), Offset, N]
There was a problem hiding this comment.
Pull request overview
This PR refactors the Pascal API surface in sherpa_onnx.pas to reduce unnecessary array copying and to speed up sample transfers between C pointers and Pascal dynamic arrays.
Changes:
- Marked several open-array
array of Singleparameters asconstto avoid implicit copying for waveform/sample inputs. - Replaced element-by-element sample copying loops with
Move(...)for faster bulk copies. - Added a few small guard checks (e.g.,
N <= 0,nilresults) before copying.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary by CodeRabbit