Compile protobuf with cxx on Tru64

With this patch, the project compile on Tru64 with cxx.

see http://code.google.com/p/protobuf/issues/detail?id=38 for more details.



Tested on : TRU64 v5.1B with the cxx compiler V6.5.

[kenton, 2008-09-19 20:40:58]

Thanks for the patch.  It's great to support more platforms.

I'm not sure I understand some of your changes.  In general, it would be nice if
you wrote comments explaining why certain changes are necessary.  I'm hoping
that some of these changes actually aren't necessary -- like replacing anonymous
namespaces with "namespace internal" -- since I think they harm the code
quality.  Specific comments below...

http://codereview.appspot.com/5886/diff/1/2
File README.txt (right):

http://codereview.appspot.com/5886/diff/1/2#newcode54
Line 54: For some reason the unittest_proto_middleman in the Makefiles cannot
work.
In what way does it not work?  Let's try to fix it instead of making people do
extra work.

http://codereview.appspot.com/5886/diff/1/3
File src/google/protobuf/compiler/cpp/cpp_unittest.cc (right):

http://codereview.appspot.com/5886/diff/1/3#newcode56
Line 56: namespace internal{
Why change this?  Surely tru64 supports anonymous namespaces?

http://codereview.appspot.com/5886/diff/1/5
File src/google/protobuf/descriptor.cc (right):

http://codereview.appspot.com/5886/diff/1/5#newcode1157
Line 1157: #if defined( __osf__ ) && !defined( __GNUC__ ) //static_cast on Label
does't work on Tru64
Why not?  Does reinterpret_cast work?  How about static_cast to int and then to
Label?  We try to avoid C-style casts.

http://codereview.appspot.com/5886/diff/1/8
File src/google/protobuf/message.h (right):

http://codereview.appspot.com/5886/diff/1/8#newcode100
Line 100: #include <istream>
You shouldn't have to #include <istream> or <ostream> after <iosfwd> was already
#included -- that's the whole point of <iosfwd>.  If tru64 requires this, please
place it in an #ifdef.

http://codereview.appspot.com/5886/diff/1/9
File src/google/protobuf/repeated_field.cc (right):

http://codereview.appspot.com/5886/diff/1/9#newcode28
Line 28: #if !(defined( __osf__ ) && !defined( __GNUC__ ))
Please write this as:

  #if !defined(__osf__) || defined(__GNUC__)

It's easier to read.  Also, please put an empty line before the #if and after
the #endif.

http://codereview.appspot.com/5886/diff/1/10
File src/google/protobuf/repeated_field.h (left):

http://codereview.appspot.com/5886/diff/1/10#oldcode690
Line 690: 
Please don't remove whitespace.

http://codereview.appspot.com/5886/diff/1/10
File src/google/protobuf/repeated_field.h (right):

http://codereview.appspot.com/5886/diff/1/10#newcode58
Line 58: virtual ~GenericRepeatedField(){}
Please undo this change.  Virtual destructors should never be inline.

http://codereview.appspot.com/5886/diff/1/10#newcode498
Line 498: inline void RepeatedPtrField<Element>::Clear() {
Please don't make this inline.

http://codereview.appspot.com/5886/diff/1/10#newcode505
Line 505: #if !(defined( __osf__ ) && !defined( __GNUC__ ))
This logic is hard to read, due to double-negatives.  Write it like this
instead:

  #if defined(__osf__) && !defined(__GNUC__)
  // Tru64 can't handle out-of-line template specialization.
  ... tru64 code ...
  #else
  ... normal code ...
  #endif

http://codereview.appspot.com/5886/diff/1/10#newcode508
Line 508: inline void LIBPROTOBUF_EXPORT RepeatedPtrField<string>::Clear();
This is not inline.  I don't think this compiles.  Did you test your changes on
a non-tru64 platform?

http://codereview.appspot.com/5886/diff/1/10#newcode517
Line 517: #endif
Please leave a blank line before the next method definition.

http://codereview.appspot.com/5886/diff/1/10#newcode696
Line 696: typedef typename iterator::reference nested_reference;
Why did you rename these?

http://codereview.appspot.com/5886/diff/1/11
File src/google/protobuf/stubs/common.h (right):

http://codereview.appspot.com/5886/diff/1/11#newcode31
Line 31: // Tru64 have an <inttypes.h> that contains much of what we need.
This comment should read:

// Tru64 lacks stdint.h, but has inttypes.h which defines a superset of what
stdint.h would define.

http://codereview.appspot.com/5886/diff/1/12
File src/google/protobuf/stubs/strutil.h (right):

http://codereview.appspot.com/5886/diff/1/12#newcode32
Line 32: //strtol is already 64 bits on Tru64 and long long strtol are not
implemented
Move this comment under the #if.

http://codereview.appspot.com/5886/diff/1/12#newcode33
Line 33: #if defined(__osf__) &&  !defined(__GNUC__)
Might as well make this an #elif and remove the #endif above, since they are
mutually exclusive.

http://codereview.appspot.com/5886/diff/1/12#newcode36
Line 36: //Cannot redefine isxdigit and isprint
I don't understand.  Why are the #undefs needed?

http://codereview.appspot.com/5886/diff/1/14
File src/gtest/gtest.cc (right):

http://codereview.appspot.com/5886/diff/1/14#newcode805
Line 805: String FormatForFailureMessage(wchar_t wchar) {
If this function isn't actually used, can you just delete it?  There's no point
in having this code exist only on some platforms.

http://codereview.appspot.com/5886/diff/1/14#newcode2065
Line 2065: test_info_list_->ForEach(internal::Delete<TestInfo>);
This line is important.  You can't just skip it on tru64.  If it doesn't work,
please replace it with something that does.

http://codereview.appspot.com/5886/diff/1/15
File src/gtest/internal/gtest-internal.h (right):

http://codereview.appspot.com/5886/diff/1/15#newcode248
Line 248: inline String FormatForFailureMessage(T& value) {
Why is this needed?

http://codereview.appspot.com/5886/diff/1/15#newcode264
Line 264: String FormatForFailureMessage(wchar_t wchar);
Again, just remove this if it isn't used.

http://codereview.appspot.com/5886/diff/1/16
File src/gtest/internal/gtest-port.h (right):

http://codereview.appspot.com/5886/diff/1/16#newcode226
Line 226: #if defined(GTEST_OS_WINDOWS) || (defined(GTEST_OS_LINUX) &&
defined(SWIG)) || (defined( __osf__ ) && !defined( __GNUC__ ))
Actually, can you just replace this whole line with:

  #ifndef __GNUC__

__attribute__((unused)) is GCC-specific anyway.

[choiniere.vincent, 2008-09-23 17:07:26]

From previous code review remarks

[choiniere.vincent, 2008-09-23 18:53:17]

one file missing...

[choiniere.vincent, 2008-09-23 18:59:55]

So this is an new "patch" based on your review.

the anonymous namespace simply don't work with nested class on this platform, so
instead of eliminate the nested class, I use a little tricks to made the
"anonymous namespace" visible with Tru64.

The GenericRepeatedField, i try to make some workaround, but the only way to
make this work, is to put everything inside the .h file.  

The compilation work on Tru64 and Visual Studio, and the Tests is 100 % on
Visual Studio...

http://codereview.appspot.com/5886/diff/401/209
File src/google/protobuf/compiler/cpp/cpp_unittest.cc (right):

http://codereview.appspot.com/5886/diff/401/209#newcode56
Line 56: namespace GOOGLE_ANONYMOUS_NAMESPACE {
The problem with the anonymous namespace in this file is about the nested class
that tru64 cannot recognize.. 

So three option can be used (I not.

The easy one, force a name anonymous namespace (in the set patch 1, it was
internal)

The second option is to avoid nested class, so instead of
class GeneratedServiceTest
{
  class MockTestService{}
    class MockTestService mock_service_;

}

use 

class MockTestService{}
class GeneratedServiceTest(){
  class MockTestService mock_service_;
}

Or, like I did here, define GOOGLE_ANONYMOUS_NAMESPACE to be anonymous_namespace
in common.h and use it only for tru64.  It will be a real anonymous_namespace
for the others compilers, but not for cxx on tru64

The GOOGLE_ANONYMOUS_NAMESPACE
#ifndef __DECCXX
#define GOOGLE_ANONYMOUS_NAMESPACE
#else
#define GOOGLE_ANONYMOUS_NAMESPACE  anonymous_namespace
#endif

http://codereview.appspot.com/5886/diff/401/210
File src/google/protobuf/descriptor.cc (right):

http://codereview.appspot.com/5886/diff/401/210#newcode1157
Line 1157:
proto->set_label(static_cast<FieldDescriptorProto::Label>(static_cast<int>(label())));
static_cast from an enum to an enum didn't work,
it can be a "c cast"
 ->set_label((FieldDescriptorProto::Label)label()).

or use a static_cast<int> between the two static_cast of enum...

I try reinterpret_cast and it didn't work.

http://codereview.appspot.com/5886/diff/401/212
File src/google/protobuf/io/zero_copy_stream_impl.cc (right):

http://codereview.appspot.com/5886/diff/401/212#newcode32
Line 32: 
Because the function "min" is not found without <algorithm>.
Do I need to use #if #endif for this?

http://codereview.appspot.com/5886/diff/401/213
File src/google/protobuf/message.h (right):

http://codereview.appspot.com/5886/diff/401/213#newcode103
Line 103: #endif
I didn't check for gcc, but instead of iosfwd, iostream do the job perfectly on
Tru64 and Microsoft Studio.

So instead of use something like above,do you want that I only use
#include <iostream> 
and erase the include of iosfwd, that will avoid the #if __DECCXX

http://codereview.appspot.com/5886/diff/401/214
File src/google/protobuf/repeated_field.h (right):

http://codereview.appspot.com/5886/diff/401/214#newcode58
Line 58: virtual ~GenericRepeatedField(){};
Here we go.

Templates are very difficult to make it work on Tru64.
In addition, many problems occur with the "link" for this class.

the linker doesn't like to have multiple definition of the same type.

For the destructor, if we put the declaration in the .cc file, the linker output
a "multiple definitions" error.

It is an error to do this. Because, honestly, the only way that it is work is to
do something like this.  

In fact, I check the stl template on the Tru64 cxx compiler and they did the
same thing for some of their destructors.

http://codereview.appspot.com/5886/diff/401/214#newcode498
Line 498: inline void RepeatedPtrField<Element>::Clear() {
The inline here, is not required...

http://codereview.appspot.com/5886/diff/401/214#newcode511
Line 511: }
I took the function in the .cc file and I put it here. This is the only way that
I can avoid the multiply defined

This inline here work perfectly on Tru64 and on Visual Studio 2005.  I cannot
tell exactly if Visual Studio really inline the function, because it can ignore
this, but for Tru64, without the inline, the linker output the "multiply
defined" error.

http://codereview.appspot.com/5886/diff/401/214#newcode693
Line 693: typedef typename iterator::difference_type nested_difference_type;
Redefinition on cxx Tru64 are not permitted, so the only choice here is to use a
new name.

http://codereview.appspot.com/5886/diff/401/215
File src/google/protobuf/stubs/common.h (right):

http://codereview.appspot.com/5886/diff/401/215#newcode43
Line 43: #endif
Anonymous namespace work on Tru64 with a lot of troubles. 

To avoid some problems (in fact, the compiler cannot find the symbol) :
1) I can eliminate the nested class inside an anonymous namespace, 
2) or use the "internal" namespace,

3) or use this hack, with the #define GOOGLE_ANONYMOUS_NAMESPACE 
anonymous_namespace on the Tru64.
the compiler on Tru64 is happy, and the real implementation of anonymous
namespace on any other compiler it in use.

http://codereview.appspot.com/5886/diff/401/216
File src/google/protobuf/stubs/strutil.h (right):

http://codereview.appspot.com/5886/diff/401/216#newcode37
Line 37: #undef isprint
isprint is a macro, a function using the same name of the macro is not permitted

http://codereview.appspot.com/5886/diff/401/218
File src/gtest/gtest.cc (left):

http://codereview.appspot.com/5886/diff/401/218#oldcode823
Line 823: }
Not used for Protobuf-test

http://codereview.appspot.com/5886/diff/401/218
File src/gtest/gtest.cc (right):

http://codereview.appspot.com/5886/diff/401/218#newcode130
Line 130: #endif
Thing like : test_info_list_->ForEach(internal::Delete<TestInfo>);  cannot
compile at all on Tru64.

The error is that "(internal::Delete<TestInfo>)" is seeing like "argument
unknown".  
The compiler cannot work on template function.

So instead of template, it use function for each case.
and for unknown reason, the name of the functions needed to be different...

http://codereview.appspot.com/5886/diff/401/219
File src/gtest/internal/gtest-internal.h (left):

http://codereview.appspot.com/5886/diff/401/219#oldcode258
Line 258: String FormatForFailureMessage(wchar_t wchar);
Not used for Protobuf-test

http://codereview.appspot.com/5886/diff/231/417
File src/google/protobuf/repeated_field.cc (left):

http://codereview.appspot.com/5886/diff/231/417#oldcode25
Line 25: namespace internal {
Evething is inside the

[choiniere.vincent, 2008-09-23 19:48:08]

update of the README file

[kenton, 2008-09-24 21:27:27]

http://codereview.appspot.com/5886/diff/401/212
File src/google/protobuf/io/zero_copy_stream_impl.cc (right):

http://codereview.appspot.com/5886/diff/401/212#newcode32
Line 32: 
On 2008/09/23 18:59:55, choiniere.vincent wrote:
> Because the function "min" is not found without <algorithm>.
> Do I need to use #if #endif for this?

No, this include is fine.

http://codereview.appspot.com/5886/diff/233/430
File src/google/protobuf/compiler/cpp/cpp_unittest.cc (right):

http://codereview.appspot.com/5886/diff/233/430#newcode56
Line 56: namespace GOOGLE_ANONYMOUS_NAMESPACE {
If you are going to assign names to these "anonymous namespaces", then name has
to be *different* in every file.

Can you please give me a link to a page discussing the bug with anonymous
namespaces and inner classes with your compiler?  I tried Googling for more info
and didn't find anything.

http://codereview.appspot.com/5886/diff/233/434
File src/google/protobuf/message.h (right):

http://codereview.appspot.com/5886/diff/233/434#newcode100
Line 100: #ifdef __DECCXX
Why __DECCXX here when you used __osf__ everywhere else?

http://codereview.appspot.com/5886/diff/233/434#newcode101
Line 101: //Tru64 iosfwd header doesn't include iostream.
The point of iosfwd is to forward-declare the iostream classes without including
their entire definitions.  I prefer this because message.h is included by a lot
of people (everyone who uses the library), and including the massive iostream
would slow down compiles.  If Tru64's version doesn't do this, it's broken.  It
will have to use iostream instead, and deal with slower compilation.

http://codereview.appspot.com/5886/diff/233/436
File src/google/protobuf/repeated_field.h (right):

http://codereview.appspot.com/5886/diff/233/436#newcode58
Line 58: virtual ~GenericRepeatedField(){};
If there is no way to make Tru64 work without declaring the destructor inline,
then please use #ifdefs here so that the destructor stays out-of-line on other
systems.  Virtual destructors should never be inline because it causes separate
copies of the vtable to be emitted in every translation unit that includes the
header.

http://codereview.appspot.com/5886/diff/233/436#newcode506
Line 506: inline void RepeatedPtrField<string>::Clear() {
Please put the code back the way it was and use #ifdefs if you need it to be
different on tru64.  Making this function inline for everyone will bloat binary
sizes and possibly harm performance.

http://codereview.appspot.com/5886/diff/233/441
File src/gtest/internal/gtest-internal.h (right):

http://codereview.appspot.com/5886/diff/233/441#newcode243
Line 243: inline String FormatForFailureMessage(T& value) {
I'd still like to know why you needed to remove the "const" here.

[choiniere.vincent, 2008-09-26 15:11:10]

See reply to your comments...

http://codereview.appspot.com/5886/diff/233/430
File src/google/protobuf/compiler/cpp/cpp_unittest.cc (right):

http://codereview.appspot.com/5886/diff/233/430#newcode56
Line 56: namespace GOOGLE_ANONYMOUS_NAMESPACE {
Like you, I did not found anything on the Internet to explain the problem...
If fact, the problem is moreabout a nested class inside an anonymous namespace
with inheritance of a class  that have virtual function.....  

 
Take the following sample (based on the class GeneratedServiceTest)


//FILE XYZ.cpp
namespace xyz {
   class ParentClass {
    public:
       ParentClass() {}
       virtual ~ParentClass() {}
       virtual int virtualFct() {}

   };
} //namespace xyz

namespace {

   class FirstClass {
    public:
       class NestedClass : public xyz::ParentClass {
          public:
          NestedClass() {}
          ~NestedClass() {}
          int testint(){return 1;}
          int virtualFct() {return 1;}
       };
      ~FirstClass(){}
      NestedClass _nestedClass;

    };
}  // namespace

//END OF THE FILE XYZ.cpp
================================================

==> if I compile this file on Tru64, I receive the following error message :
cxx: Error: google/protobuf/compiler/cpp/cpp_unittest.cc, line xx: function
          "<unnamed>::FirstClass::NestedClass::~NestedClass" was referenced  but
not defined ~NestedClass() {}
cxx: Error: google/protobuf/compiler/cpp/cpp_unittest.cc, line xx: function
          "<unnamed>::FirstClass::NestedClass::virtualFct" was referenced  but
not defined int virtualFct() {return 1;}


==> I even try to put to declare the same classes in a header... same error.
============================

The only way to compile this is do one of the three steps :

1) Remove any "virtual function" of the parent class (here ParentClass)  ==> not
a good option

2) By not doing nested class, here, put the declaration of NestedClass outside
the "FirstClass" ==> better than the first option

3) Don't use anonymous namespace. ==> easier to understand..


==> Nested Class inside anonymous namespace work in Tru64 but not with
inheritance class with virtual function...

http://codereview.appspot.com/5886/diff/233/434
File src/google/protobuf/message.h (right):

http://codereview.appspot.com/5886/diff/233/434#newcode100
Line 100: #ifdef __DECCXX
__ofs__ was removed because it was used with !(_GNUC__). 

So instead, I check if the compiler is a Compaq C++ used in Tru64.  


So to avoid any problem with the HP C++ for Linux Alpha, I will use __DECCXX and
__osf__ to check is the compiler is an compaq C++ (hpC++) and that the system is
an Tru64 UNIX.

http://codereview.appspot.com/5886/diff/233/434#newcode101
Line 101: //Tru64 iosfwd header doesn't include iostream.
On 2008/09/24 21:27:28, kenton wrote:
No problem for that....

http://codereview.appspot.com/5886/diff/233/436
File src/google/protobuf/repeated_field.h (right):

http://codereview.appspot.com/5886/diff/233/436#newcode58
Line 58: virtual ~GenericRepeatedField(){};
On 2008/09/24 21:27:28, kenton wrote:
okai...

http://codereview.appspot.com/5886/diff/233/436#newcode506
Line 506: inline void RepeatedPtrField<string>::Clear() {
On 2008/09/24 21:27:28, kenton wrote:

okai, no problem for this...
I was just trying to not "overload" the code for Tru64...
In fact, the compiler usually decide if the function will be inline or not.

http://codereview.appspot.com/5886/diff/233/441
File src/gtest/internal/gtest-internal.h (right):

http://codereview.appspot.com/5886/diff/233/441#newcode243
Line 243: inline String FormatForFailureMessage(T& value) {
On 2008/09/24 21:27:28, kenton wrote:
I can simply remove this part, because, the Test never use this function...

The problem is that the compiler don't like when it can match more than one
overload instance...

[choiniere.vincent, 2008-09-30 21:02:14]

One more patch for Tru64

[choiniere.vincent, 2008-09-30 21:03:42]

The last patch is about my "Patch Set 4" comments to your last reply