Uh oh!
There was an error while loading. Please reload this page.
Fix gcc warnings in core_http_client.c. Update ci.yml. - #101
Fix gcc warnings in core_http_client.c. Update ci.yml.#101paulbartell wants to merge 16 commits into
Conversation
e7a0c2f to
6dc1426CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
abhidixi11
left a comment
There was a problem hiding this comment.
Unless there is absolute necessity, HTTP parser should not be updated. It should be also tested before updating.
Uh oh!
There was an error while loading. Please reload this page.
paulbartell
commented
Apr 28, 2021
The change itself is very minimal and is just to quiet a gcc warning. It is absolutely necessary to do so or we can't enable -Werror and will continue to miss compiler warnings. Here's one of the warnings generated by the library: |
f966279 to
a342513CompareUh oh!
There was an error while loading. Please reload this page.
| run: | | ||
| cd build/ | ||
| cd build | ||
| make coverage |
There was a problem hiding this comment.
(Minor) What's the purpose of building coverage target instead of just the unit test binaries with the all target when the next step in this job runs test for coverage?
There was a problem hiding this comment.
The all target does not run the unit tests, only the coverage target does.
There was a problem hiding this comment.
You don't need the coverage target to build and run the unit tests necessarily. The ctest command takes care of running the unit tests
Uh oh!
There was an error while loading. Please reload this page.
| sudo apt-get install -y cmake lcov | ||
| CFLAGS=" --coverage -O0 -Wall -Wextra" | ||
| CFLAGS+=" -Werror -Wno-error=pedantic" | ||
| CFLAGS+=" -D_FORTIFY_SOURCE=2" |
There was a problem hiding this comment.
Can you add a comment about what this configuration and its value does?
Uh oh!
There was an error while loading. Please reload this page.
| pBufferCur += fieldLen; | ||
| /* Copy the field separator, ": ", into the buffer. */ | ||
| ( void ) strncpy( pBufferCur, |
There was a problem hiding this comment.
Just leaving this here: This was the commit where we changed memcpy to strncpy to satisfy Coverity :
e0ebc00
Uh oh!
There was an error while loading. Please reload this page.
8fb5bac to
166cbe0CompareAlso enables -Werror and -Wno-error=pedantic so that new warnings cause a failed CI build. This includes FORTIFY_SOURCE warnings.
32b5837 to
4b1ff21Compare
Fix gcc warnings in http_parser and core_http_client.c.
Update ci.yml to use the latest ubuntu image and -Werror and -fsanitize.