Skip to content

C++ compiler issues with generated code #437

New issue
New issue
Closed
Closed
C++ compiler issues with generated code#437

Description

@DanielDylan
DanielDylan
opened on Feb 17, 2017

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

Activity

  1. tmontgomery commented on Feb 17, 2017

    @tmontgomery
    tmontgomery
    on Feb 17, 2017
    Contributor

    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.

    1. 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.

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

  2. tmontgomery commented on Feb 17, 2017

    @tmontgomery
    tmontgomery
    on Feb 17, 2017
    Contributor

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

  3. tmontgomery
    added a commit that references this issue on Feb 17, 2017

    change (void) param methods to jsut () for #437. Make tautological co…

    37e75a0

  4. DanielDylan commented on Feb 19, 2017

    @DanielDylan
    DanielDylan
    on Feb 19, 2017
    Author

    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.

  5. tmontgomery commented on Mar 1, 2017

    @tmontgomery
    tmontgomery
    on Mar 1, 2017
    Contributor

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

  6. ccge commented on Apr 26, 2017

    @ccge
    ccge
    on Apr 26, 2017

    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

  7. tmontgomery commented on Apr 27, 2017

    @tmontgomery
    tmontgomery
    on Apr 27, 2017
    Contributor

    @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.

  8. nauhygon commented on May 8, 2017

    @nauhygon
    nauhygon
    on May 8, 2017
    Contributor

    @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)

  9. tmontgomery commented on May 9, 2017

    @tmontgomery
    tmontgomery
    on May 9, 2017
    Contributor

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

  10. nauhygon
    added a commit that references this issue on May 10, 2017

    aeron-io#437: use compiler detections to guard pragmas to suppress ex…

    0f97cc0

  11. nauhygon commented on May 10, 2017

    @nauhygon
    nauhygon
    on May 10, 2017
    Contributor

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

  12. tmontgomery commented on May 11, 2017

    @tmontgomery
    tmontgomery
    on May 11, 2017
    Contributor

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

  13. nauhygon
    added a commit that references this issue on May 11, 2017

    aeron-io#437: use compiler detections to guard pragmas to suppress ex…

    ec70e94
  14. tmontgomery
    added a commit that references this issue on May 11, 2017

    Merge pull request #454 from nauhygon/master

    623726f
  15. mjpt777
    closed this as completedon Jan 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions