- 
                Notifications
    
You must be signed in to change notification settings  - Fork 38
 
Plugins Support for C/C++ #2353
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
Conversation
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.
Apart from the changes suggested, I think this looks good to me. Obviously, this is from my POV after having worked on this for a short time. Would be good to know how it fares for someone looking at it for the first time.
| 
           Thanks much @itsmesamster for the review. I've resolved the provided issue and added extra readme files for the templates with clearer guide for plugins development in C/C++  | 
    
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.
Looks good to me now.
415da34    to
    466e60f      
    Compare
  
    | global_log_level = general_configs->log_level; | ||
| if (global_log_level >= CHIPMUNK_SHARED_LOGGING_LEVEL_INFO) { | ||
| parse_string_t log_msg; | ||
| parse_string_dup(&log_msg, "Initi message called"); | 
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.
typo: "Initi"
| } | ||
| 
               | 
          ||
| // *** Demonstrate printing to stdout *** | ||
| std::cout << "Inint function called with log level: " | 
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.
typo: "Inint"
| break; | ||
| } | ||
| } | ||
| 
               | 
          
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.
- err is not used.
 - exports_chipmunk_parser_parser_list_config_item_free needs to be called to free the input parameter: plugin_configs
 
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.
Thanks a lot for the hint of freeing the items. I've added that in multiple items in both C and C++ templates!
- err is not used.
 
I think err here is the output in case this function fails. It will be None by default, and we need to change it only on errors.
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.
err is commented out and a useful note is added to the function docs
| 
               | 
          ||
| // *** Return length of provided bytes *** | ||
| ret->len = 1; | ||
| auto &item = ret->ptr[0]; | 
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.
It needs a new memory allocation for the pointer ret->ptr[0]
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.
I think the binding will allocate enough memory for one item by default, therefore there is no need to reallocate memory if case we have one item only. I've allocated memory in scenarios where we need to provide more than one item
| 
               | 
          ||
| std::string log_line = | ||
| "The length of provided bytes: " + std::to_string(data->len); | ||
| parse_string_set(&item.value.val.val.message.val.line, log_line.c_str()); | 
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.
I think we need to use dynamically allocated string so we do not need to provide custom *_post_return function:
https://github.com/bytecodealliance/wit-bindgen/blob/main/crates/c/README.md#:~:text=Importantly%2C%20the%20generated,by%20the%20linker.
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.
Great catch, Thanks! It's weird that this function didn't error when we run the plugin.
I've fixed that by using parse_string_dup() instead which will duplicate the string.
| 
               | 
          ||
| return true; | ||
| 
               | 
          ||
| #endif | 
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.
- err is not used?
 - data needs to be freed with : parse_list_u8_free
 
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.
Resolved freeing the data is resolved. That would have been a huge memory leak without this catch!
- err is not used?
 
As mentioned above, I think error should be used when we want to report an error only.
| 
           @ermankomurcu Thanks a lot for the valuable review! I've included some important fixes and added a section in the documentation about memory ownership with a link to the README on  Changes are all included, and it would be great to give a final review 🙏  | 
    
ac0c977    to
    3208332      
    Compare
  
    * Provide documentation about developing plugins in C/C++ * Template for developing parser plugin in C/C++ * Makefile for plugin template.
* Demonstrate basic logging functionality.
* Add plugins configuration sections to plugin template to show sending configs schemas and log their values in init function. * Add Render options to plugins, providing two modes for messages with single line or multiple columns.
C and C++ differ a lot in the build steps and each of them must have each own template.
* Change the directory structure of the plugins template for C language making the template more professional * Improve Makefile to adjust to the rules + Remove downloading reactor to it's own script file.
* Change the directory structure of the plugins template for C++ language making the template more professional * Improve Makefile to adjust to the rules + Remove downloading reactor to it's own script file.
* Group plugins templates & examples by their programming language since we are supporting rust and C/C++ * Update links in documentation accordingly
* Provide detailed documentation about plugins development with C/C++ providing the needed tools, explaining the templates and a step by step guide for development. * Document source code of plugins templates * README files for the new directory structure
* Provide separated instructions for templates * Separated README file for each template * Remove generated source files since they could confuse developers. * Extend documentation in source code * Include that C++ template will use C bindings and will not generate an abstraction layer on the top of them. * Rename documentation file
* Free memory from input items to function since the plugin owns their memory. * Always allocate memory on the input avoiding relaying on the allocated memory from the runtime because it's not guaranteed. * Explicitly point out that err pointer isn't used. * Extend documentation with memory ownership model. * Duplicate memory when cpp string is used with c_str() method. * Introduce pointers to avoid long nested structures in C template. * Extend documentation inside templates. * Fix typos. * Rearrange include statements.
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.
LGTM
This PR closes #2326
It provides:
Templates for parser plugin in C and C++
It provides to starter templates to develop plugins in both C and C++ with directory structure and
Makefilewhich will:wasi reactorif not provided by developersThe template provides starter implementation for parsers demonstrating the function provided by Chipmunk host and the functions needed from the plugins
Documentation for Plugin Development with C/C++
It provides detailed documentation for plugin development with C/C++ including:
Makefileswith examplesRefactoring
Plugins examples and templates are now grouped with the their programming language since we are supporting C and C++ besides rust now