fix: docs stmt2 demo #34531
fix: docs stmt2 demo #34531
Conversation
Pengrongkun
requested review from
a team,
dapan1121,
guanshengliang,
hzcheng and
zitsen
as code owners
Summary of ChangesHello @Pengrongkun, 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 refines the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
There was a problem hiding this comment.
Pull request overview
This pull request fixes the stmt2 demo and improves error messages for better debugging. The primary focus is correcting the TAOS_STMT2_BIND usage in the demo file and renaming a function for consistency.
Changes:
- Fixed stmt2 demo to correctly use NULL for length field on fixed-length data types (INT, FLOAT, TIMESTAMP)
- Renamed error string function from
stmtErrstr2tostmt2Errstrfor naming consistency - Enhanced error messages in parameter binding to include type information
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/cases/83-DocTest/c.sh | Enabled database cleanup for stmt2_insert_demo test to ensure clean test environment |
| source/libs/parser/src/parInsertStmt.c | Improved error messages to include column and buffer type information for easier debugging |
| source/client/src/clientStmt2.c | Renamed function from stmtErrstr2 to stmt2Errstr |
| source/client/src/clientMain.c | Updated all references to use stmt2Errstr and improved error logging |
| source/client/inc/clientStmt2.h | Updated function declaration to stmt2Errstr |
| docs/examples/c/stmt2_insert_demo.c | Fixed BIND structure usage, removed unnecessary length allocations for fixed types, added comprehensive error checking and memory management |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 significantly improves the stmt2_insert_demo.c example by adding robust error handling for memory allocations, using safer string functions (snprintf), and simplifying data binding. The changes make the example more reliable and a better reference for users. Additionally, error messages in other parts of the codebase have been made more descriptive, which is a great enhancement for debugging.
I've found one area for improvement in the new freeBindData function, which has a potential memory leak and overly complex logic. My suggestion simplifies the function and resolves the issue.
| static void freeBindData(char ***table_name, TAOS_STMT2_BIND ***tags, TAOS_STMT2_BIND ***params) { | ||
| if (table_name == NULL || *table_name == NULL) return; | ||
| if (tags == NULL || *tags == NULL) { | ||
| free(*table_name); | ||
| *table_name = NULL; | ||
| return; | ||
| } | ||
| if (params == NULL || *params == NULL) { | ||
| for (int i = 0; i < NUM_OF_SUB_TABLES; i++) { | ||
| free((*table_name)[i]); | ||
| if ((*tags)[i] != NULL) { | ||
| free((*tags)[i][0].buffer); | ||
| free((*tags)[i][1].buffer); | ||
| free((*tags)[i][1].length); | ||
| free((*tags)[i]); | ||
| } | ||
| } | ||
| free(*table_name); | ||
| free(*tags); | ||
| *table_name = NULL; | ||
| *tags = NULL; | ||
| return; | ||
| } | ||
| for (int i = 0; i < NUM_OF_SUB_TABLES; i++) { | ||
| free((*table_name)[i]); | ||
| for (int j = 0; j < 2; j++) { | ||
| free((*tags)[i][j].buffer); | ||
| free((*tags)[i][j].length); | ||
| if ((*tags)[i] != NULL) { | ||
| free((*tags)[i][0].buffer); | ||
| free((*tags)[i][1].buffer); | ||
| free((*tags)[i][1].length); | ||
| free((*tags)[i]); | ||
| } | ||
| free((*tags)[i]); | ||
|
|
||
| for (int j = 0; j < 4; j++) { | ||
| free((*params)[i][j].buffer); | ||
| free((*params)[i][j].length); | ||
| if ((*params)[i] != NULL) { | ||
| for (int j = 0; j < 4; j++) { | ||
| free((*params)[i][j].buffer); | ||
| free((*params)[i][j].length); | ||
| } | ||
| free((*params)[i]); | ||
| } | ||
| free((*params)[i]); | ||
| } | ||
| free(*table_name); | ||
| free(*tags); | ||
| free(*params); | ||
| *table_name = NULL; | ||
| *tags = NULL; | ||
| *params = NULL; | ||
| } |
There was a problem hiding this comment.
The new freeBindData function is a great addition for robust cleanup. However, the current implementation is overly complex, contains duplicated code, and has a potential memory leak.
Specifically:
- If
tags == NULLbutparams != NULL, the function will free*table_nameandreturnearly, leaking all memory associated withparams. - The logic block from lines 200-215 is largely a repetition of the main cleanup loop that follows, which makes the code harder to maintain.
I suggest refactoring this function to be simpler and more robust by removing the early returns and duplicated code. The revised version below handles all cases correctly and is easier to read.
static void freeBindData(char ***table_name, TAOS_STMT2_BIND ***tags, TAOS_STMT2_BIND ***params) {
if (table_name == NULL || *table_name == NULL) {
return;
}
for (int i = 0; i < NUM_OF_SUB_TABLES; i++) {
free((*table_name)[i]);
if (tags && *tags && (*tags)[i]) {
free((*tags)[i][0].buffer);
free((*tags)[i][1].buffer);
free((*tags)[i][1].length);
free((*tags)[i]);
}
if (params && *params && (*params)[i]) {
for (int j = 0; j < 4; j++) {
free((*params)[i][j].buffer);
}
free((*params)[i]);
}
}
free(*table_name);
*table_name = NULL;
if (tags && *tags) {
free(*tags);
*tags = NULL;
}
if (params && *params) {
free(*params);
*params = NULL;
}
}
2 participants
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.