Conversation
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 C function declarations within the regex library to adhere to contemporary ANSI C standards. The primary goal is to improve code maintainability and ensure compatibility with modern compilers, preventing potential build failures or warnings related to deprecated syntax. This change systematically updates function signatures to include parameter types directly in the function declaration, moving away from the older K&R style. 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
|
There was a problem hiding this comment.
Code Review
This pull request is a great initiative to modernize the codebase by converting old-style K&R C function definitions to ANSI C prototypes. The changes across the various files in the regex directory are largely correct and improve compatibility with modern compilers. I've identified a few areas in glob.c where function signature formatting is inconsistent, leading to long lines that are hard to read. My review comments provide specific suggestions to address these for better code consistency and readability. Overall, a valuable and well-executed refactoring.
Unit Test Results9 525 tests 8 933 ✅ 52m 31s ⏱️ Results for commit 176a6a4. ♻️ This comment has been updated with latest results. |
julianuziemblo
left a comment
There was a problem hiding this comment.
Small nits. I'd consider adding a (nearly) empty .clang-format file to this directory so that the CI doesn't shout at us every time we have make fixes here. Other than those, LGTM.
7dcd7d4 to
f89df4a
Compare
We use a regex implementation from one of BSD's, and it contains old-style definitions which are being slowly dropped from newer compilers. JIRA: RTOS-1233
We use BSD implementation which does not adhere to our coding style. JIRA: RTOS-1233
b94e44b to
176a6a4
Compare
| static int g_lstat(Char *fn, struct stat *sb, glob_t *pglob); | ||
| static DIR *g_opendir(Char *str, glob_t *pglob); | ||
| static Char *g_strchr(Char *str, int ch); | ||
| /* static Char *g_strcat(Char *, const Char *); */ |
There was a problem hiding this comment.
Do we need to keep this? Was there any deeper meaning to previous #ifdef notdef of this declaration other than that it was not defined?
There was a problem hiding this comment.
We don't and there wasn't.
I'm just thinking that it's better not to deviate much from the original code, in case we want to pull changes from upstream implementation.
There was a problem hiding this comment.
If we don't want to deviate, then maybe let's revert to the original?
Quick look suggests that the notdef seems to be a common pattern in FreeBSD source used instead of if 0:
We use a regex implementation from one of BSD's, and it contains old-style definitions which are being slowly dropped from newer compilers.
JIRA: RTOS-1233
Types of changes
How Has This Been Tested?
Checklist:
Special treatment