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

tool_doswin: Fix uninitialized field warning #3254

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@danielgustafsson
Member

danielgustafsson commented Nov 9, 2018

The partial struct initialization in 397664a caused a warning in the buildfarm on uninitialized MODULEENTRY32 struct members:

  /src/tool_doswin.c:681:3: warning: missing initializer for field 'th32ModuleID' of 'MODULEENTRY32 {aka struct tagMODULEENTRY32}' [-Wmissing-field-initializers]

This is sort of a bogus warning as the remaining members will be set to zero by the compiler, as all omitted members are. Nevertheless, remove the warning by omitting all members and setting the dwSize member explicitly. (Setting th32ModuleID to 1 as it's documented value is will only shift the warning to complain about the next field, so we need to do all or nothing.)

@jay does this seem reasonable to you?

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Nov 9, 2018

Aren't empty initializers C++? I'm mainly a C++ developer, so I'm not 100 % sure, but I thought the universal zero initializer was {} for C++ and {0} for C.

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Nov 9, 2018

tool_doswin: Fix uninitialized field warning
The partial struct initialization in 397664a caused a
warning on uninitialized MODULEENTRY32 struct members:

  /src/tool_doswin.c:681:3: warning: missing initializer for field
  'th32ModuleID' of 'MODULEENTRY32 {aka struct tagMODULEENTRY32}'
  [-Wmissing-field-initializers]

This is sort of a bogus warning as the remaining members will be set
to zero by the compiler, as all omitted members are. Nevertheless,
remove the warning by omitting all members and setting the dwSize
members explicitly.

Closes #xxxx

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-struct_init branch from d686fa0 to 6b6b653 Nov 9, 2018

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Nov 9, 2018

You might be right, I don’t have a copy of the standard handy right now to check.

Following up on this now that I had more time, {} is a GNU extension, {0} is the correct zero initializer. Patch updated accordingly

@bagder

This comment has been minimized.

Member

bagder commented Nov 10, 2018

silly appveyor-flaky test =(

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Nov 14, 2018

@MarcelRaad are you happy with this patch to go in? Would be nice to get back to a warning free windows cross-compilation.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Nov 14, 2018

I'm not 100 % sure if changing the value to 0 makes the warning go away, I'd hope it does by now - old GCC versions still warned on this IIRC. And unfortunately, I don't have access to my development environment right now. But the change looks good to me.

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Nov 18, 2018

Unless there are objections I propose to push this later today to try and squash the warnings in the autobuilds.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Nov 18, 2018

Just tried with https://godbolt.org/z/oeVJU-, seems to work in GCC 4.7 and later! In case it unexpectedly doesn't, using memset instead of aggregate initialization is also an option.

@jay

jay approved these changes Nov 18, 2018

The proposed change is fine and AFAIK still technically partial initialization, which is completely fine and defined behavior though they don't call it that in the standard. An object is either initialized or it's not. The standard basically says if there are less values than elements then the remaining are 0. struct foo bar = {0} is partial initialization but is so common it won't throw a warning as Marcel has shown. Any other value would probably cause it if extra warnings are enabled, which I didn't know when I sent it upstream since the CI passed.

@jay

This comment has been minimized.

Member

jay commented Nov 18, 2018

Aren't empty initializers C++? I'm mainly a C++ developer, so I'm not 100 % sure, but I thought the universal zero initializer was {} for C++ and {0} for C.

the chosen answer of this question has a good breakdown: https://stackoverflow.com/questions/10828294/c-and-c-partial-initialization-of-automatic-structure

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Nov 18, 2018

Thanks for the reviews! The windows crosscompilation autobuilds should have two warnings less tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment