-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix function definition regex bug #6547
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
Fix function definition regex bug #6547
Conversation
The whitespace fix to this regex last week triggered another bug in libmodem. They have a typedef that the regex parsed as a function definition because it accepted too many character in the argument list of a function. This fix updates the regex to only allow characters you would expect in an argument list of a function def. Characters expected in the argument list of a function def: \w A-Z a-z 0-9 and _ \s All whitespace , Argument separator * Pointers \. Variadic argument \[ Array start \] Array end Signed-off-by: Gregers Gram Rygg <[email protected]>
0a21194 to
d3fdbd0
Compare
|
You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds. Note: This comment is automatically posted by the Documentation Publishing GitHub Action. |
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 @gregersrygg, mocks are generated correctly in libmodem now.
I wonder if the CI on this PR runs any unit tests though, we should make sure it does.
|
@nordic-krch , @tejlmand please have a look. |
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.
Could you please add a test for your new regex here with an example of what you're fixing:
https://github.com/nrfconnect/sdk-nrf/blob/main/tests/unity/example_test/src/foo/foo.h
feel free to add such tests in other files under https://github.com/nrfconnect/sdk-nrf/blob/main/tests/unity/example_test if more appropriate.
Please also add a test for the previous fix you made here: #6478
|
@tejlmand Yes, I noticed the example_test while going through previous commits on some of the files I touched. I also felt the need for more testing on this functionality. However I didn't want to clutter the example_test with more tests, so I separated the wrap tests from the example_test. |
Perfectly fine. Just wanted to ensure to get some test on this, so thanks for improving testing. |
0e042c6 to
42e3f1a
Compare
|
I think it is better to write a README or some documentation explaining what |
|
I also made a small experiment on using pycparser to parse the C code instead of using regexes. However it relies on the C preprocessor to run first (for |
8488cec to
806486a
Compare
This PR only removes code from the example_test, so I don't understand why I would then have to write documentation for for it as part of this bugfix/cleanup. There's also end user documentation for the example_test on our documentation site. |
347f519 to
2a4afe8
Compare
2a4afe8 to
fc75a17
Compare
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.
Approved under the expectation that snippets.c/h will be renamed according to discussions.
fc75a17 to
b1e117d
Compare
The python scripts to prefix functions with __wrap was tested using the example_test. This test is mean as an example of how to do unit testing. Moved the existing wrap tests and some new ones to a new test suite to avoid adding more irrelevant tests to the example test. Signed-off-by: Gregers Gram Rygg <[email protected]>
b1e117d to
12fc8ed
Compare
|
The compliance check that fail is because of intentional syntax in
|
|
@carlescufi or @mbolivar-nordic this is good to merge. Compliance failures are expected, as test code need to break formatting to test that the scripts works also on 3rd party code that are not following our coding guideline (and hence our test headers replicating that formatting break our guidelines) |
The whitespace fix to this regex last week triggered another bug in
libmodem. They have a typedef that the regex parsed as a function
definition because it accepted too many character in the argument
list of a function. This fix updates the regex to only allow
characters you would expect in an argument list of a function def.
Characters expected in the argument list of a function def: