Refactor Dart API to check for nullptr. - #3329
Conversation
dosubot
Bot
added
the
size:XL
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis pull request adds defensive null-checks and initialization guards across 14 Dart binding files in the Flutter sherpa-onnx package, ensuring SherpaOnnxBindings is initialized and native pointers are valid before dereferencing them in methods like free, createStream, compute, and other lifecycle operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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, 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 refactors the Dart API by integrating robust null pointer checks and improved error handling mechanisms. The changes aim to enhance the stability and reliability of the 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 pull request refactors the Dart API to add checks for nullptr before using native pointers, which is a great improvement for robustness. However, the error handling for these null pointer checks is inconsistent across the codebase. In some cases, an exception is thrown, while in many others, the function fails silently by returning a default or empty value. This inconsistency can hide bugs and make debugging difficult. My review focuses on making this error handling consistent by throwing exceptions when an object is used after it has been freed. I've also pointed out opportunities to reduce code duplication for binding initialization checks and to use more specific exception messages.
| if (ptr == nullptr || stream.ptr == nullptr) { | ||
| return <AudioEvent>[]; | ||
| } |
There was a problem hiding this comment.
For consistency with other methods like createStream that throw exceptions on invalid state, this method should also throw an exception instead of returning an empty list. Using an object that has been freed is a programmer error and should be surfaced as an exception to avoid hiding potential bugs.
| if (ptr == nullptr || stream.ptr == nullptr) { | |
| return <AudioEvent>[]; | |
| } | |
| if (ptr == nullptr || stream.ptr == nullptr) { | |
| throw Exception('AudioTagging or stream has been freed and cannot be used.'); | |
| } | |
| if (ptr == nullptr || stream.ptr == nullptr) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Using a freed object (ptr or stream.ptr is nullptr) should result in an exception rather than a silent failure. This helps in identifying incorrect API usage early. Please throw an exception here for consistency with methods like createStream.
| if (ptr == nullptr || stream.ptr == nullptr) { | |
| return false; | |
| } | |
| if (ptr == nullptr || stream.ptr == nullptr) { | |
| throw Exception('KeywordSpotter or stream has been freed and cannot be used.'); | |
| } | |
| if (ptr == nullptr || stream.ptr == nullptr) { | ||
| return KeywordResult(keyword: ''); | ||
| } |
There was a problem hiding this comment.
Returning a default value when the object has been freed can hide bugs. It's better to throw an exception to signal that the object is in an invalid state and cannot be used.
| if (ptr == nullptr || stream.ptr == nullptr) { | |
| return KeywordResult(keyword: ''); | |
| } | |
| if (ptr == nullptr || stream.ptr == nullptr) { | |
| throw Exception('KeywordSpotter or stream has been freed and cannot be used.'); | |
| } | |
| if (ptr == nullptr || stream.ptr == nullptr) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This method should throw an exception if ptr or stream.ptr is nullptr, instead of returning silently. This ensures consistent error handling for invalid object states.
| if (ptr == nullptr || stream.ptr == nullptr) { | |
| return; | |
| } | |
| if (ptr == nullptr || stream.ptr == nullptr) { | |
| throw Exception('KeywordSpotter or stream has been freed and cannot be used.'); | |
| } | |
| if (ptr == nullptr || stream.ptr == nullptr) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
To maintain consistency in error handling, please throw an exception here if the object or stream has been freed, instead of failing silently.
| if (ptr == nullptr || stream.ptr == nullptr) { | |
| return; | |
| } | |
| if (ptr == nullptr || stream.ptr == nullptr) { | |
| throw Exception('KeywordSpotter or stream has been freed and cannot be used.'); | |
| } | |
| if (ptr == nullptr) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
For consistency and robustness, please throw an exception here if the VAD is used after being freed.
| if (ptr == nullptr) { | |
| return; | |
| } | |
| if (ptr == nullptr) { | |
| throw Exception('VoiceActivityDetector has been freed and cannot be used.'); | |
| } | |
| if (ptr == nullptr) { | ||
| return SpeechSegment(samples: Float32List(0), start: 0); | ||
| } |
There was a problem hiding this comment.
Returning a default SpeechSegment for a freed VAD instance can hide bugs. It's safer to throw an exception to signal an invalid state.
| if (ptr == nullptr) { | |
| return SpeechSegment(samples: Float32List(0), start: 0); | |
| } | |
| if (ptr == nullptr) { | |
| throw Exception('VoiceActivityDetector has been freed and cannot be used.'); | |
| } | |
| if (ptr == nullptr) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This method should throw an exception if the VAD has been freed, rather than returning silently. This helps in catching lifecycle management issues.
| if (ptr == nullptr) { | |
| return; | |
| } | |
| if (ptr == nullptr) { | |
| throw Exception('VoiceActivityDetector has been freed and cannot be used.'); | |
| } | |
| if (ptr == nullptr) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
For consistency and robustness, please throw an exception here if the VAD is used after being freed.
| if (ptr == nullptr) { | |
| return; | |
| } | |
| if (ptr == nullptr) { | |
| throw Exception('VoiceActivityDetector has been freed and cannot be used.'); | |
| } | |
| if (ptr == nullptr) { | ||
| throw Exception("Failed to create offline stream"); | ||
| } |
There was a problem hiding this comment.
The exception message "Failed to create offline stream" is also used on line 202, but these two checks handle different error conditions. Using distinct messages would improve clarity and make debugging easier.
For example, here the error is that the AudioTagging instance itself is invalid. A more specific message could be:
throw Exception("AudioTagging instance is not valid. Cannot create stream.");
And for the check on line 202, where the native call fails:
throw Exception("Native offline stream creation failed.");
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
Config-invalid (e.g. missing model files) makes native factory return null; first method call then SIGSEGVs. require(ptr != 0L) in all binding classes and streams, mirroring Java binding and Dart PR #3329. Co-authored-by: Anna Medonosova <anna.medonosova@gmail.com>
1 participant
Summary by CodeRabbit
Release Notes