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

tool_doswin: Fix uninitialized field warning #3254

Closed

Conversation

danielgustafsson
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

danielgustafsson commented Nov 9, 2018 via email

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
Copy link
Member Author

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
Copy link
Member

bagder commented Nov 10, 2018

silly appveyor-flaky test =(

@danielgustafsson
Copy link
Member Author

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

@MarcelRaad
Copy link
Member

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
Copy link
Member Author

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

@MarcelRaad
Copy link
Member

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.

Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

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
Copy link
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
Copy link
Member Author

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

@lock lock bot locked as resolved and limited conversation to collaborators Feb 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants