-
Notifications
You must be signed in to change notification settings - Fork 68
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 all gcc-13 -Wmaybe-uninitialized warnings #119
base: develop
Are you sure you want to change the base?
Conversation
Thanks for the PR. In the past I consistently rejected such pessimizaitons. The docs have a section on this false positive: https://www.boost.org/doc/libs/1_84_0/libs/optional/doc/html/boost_optional/tutorial/gotchas/false_positive_with__wmaybe_uninitialized.html When I look at the test matrix, which uses gcc 13, I cannot see this watning in the test suite. Is it possible to provide a small Compiler Explorer example that demonstrates the warning? |
I'm having trouble parsing this but I'm assuming this is referring to the addition of the constructor/destructor to the The union here shouldn't be initializing the actual wrapped T, this is just communicating to the abstract machine that it's safe to copy.
Which test matrix is this? I didn't see gcc-13 in the CI. To recreate locally, all I did was: ./b2 libs/optional/test warnings=pedantic cxxflags="-Werror=maybe-uninitialized" variant=release |
@@ -39,7 +39,7 @@ class tc_optional_base : public optional_tag | |||
|
|||
tc_optional_base ( none_t ) | |||
: | |||
m_initialized(false) {} | |||
m_initialized(false), m_storage() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a write that will never be read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I understand now. I thought you were still using the union
here as a form of aligned_storage.
There is a write here because m_storage
is actually a T
, not a union { T t_ }
You know what this means? It means that the maybe-uninitialized
warning truly is genuine then!
If an optional is constructed with none and then copied, we're technically copying uninitialized values.
My first instinct would be to simply update this code to store a union
but I'm not sure what the goals of this trivially copyable base specialization is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have an idea:
struct zst{};
union tc_storage { T t_; zst z_; };
private :
bool m_initialized ;
tc_storage m_storage ;
Then whenever we need default constructibility, we just do: m_storage.z_={};
Exactly like this:
tc_optional_base ( none_t )
:
m_initialized(false) {
m_storage.z_={};
}
For some background, the goal of the trivially copyable base is to make |
I am answering an earlier quesiton. I do not have gcc 13 on my machine. The only place where I can test it is either Boost test matrix: Or Compiler Explorer: And they do not confirm this. A Compiler Explorer link would be helpful here. Or any link, for that matter. |
Ok, I can see similar warnings in gcc 12. |
Ok, so I pushed a commit with most of your changes: e31cf6f |
Awesome! I still wanna update your CI. I had most of a working prototype there but I hit issues with the install command and the package list. Out of curiosity, why do we need libpython-dev? That package in particular seemed to make the most trouble in the later containers that we'd need for latest gcc. |
I would be most grateful for that. Maybe do it via a separate PR?
I am not acquainted with the new CI mechanisms (I rely on the Boost Test Matrix). Kind people, like yourself, file CI-related PRs and I usually accept them without understanding. It is likely that libpython-dev may not be needed. |
The test suite generates quite a few
-Wmaybe-uninitialized
warnings with gcc-13.I've applied judicious uses of launder() to fix these.
I had to update the union to use an explicit constructor,destructor so that I could call it from the
none
constructor overload which also silenced another uninitialized warning in the code.