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

Overview: initialize color variables #916

Merged
merged 1 commit into from
Oct 11, 2019

Conversation

eht16
Copy link
Member

@eht16 eht16 commented Oct 3, 2019

While this might not be necessary technically, cppcheck complained:
overviewscintilla.c:800:53: note: Calling function 'overview_scintilla_get_overlay_color', 2nd argument '&color' value is
overview_scintilla_get_overlay_color (self, &color);
^
overviewscintilla.c:1146:11: note: Uninitialized variable: color

While this might not be necessary technically, cppcheck complained:
overviewscintilla.c:800:53: note: Calling function 'overview_scintilla_get_overlay_color', 2nd argument '&color' value is <Uninit>
        overview_scintilla_get_overlay_color (self, &color);
                                                    ^
overviewscintilla.c:1146:11: note: Uninitialized variable: color
@eht16
Copy link
Member Author

eht16 commented Oct 3, 2019

Newer cppcheck versions (probably 1.89 which is in Debian Sid since a few days) complained about it and so the nightly builds broke.

@codebrainz
Copy link
Member

Maybe it's a bug in cppcheck?

@frlan
Copy link
Member

frlan commented Oct 3, 2019

But or not I think setting a variable to a default is a good thing.

@elextr
Copy link
Member

elextr commented Oct 3, 2019

It would appear to be a correct warning, there is nothing to tell cppcheck that argument 2 is an out only argument so it has to assume the value is used in the function and hence an uninitialised value is being passed.

@codebrainz
Copy link
Member

But or not I think setting a variable to a default is a good thing.

I haven't done a proper survey, but I'd wager if we did, we'd find hundreds if not thousands of uninitialized variables in Geany and GP's code.

there is nothing to tell cppcheck that argument 2 is an out

If it did proper analysis (ie. look into callee) it would see it's passed to memcpy and fully written.

In any case, I'm fine with this PR if it shuts up cppcheck; this isn't performance critical in anyway, and it doesn't affect readability.

@elextr
Copy link
Member

elextr commented Oct 4, 2019

If it did proper analysis (ie. look into callee) it would see it's passed to memcpy and fully written.

  1. "if" :)

  2. and if it did, the return_ifs can shortcut return and leave it uninitialised.

@eht16
Copy link
Member Author

eht16 commented Oct 6, 2019

I don't mind much.
If you think it's a false-positive in cppcheck, then you should report it (or comment on https://trac.cppcheck.net/ticket/9347, it looks close to our case).

@lpaulsen93
Copy link
Contributor

In any case, I'm fine with this PR if it shuts up cppcheck; this isn't performance critical in anyway, and it doesn't affect readability.

I agree to this and vote for merging it.

@codebrainz
Copy link
Member

It looks like maybe it was fixed in danmar/cppcheck#2167, but I haven't tested it.

@b4n
Copy link
Member

b4n commented Oct 6, 2019

  1. and if it did, the return_ifs can shortcut return and leave it uninitialised.

@elextr got a point, it's actually technically correct to say the value may not be initialized because of those checks, and it seem impossible for a tool to infer that those checks will succeed, even if we know they will -- unless something else went terribly bad and we're fubar.

So although I initially blamed that on cppcheck, I guess it's fair and this PR makes sense.

@codebrainz
Copy link
Member

it's actually technically correct to say the value may not be initialized because of those checks

Depending on whether they're actually enabled, of course.

@frlan frlan merged commit 0bfe89d into geany:master Oct 11, 2019
@eht16 eht16 deleted the overview_fix_cppcheck branch August 23, 2020 15:17
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.

6 participants