[C++]: Static constexpr fields in CppGenerator

[vdidenko]

The pull request addresses the issue #353 and is limited to HeaderStructure data in the stubs.

[rigtorp]

Please revert this patch. It defines static member variables which causes linking issues.

[vdidenko]

Can you possibly be more specific/ provide example/diagnostic? At least more context?

[rigtorp]

SBE_CONST_KIND std::uint16_t Establish::SbeBlockLength;
SBE_CONST_KIND std::uint16_t Establish::SbeTemplateId;
SBE_CONST_KIND std::uint16_t Establish::SbeSchemaId;
SBE_CONST_KIND std::uint16_t Establish::SbeSchemaVersion;
SBE_CONST_KIND char Establish::SbeSemanticType[];

These needs to be in a .cpp file otherwise two .o files that include the SBE flyweight won't link due to multiple definitions of these static members.

[rigtorp]

I created a GeneratedStubExample2.cpp that also includes the Car flyweight. If you try to link GeneratedStubExample.cpp and GeneratedStubExample2.cpp you get this error:

[ 74%] Building CXX object sbe-samples/src/main/cpp/CMakeFiles/GeneratedStubExample.dir/GeneratedStubExample2.cpp.o
/home/erigtorp/src/simple-binary-encoding/sbe-samples/src/main/cpp/GeneratedStubExample2.cpp: In function 'int main2(int, const char**)':
/home/erigtorp/src/simple-binary-encoding/sbe-samples/src/main/cpp/GeneratedStubExample2.cpp:31:10: warning: unused variable 'buffer' [-Wunused-variable]
     char buffer[2048];
          ^
Linking CXX executable ../../../../binaries/GeneratedStubExample
CMakeFiles/GeneratedStubExample.dir/GeneratedStubExample2.cpp.o:(.rodata+0x0): multiple definition of `baseline::Car::SbeSemanticType'
CMakeFiles/GeneratedStubExample.dir/GeneratedStubExample.cpp.o:(.rodata+0x50): first defined here
CMakeFiles/GeneratedStubExample.dir/GeneratedStubExample2.cpp.o: In function `main2(int, char const**)':
/home/erigtorp/src/simple-binary-encoding/sbe-samples/src/main/cpp/GeneratedStubExample2.cpp:37: multiple definition of `baseline::Car::SbeSchemaVersion'
CMakeFiles/GeneratedStubExample.dir/GeneratedStubExample.cpp.o:/home/erigtorp/src/simple-binary-encoding/generated/baseline/MessageHeader.h:48: first defined here
CMakeFiles/GeneratedStubExample.dir/GeneratedStubExample2.cpp.o:/opt/cresearch/vendor/gcc/5.3.0/include/c++/5.3.0/iostream:74: multiple definition of `baseline::Car::SbeSchemaId'
CMakeFiles/GeneratedStubExample.dir/GeneratedStubExample.cpp.o:/home/erigtorp/src/simple-binary-encoding/generated/baseline/MessageHeader.h:48: first defined here
CMakeFiles/GeneratedStubExample.dir/GeneratedStubExample2.cpp.o:/opt/cresearch/vendor/gcc/5.3.0/include/c++/5.3.0/iostream:74: multiple definition of `baseline::Car::SbeTemplateId'
CMakeFiles/GeneratedStubExample.dir/GeneratedStubExample.cpp.o:/home/erigtorp/src/simple-binary-encoding/generated/baseline/MessageHeader.h:48: first defined here
CMakeFiles/GeneratedStubExample.dir/GeneratedStubExample2.cpp.o:/opt/cresearch/vendor/gcc/5.3.0/include/c++/5.3.0/iostream:74: multiple definition of `baseline::Car::SbeBlockLength'
CMakeFiles/GeneratedStubExample.dir/GeneratedStubExample.cpp.o:/home/erigtorp/src/simple-binary-encoding/sbe-samples/src/main/cpp/GeneratedStubExample.cpp:40: first defined here
collect2: error: ld returned 1 exit status
make[2]: *** [binaries/GeneratedStubExample] Error 1
make[1]: *** [sbe-samples/src/main/cpp/CMakeFiles/GeneratedStubExample.dir/all] Error 2
make: *** [all] Error 2

[rigtorp]

Added issue #367 to track this.

[vdidenko]

It makes sense that the definitions would be guarded within one object file, yet collide if included in multiple and then linked into one. I am struggling to understand a benefit of such diamond dependency use case.

I am sure there can be a reason (yet I am curious to learn, if you do not mind), and it aside, what may be options for keeping SbeTemplateId available for use in a switch statement?

Considering that changing static const std::uint16_t sbeTemplateId(void) signature is not an option one way would be to have a second, constexpr static member function returning SbeTemplateId in addition to the existing const function. A little messy, But is the only thing which comes to mind immediately.

Any other thoughts?

[vdidenko]

Instead of hopping between the issues, would it be OK to reopen the #353? My preference is to keep reasoning in one place.

[rigtorp]

This is one solution:

struct Flyweight {
  enum {
    SbeTemplateId = x,
    // etc
  };
 // rest
}

[rigtorp]

It makes sense that the definitions would be guarded within one object file, yet collide if included in multiple and then linked into one. I am struggling to understand a benefit of such diamond dependency use case.

It's not a diamond dependency. I just have two parts of a program using the same flyweight. With the new codegen that is not possible.

[tmontgomery]

Another option might be to only add the new statics when it is C++11 and can be constexprs.

[vdidenko]

Ok, sorry, long day. Makes sense. Moving to the issue thread.

[rigtorp]

Considering that changing static const std::uint16_t sbeTemplateId(void) signature is not an option one way would be to have a second, constexpr static member function returning SbeTemplateId in addition to the existing const function. A little messy, But is the only thing which comes to mind immediately.

static constexpr uint16_t sbeTemplateId() works fine to use in switch() with gcc-5.3.0 -std=c++14. Maybe others too.

[rigtorp]

Another option might be to only add the new statics when it is C++11 and can be constexprs.

Just add constexpr to the current functions. Should work on C++11.