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

Fix redundant cast-to-int when INI_USE_STACK!=0 #137

Merged
merged 1 commit into from
Jun 29, 2022
Merged

Fix redundant cast-to-int when INI_USE_STACK!=0 #137

merged 1 commit into from
Jun 29, 2022

Conversation

bostjan
Copy link
Contributor

@bostjan bostjan commented Jun 26, 2022

Preamble:
@benhoyt, I understand that this PR is borderline bikeshedding, so feel free to reject it. I mainly wanted to make sure to bring this to your attention, since it might be important to you for other reasons (i.e. other users that are using SonarQube/SonarCloud too may see this annoyance too).


Code context (ini.c):

    101: #if INI_USE_STACK
    102:     char line[INI_MAX_LINE];
    103:     int max_line = INI_MAX_LINE;
    104: #else
    105:     char* line;
    106:     size_t max_line = INI_INITIAL_ALLOC;
    107: #endif
    ...
    133: while (reader(line, (int)max_line, stream) != NULL) {

SonarCloud is reporting a "code smell" due to a redundant cast to int at line 133.
This only happens when INI_USE_STACK is true, and not otherwise, as in that case
size_t gets correctly casted to int.

This patch makes the usage of type cast to int depend on the INI_USE_STACK too,
and skips the unnecessary type cast when max_line is already defined as int.

The original SonarCloud report URL:
https://sonarcloud.io/project/issues?pullRequest=221&issues=AYGYdhPZKkhmUWy1nsvP&open=AYGYdhPZKkhmUWy1nsvP&id=snoopy

@benhoyt
Copy link
Owner

benhoyt commented Jun 26, 2022

Hi @bostjan, thanks for this. I'm not that enthusiastic about the fix as is, because it adds complication to silence a (false positive-ish) tool warning ... however, what about the following fix -- does that work for your tool?

diff --git a/ini.c b/ini.c
index f8a3ea3..8d3b597 100644
--- a/ini.c
+++ b/ini.c
@@ -100,7 +100,7 @@ int ini_parse_stream(ini_reader reader, void* stream, ini_handler handler,
     /* Uses a fair bit of stack (use heap instead if you need to) */
 #if INI_USE_STACK
     char line[INI_MAX_LINE];
-    int max_line = INI_MAX_LINE;
+    size_t max_line = INI_MAX_LINE;
 #else
     char* line;
     size_t max_line = INI_INITIAL_ALLOC;

Code context (ini.c):

    101: #if INI_USE_STACK
    102:     char line[INI_MAX_LINE];
    103:     int max_line = INI_MAX_LINE;
    104: #else
    105:     char* line;
    106:     size_t max_line = INI_INITIAL_ALLOC;
    107: #endif
    ...
    133: while (reader(line, (int)max_line, stream) != NULL) {

SonarCloud is reporting a "code smell" due to a redundant cast to `int` at line
133. This only happens when INI_USE_STACK is true, and not otherwise, as in that
case, `max_line` is of type `size_t` that gets correctly casted to `int`.

This patch changes the type of `max_line` variable to `size_t` for all situations.

SonarCloud report URL:
https://sonarcloud.io/project/issues?pullRequest=221&issues=AYGYdhPZKkhmUWy1nsvP&open=AYGYdhPZKkhmUWy1nsvP&id=snoopy

Co-authored-by: Ben Hoyt <[email protected]>
@bostjan
Copy link
Contributor Author

bostjan commented Jun 29, 2022

But of course it works like, @benhoyt :)

Clearly your version is more elegant. Not sure why I didn't want to delve into a does that max_size _need_ to be an int with INI_USE_STACK==0? question initially.

Results from a new scan with your patch applied:
https://sonarcloud.io/project/issues?id=snoopy&pullRequest=222&resolved=false&types=CODE_SMELL

I've changed the PR to your suggestion (and added you as a coauthor of the change). Thanks for your time!

@benhoyt benhoyt merged commit 0566527 into benhoyt:master Jun 29, 2022
@bostjan bostjan deleted the fix-sonarcloud-code-smell branch June 29, 2022 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants