Skip to content
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

Make GitHub Actions enforce clang-tidy clean code + address current clang-tidy warnings #798

Merged
merged 5 commits into from
Jan 12, 2024

Conversation

hartwork
Copy link
Member

No description provided.

@hartwork hartwork added this to the 2.6.0 milestone Jan 12, 2024
clang-tidy output was:
> [..]/examples/element_declarations.c:163:16: warning: Potential leak of memory pointed to by 'stackTop' [clang-analyzer-unix.Malloc]
>   163 |         return false;
>       |                ^
@@ -135,6 +135,8 @@ getXMLCharset(const char *buf, char *charset) {
if (0) {
if (! matchkey(p, next, "xml") && charset[0] == '\0')
return;
} else {
(void)p;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an UNUSED_P() macro which is already used in xmlwf.c, so could also be used here.

...however, it's defined in lib/internal.h, which is not very nice to include from outside the lib. xmlwf.c says /* for UNUSED_P only */ at the include, but it's easy to miss/forget.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Snild-Sony thanks for having a look! I should note that xmlmime.c is one of the unused files that #119 is about, it is not compiled by either of the two build systems today. Let me revert this change and point to #119 right above the list of excluded files in script apply-clang-tidy.sh where I'll add xmlmime.c for exclusion also. The original idea was to keep the exclusion list as short as possible (and this small change allowed the list to be shorter) but if xmlmime.c does not even see a compiler and its warnings, maybe it should be excluded from clang-tidy instead until #119 finds some decision.

…NullParamChecker

clang-tidy output was:
> [..]/tests/acc_tests.c:368:9: warning: Null pointer passed to 1st parameter expecting 'nonnull' [clang-analyzer-core.NonNullParamChecker]
>   368 |     if (strlen(printable) < (size_t)1)
>       |         ^      ~~~~~~~~~

Note: It was harmless because fail(..) right before catches that case.
…ullDereference

clang-tidy output was:
> [..]/tests/basic_tests.c:2083:19: warning: Dereference of null pointer [clang-analyzer-core.NullDereference]
>  2083 |   errorFlags |= ((model[0].type == XML_CTYPE_SEQ) ? 0 : (1u << 2));
>       |                   ^~~~~~~~~~~~~
@hartwork hartwork merged commit 7664ecd into master Jan 12, 2024
68 checks passed
@hartwork hartwork deleted the clang-tidy branch January 12, 2024 20:45
@hartwork hartwork mentioned this pull request Jan 12, 2024
35 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants