search

flit / googletest

Automatically exported from code.google.com/p/googletest
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Fix g++ compiler warnings #132

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Compiling against Google Test 1.3.0 on g++ 4.1.2 (as provided by Debian
4.0) with -Wall -Wextra gives the following warnings:

/home/jkelley/local/include/gtest/internal/gtest-param-util.h:487: warning:
unused parameter ‘file’
/home/jkelley/local/include/gtest/internal/gtest-param-util.h:487: warning:
unused parameter ‘line’
/home/jkelley/local/include/gtest/internal/gtest-param-util.h:247: warning:
base class ‘class testing::internal::ParamIteratorInterface<int>’ should be
explicitly initialized in the copy constructor

Path available upon request.  (Since it's only 4 lines and the fixes are
obvious, I didn't know if I should do a full code review submission?)

Original issue reported on code.google.com by josh...@gmail.com on 23 Mar 2009 at 8:48

GoogleCodeExporter commented 9 years ago 
I understand the first two warnings but don't know what the last one is about.  
I
tried gcc 4.0.3 and it doesn't generate the last warning.  Could you please 
provide a
patch for review?  I already have a pending patch for the first two warnings, 
so your
patch only needs to cover the last one.  Thanks.

Original comment by zhanyong...@gmail.com on 24 Mar 2009 at 10:08

GoogleCodeExporter commented 9 years ago

Original comment by zhanyong...@gmail.com on 24 Mar 2009 at 10:17

GoogleCodeExporter commented 9 years ago 
g++ thinks that the default constructor of the base class should be explicitly 
invoked:

--- gtest-param-util.h.orig     2009-03-26 17:50:43.000000000 -0400
+++ gtest-param-util.h  2009-03-26 17:50:58.000000000 -0400
@@ -245,8 +245,8 @@

    private:
     Iterator(const Iterator& other)
-        : base_(other.base_), value_(other.value_), index_(other.index_),
-          step_(other.step_) {}
+        : ParamIteratorInterface<T>(), base_(other.base_),
+          value_(other.value_), index_(other.index_), step_(other.step_) {}

     const ParamGeneratorInterface<T>* const base_;
     T value_;

Original comment by josh...@gmail.com on 26 Mar 2009 at 9:51

GoogleCodeExporter commented 9 years ago 
Thanks.  Could you upload a patch to the code review tool?

Original comment by zhanyong...@gmail.com on 31 Mar 2009 at 7:39

GoogleCodeExporter commented 9 years ago 
Sorry - it has been a while and I forgot the status: is there any remaining 
work needed 
for this, Josh?

Original comment by zhanyong...@gmail.com on 5 Jun 2009 at 6:10

GoogleCodeExporter commented 9 years ago 
Sorry, I'd forgotten about this too.

For reasons beyond my understanding, I can no longer get gcc 4.1.2 to throw that
warning, and we've upgraded gcc since then.  I can upload the patch from 
comment 3 to
the code review tool if you'd still like to see it included, but it would no 
longer
affect me (or, I suspect, others), so whatever you want is fine.

Original comment by josh...@gmail.com on 11 Jun 2009 at 1:44

GoogleCodeExporter commented 9 years ago 
Closed as Josh indicated.

Original comment by zhanyong...@gmail.com on 19 Jun 2009 at 4:01

GoogleCodeExporter commented 9 years ago 
We get exactly this error when building Chrome with -Wextra.  Unfortunately, 
since this is a gtest header that 
is included by our tests, there is no workaround except to disable -Wextra for 
all of our tests.  :(

.../testing/gtest/include/gtest/internal/gtest-param-util.h: In copy 
constructor 
'testing::internal::RangeGenerator<T, IncrementT>::Iterator::Iterator(const 
testing::internal::RangeGenerator<T, IncrementT>::Iterator&) [with T = int, 
IncrementT = int]':
.../testing/gtest/include/gtest/internal/gtest-param-util.h:234:   instantiated 
from 
'testing::internal::ParamIteratorInterface<T>* 
testing::internal::RangeGenerator<T, 
IncrementT>::Iterator::Clone() const [with T = int, IncrementT = int]'
.../chrome/browser/sync/syncable/directory_backing_store_unittest.cc:685:   
instantiated from here
.../testing/gtest/include/gtest/internal/gtest-param-util.h:249:warning: base 
class 'class 
testing::internal::ParamIteratorInterface<int>' should be explicitly 
initialized in the copy constructor

I in fact had written same patch locally:

$ svn diff
Index: include/gtest/internal/gtest-param-util.h
===================================================================
--- include/gtest/internal/gtest-param-util.h   (revision 359)
+++ include/gtest/internal/gtest-param-util.h   (working copy)
@@ -247,7 +247,8 @@

    private:
     Iterator(const Iterator& other)
-        : base_(other.base_), value_(other.value_), index_(other.index_),
+        : ParamIteratorInterface<T>(other),
+          base_(other.base_), value_(other.value_), index_(other.index_),
           step_(other.step_) {}

     // No implementation - assignment is unsupported.

Would you like me to upload it to a code review system or is that sufficient?

Original comment by evan@chromium.org on 16 Mar 2010 at 5:47

GoogleCodeExporter commented 9 years ago 
BTW, I tried to see if I could just disable this warning, but I'm not sure if I 
can.  
I found this diff which doesn't look promising:

http://gcc.gnu.org/viewcvs/trunk/gcc/cp/init.c?r1=132324&r2=132323&pathrev=13232
4
(note the blanket test of "extra_warnings")

Original comment by evan@chromium.org on 16 Mar 2010 at 5:49

GoogleCodeExporter commented 9 years ago 
Bumps up the priority as it blocks chromium work.

I'm working on this.  Expect a patch soon.

Original comment by w...@google.com on 16 Mar 2010 at 6:44

GoogleCodeExporter commented 9 years ago 
Fixed in r394.

Original comment by w...@google.com on 17 Mar 2010 at 12:14