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

Jump to bottom

Attempt to fix GTEST_DEFINE_STATIC_MUTEX_ missing-field-initializers gcc warning #433

Closed
GoogleCodeExporter opened this issue Jul 28, 2015 · 6 comments
Closed

Attempt to fix GTEST_DEFINE_STATIC_MUTEX_ missing-field-initializers gcc warning #433

GoogleCodeExporter opened this issue Jul 28, 2015 · 6 comments

Comments

@GoogleCodeExporter
Copy link
Contributor 

Currently, GTEST_DEFINE_STATIC_MUTEX_ is implemented as follows:


// Defines and statically (i.e. at link time) initializes a static mutex.
// The initialization list here does not explicitly initialize each field,
// instead relying on default initialization for the unspecified fields. In
// particular, the owner_ field (a pthread_t) is not explicitly initialized.
// This allows initialization to work whether pthread_t is a scalar or struct.
// The flag -Wmissing-field-initializers must not be specified for this to work.
# define GTEST_DEFINE_STATIC_MUTEX_(mutex) \
    ::testing::internal::MutexBase mutex = { PTHREAD_MUTEX_INITIALIZER, false }

1) I suggest to change the comment "The flag -Wmissing-field-initializers must 
not be specified for this to work." to "Use -Wno-missing-field-initializers to 
make GCC silent." since "-Wmissing-field-initializers" does not prevent gtest 
from successful build in any way (as far as -Werror is not supplied).
2) I think we should change the code the following way:
# define GTEST_DEFINE_STATIC_MUTEX_(mutex) \
    ::testing::internal::MutexBase mutex = { PTHREAD_MUTEX_INITIALIZER, false, pthread_t() }

Thus we are using the default constructor. It works either with scalar or 
struct types and fixes the GCC warning.

Original issue reported on code.google.com by gmark...@gmail.com on 13 May 2013 at 12:43
@GoogleCodeExporter
Copy link
Contributor Author 

Is there a reason why suggestion 2) could not be implemented?

Disabling warnings is a bad idea in my opinion.

Original comment by daCh...@gmail.com on 10 Apr 2014 at 8:27

@GoogleCodeExporter
Copy link
Contributor Author 

If pthread_t() does not work in all cases then I see no harm of using 
pthread_self() instead. That should provide valid value for pthread_t. Initial 
value doesn't really have a meaning in here as there is boolean has_owner_ that 
defines whether this value is valid or not.

Original comment by daCh...@gmail.com on 10 Apr 2014 at 8:41

@GoogleCodeExporter
Copy link
Contributor Author 

2) seems to work for me on linux and windows, thanks.

Adding -Wmissing-field-initializers is hard to do surgically, I'd rather not 
turn off any warnings.

Original comment by daniel.r...@gmail.com on 8 Oct 2014 at 10:33

@GoogleCodeExporter
Copy link
Contributor Author 

2) worked for me on OSX 10.9, Xcode 6.1.1

Original comment by ketilwri...@gmail.com on 11 Dec 2014 at 10:05

@plopresti
Copy link
Contributor

Option (2) is perfectly standards-compliant regardless of the underlying type of pthread_mutex_t. Could we get this applied for those of us who like to compile with -Wextra ?

Complete patch looks like this. It is pretty small, but would you prefer a pull request?

diff --git a/googletest/include/gtest/internal/gtest-port.h b/googletest/include/gtest/internal/gtest-port.h
index 141d457..2288fae 100644
--- a/googletest/include/gtest/internal/gtest-port.h
+++ b/googletest/include/gtest/internal/gtest-port.h
@@ -1968,13 +1968,8 @@ class MutexBase {
      extern ::testing::internal::MutexBase mutex

 // Defines and statically (i.e. at link time) initializes a static mutex.
-// The initialization list here does not explicitly initialize each field,
-// instead relying on default initialization for the unspecified fields. In
-// particular, the owner_ field (a pthread_t) is not explicitly initialized.
-// This allows initialization to work whether pthread_t is a scalar or struct.
-// The flag -Wmissing-field-initializers must not be specified for this to work.
 #  define GTEST_DEFINE_STATIC_MUTEX_(mutex) \
-     ::testing::internal::MutexBase mutex = { PTHREAD_MUTEX_INITIALIZER, false }
+     ::testing::internal::MutexBase mutex = { PTHREAD_MUTEX_INITIALIZER, false, pthread_t() }

 // The Mutex class can only be used for mutexes created at runtime. It
 // shares its API with MutexBase otherwise.

@plopresti
Copy link
Contributor

Created pull request #633

peterebden added a commit to peterebden/please that referenced this issue Aug 24, 2018
@peterebden
fix for google/googletest#433
e69ab47
peterebden added a commit to peterebden/please that referenced this issue Sep 4, 2018
@peterebden
fix for google/googletest#433
0e4ee1a
peterebden added a commit to thought-machine/please that referenced this issue Sep 4, 2018
@peterebden
Subrepo fixes (#403)
6ff3031 
* Setting up builtin subrepo mabob

* Correct BuildLabel::String a bit.

Unbelievably there's been a bug in there for like three bloody years. Fortunately nobody noticed...

* Turns out that wasn't what I wanted. This is more better.

* Update a bunch of libraries.

Am getting 504s off Github. Not sure what has triggered that (I don't *think* it's related to my connection) but seems worth updating.

* oops

* Look for BUILD files in correct places

* Just set State always so we don't have to do nil checks

* Fix up some config stuff

* Strip old stuff from build defs

* Temp, for testing; use local repo

* Add new_local_repository rule

* think this makes things a bit better

* Report better BUILD file locations

* jarcat reference needs to be absolute

* Adding a gtest example

* add a custom build file for it

* Revert "Temp, for testing; use local repo"

This reverts commit 4d86304.

* working on this test_x86 issue

* Cleaning up some overly verbose logging

* clean some things up

* more subrepo chaos

* move pleasings out of custom package (it's in the wrong place basically)

* cleanup

* fix for google/googletest#433

* Reverting the UnitTest++ changes for now
peterebden added a commit to thought-machine/please that referenced this issue Oct 8, 2018
@peterebden
Moves UnitTest++ main out of plz core (#382)
391cefa 
* Setting up builtin subrepo mabob

* Correct BuildLabel::String a bit.

Unbelievably there's been a bug in there for like three bloody years. Fortunately nobody noticed...

* Turns out that wasn't what I wanted. This is more better.

* Update a bunch of libraries.

Am getting 504s off Github. Not sure what has triggered that (I don't *think* it's related to my connection) but seems worth updating.

* oops

* Look for BUILD files in correct places

* Just set State always so we don't have to do nil checks

* Fix up some config stuff

* Strip old stuff from build defs

* Temp, for testing; use local repo

* Add new_local_repository rule

* think this makes things a bit better

* Report better BUILD file locations

* jarcat reference needs to be absolute

* Adding a gtest example

* add a custom build file for it

* Revert "Temp, for testing; use local repo"

This reverts commit 4d86304.

* working on this test_x86 issue

* Cleaning up some overly verbose logging

* clean some things up

* more subrepo chaos

* move pleasings out of custom package (it's in the wrong place basically)

* cleanup

* fix for google/googletest#433

* tweak coverage settings

* update comment

* Don't attach C++ dependency to C tests

For now there is no config property, they'll have to be done manually. We could easily introduce later though.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@plopresti @GoogleCodeExporter @gennadiycivil