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

[clang-tidy] cppcoreguidelines-pro-type-member-init check flags anonymous union member #54748

Closed
paulaltin opened this issue Apr 5, 2022 · 8 comments

Comments

@paulaltin
Copy link
Contributor

Running clang-tidy on this code:

struct s {
    union {
        int a = 0;
        int b;
    };
};

results in the following warning:

warning: constructor does not initialize these fields: b [cppcoreguidelines-pro-type-member-init]

However, adding an initializer to b (e.g. int b = 0) results in a clang diagnostic:

error: initializing multiple members of union [clang-diagnostic-error]
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 5, 2022

@llvm/issue-subscribers-clang-tidy

@paulaltin paulaltin changed the title [clang-tidy] cppcoreguidelines-pro-type-member-init check flags union member [clang-tidy] cppcoreguidelines-pro-type-member-init check flags anonymous union member Apr 25, 2022
@paulaltin
Copy link
Contributor Author

The cppcoreguidelines-pro-type-member-init.cpp file has a test that ensures the check does not flag a union with only one member initialized:

and a test that ensures the check does flag members of anonymous unions when none are initialized:

(both authored by @alexfh).

@paulaltin
Copy link
Contributor Author

A similar issue was reported in 2017: #34232

At the time, @JonasToth suggested the bug might be related to this commit: llvm-mirror/clang-tools-extra@b7c157e

Since then, a commit by @Sockke was added to fix an apparently related issue: e490248

@dfrib
Copy link

dfrib commented May 25, 2022

The same warning is emitted also for when initializing one of the union members in a mem-init-list (as opposed to default mem initializers in the union), flagging e.g. the idiomatic std::optional backports for constexpr default-construction of disengaged optionals:

struct Optional {
    // warning: constructor does not initialize these fields: value_ 
    // [cppcoreguidelines-pro-type-member-init,hicpp-member-init]
    constexpr Optional() : dummy_{} {}
    union {
        char dummy_;
        int value_;
    };
};

@movie-travel-code
Copy link
Contributor

Btw, the review differential of the fix patched by @Sockke is https://reviews.llvm.org/D108370. IMHO, clang-tidy should be conservative when emitting the diagnostics, if the clang-tidy cannot afford the overhead or has no confidence to cover all the cases, it should give up emitting the warnings.

@Sockke
Copy link
Contributor

Sockke commented Jun 8, 2022

The latest patch is here: https://reviews.llvm.org/D127293, and there are still omissions in the processing of a union.

Sockke added a commit that referenced this issue Aug 25, 2022
…nitialized in cppcoreguidelines-pro-type-member-init

If a union member is initialized, the other members of the union are still suggested to be initialized in this check.  This patch fixes this behavior.
Reference issue: #54748

Reviewed By: njames93

Differential Revision: https://reviews.llvm.org/D127293
@Sockke
Copy link
Contributor

Sockke commented Aug 25, 2022

Fixed

@Sockke Sockke closed this as completed Aug 25, 2022
@paulaltin
Copy link
Contributor Author

Thanks for your work fixing this and getting it through review @Sockke!

movie-travel-code added a commit that referenced this issue Sep 2, 2022
…nitialized in cppcoreguidelines-pro-type-member-init

If a union member is initialized, other members are still recorded in the container to be initialized.  This patch fixes this behavior.
Reference issue: #54748

Differential Revision: https://reviews.llvm.org/D127293
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants