C++ compiler issues with generated code

[DanielDylan]

Firstly thank you for providing this code as open source.

I have found some issues with the C++ generated code:

1. Some methods taking no parameters use (void). That is not standard C++
2. SBE_CONSTEXPR is sometimes followed by const for integer types. That's not legal C++ when SBE_CONSTEXPR is defined as constexpr
3. SBE_CONSTEXPR is sometimes followed by char *. That's not legal C++ when SBE_CONSTEXPR is defined as constexpr as a constexpr may not return a char *, it must return a const char *.
4. The code generated for method wrapForEncode to check if the count is in bounds "if (count < %5$d || count > %6$d)" is illegal when the minimum is the minimum for the type or the maximum is the maximum for the type or both. A #pragma GCC diagnostic ignored "-Wtautological-compare" directive would resolve this but see next.
5. The #pragma GCC diagnostic ignored "-Wtautological-compare" directive is not valid for gcc (at least not with gcc 5.4.0. It seems to be ok with clang 3.7.
6. The C style casts should be replaced by static_casts and reinterpret_casts. Though I note that you have specifically stated this is meant to generate C++98 code.
7. The generated headers have extension of .h. That is no longer the preferred extension for C++ header files.

I have forked your code and added a C++11 generator (C11Generator.java) which fixes the above, but it would be more widely beneficial if it was co-ordinated with your CppGenerator.

Happy to discuss further.

Thank you
Dany

[tmontgomery]

Thanks! Couple comments. Few of these (1, 3, and 6) are good cleanup and would be great for a PR. Feel free to submit it.

2. has been iterated on a number of times. If you have a better way to handle it that works in a broad sense, let us know.

3. I would like to make the extension configurable for ease of use.

[tmontgomery]

Also, 2 and 4 are a pending PR in #384

[DanielDylan]

Thanks for the quick reply,

ignore item 3. That was my own earlier mistake in trying to solve 2.

For 4, I have the java code determine if it's sensible to generate C++ code for the comparison. When the type minimum coincides with the number in group minimum then that term of the comparison expression is not generated. When the type maximum coincides with the number in group maximum, then that term of the comparison expression is not generated. If neither term is generated, then no comparison expression is generated at all.

For 5, if the relevant constexpr method XSinceVersion would return zero, then the boolean expression would always be true so the XInActingVersion always returns true.

I believe this to be generally correct, but I may have missed something. If that's an acceptable modification I'll create a pull request just for those changes.

[tmontgomery]

@DanielDylan go ahead and create a PR if you wish.

[ccge]

Is 5. why I have a warning for every #pragma clang diagnostic in TokenCodec.h and FrameCodec.h?

For example:
1.

/uk_co_real_logic_sbe_ir_generated/TokenCodec.h:170:0: warning: ignoring #pragma clang diagnostic [-Wunknown-pragmas]
#pragma clang diagnostic push

uk_co_real_logic_sbe_ir_generated/FrameCodec.h:170:0: warning: ignoring #pragma clang diagnostic [-Wunknown-pragmas]
 #pragma clang diagnostic push

[tmontgomery]

@ccge some other flags warn have been turned on when they encounter pragmas they do not understand. These can be safely ignored if using gcc.

[nauhygon]

@tmontgomery The warnings spilled out at me as well on Windows against MSVC. Would it make sense to use compiler tests to guard against these compiler-specific #pragmas so other compilers are oblivious to them? This way it removes a ton of warnings in cppbuild.

    bool modelYearInActingVersion()
    {
#if SBE_IS_CLANG
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wtautological-compare"
#endif
        return m_actingVersion >= modelYearSinceVersion();
#if SBE_IS_CLANG
#pragma clang diagnostic pop
#endif
    }

We may need to add the compiler checks like the following.

#define SBE_IS_CLANG defined(__clang__)
#define SBE_IS_GNUC (defined(__GNUC__) || defined(__GNUG__)) && !defined(__clang__)
#define SBE_IS_MSC defined(__MSC_VER)

[tmontgomery]

@nauhygon probably OK to simply inline the compiler checks as it is being generated anyway.

[nauhygon]

@tmontgomery Like this? If the changes look fine to you, I am going to submit a PR. Thanks!

[tmontgomery]

Yeah, this looks fine. Send a PR and I will check into it. Thanks!