-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add C++ stages/expressions and Constant value support #14588
Conversation
wu-hui
commented
Mar 17, 2025
•
edited
Loading
edited
- Add stages and expressions in C++
- Use FSTUserDataReader to read constants
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) 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 is currently in preview and 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 to provide feedback. |
Generated by 🚫 Danger |
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces C++ stages and expressions, along with constant value support using FSTUserDataReader. The changes touch several files, primarily in the Firestore/Source and Firestore/core directories. Overall, the changes seem well-structured and introduce important new functionality. However, there are several areas that could benefit from closer scrutiny and potential improvement.
Summary of Findings
- Redundant
isUserDataRead
checks: TheisUserDataRead
flag and associated logic inFIRConstantBridge
andFIRFunctionExprBridge
seem redundant. TheFSTUserDataReader
should handle the parsing and caching of values, eliminating the need for manual tracking. This could be simplified. - Missing Copyright Header: The new files
aggregate_expressions.h
,aggregate_expressions.cc
,ordering.h
, andordering.cc
are missing the standard copyright header. This should be added for consistency. - Potential Memory Leak in
encodeBlob
: InFSTUserDataReader.mm
, theencodeBlob
method copies the blob usingnanopb::CopyBytesArray
, but it's unclear if the originalByteString
's memory is properly managed. This could lead to a memory leak if not handled correctly. - Inconsistent use of
std::move
: There are some inconsistencies in the use ofstd::move
. For example, inWhere
class constructor instages.h
,std::move
is used forexpr_
, but not inCollectionSource
constructor. Consistent use ofstd::move
can improve performance.
Merge Readiness
While the pull request introduces valuable functionality, the issues identified, particularly the potential memory leak and redundant logic, should be addressed before merging. I am unable to approve this pull request, and it should be reviewed and approved by others before merging. Addressing the high and medium severity issues is highly recommended before merging.
4e8c9b7
to
5127bd0
Compare
5127bd0
to
5d701f8
Compare