ActiveMQ C++ Client
  1. ActiveMQ C++ Client
  2. AMQCPP-36

Cleanup whitespace and member function definitions in header files

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.1
    • Component/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      The AMQCPP code looks great in general, but many files have some stray whitespace. Also, there are quite a few headers that contain member function definitions. Some are simple, but there's also some big ones in there. I usually follow the rule that anything that can go into the .cpp should.

      Attached is a patch to clean up the files in activemq::concurrent. If Timothy/Nathan agrees that doing this is useful, I'll submit patches for the rest.

        Activity

        Albert Strasheim created issue -
        Hide
        Timothy Bish added a comment -

        > I usually follow the rule that anything that can go into the .cpp should.

        I don't follow this rule.

        > but many files have some stray whitespace

        One person's extra whitespace is another persons useful code separation.

        Also patches submitted that add new files shouldn't only update the windows projects, the makefiles would also need to be updated as well, so you need to update them as well or provide a source listing of what you added so we can more easily apply the patches.

        Show
        Timothy Bish added a comment - > I usually follow the rule that anything that can go into the .cpp should. I don't follow this rule. > but many files have some stray whitespace One person's extra whitespace is another persons useful code separation. Also patches submitted that add new files shouldn't only update the windows projects, the makefiles would also need to be updated as well, so you need to update them as well or provide a source listing of what you added so we can more easily apply the patches.
        Hide
        Albert Strasheim added a comment -

        > I don't follow this rule.

        Do you mind if the patches move member functions to .cpp files? As I see it, headers shouldn't include implementation details.

        > One person's extra whitespace is another persons useful code separation.

        What I meant by stray whitespace is that there are lines that have 3 or 4 spaces at the end where they serve no purpose. It's the kind of whitespace that Eclipse's "Remove trailing whitespace" removes and the kind that Checkstyle warns you about.

        I'll update the Linux build files in a new patch.

        Meanwhile, here's a signedness fix from StompFrame.h.

        Show
        Albert Strasheim added a comment - > I don't follow this rule. Do you mind if the patches move member functions to .cpp files? As I see it, headers shouldn't include implementation details. > One person's extra whitespace is another persons useful code separation. What I meant by stray whitespace is that there are lines that have 3 or 4 spaces at the end where they serve no purpose. It's the kind of whitespace that Eclipse's "Remove trailing whitespace" removes and the kind that Checkstyle warns you about. I'll update the Linux build files in a new patch. Meanwhile, here's a signedness fix from StompFrame.h.
        Albert Strasheim made changes -
        Field Original Value New Value
        Attachment StompFrame-signmismatch.diff [ 15260 ]
        Hide
        Timothy Bish added a comment -

        Yes, I do mind. I prefer to leave small functions in the headers of little classes like this, if they get inlined by the compiler or not its fine by me. Simply by nature of C++ the headers have implementation detail, and for that matter most people shouldn't need to be in that code as the CMS interfaces define the core functionality that most users of AMQCPP will be using.

        The question to ask is does the code work, if yes is the answer, then making all these trivial changes is just pulling time away from implementing the current enhancement under development.

        Show
        Timothy Bish added a comment - Yes, I do mind. I prefer to leave small functions in the headers of little classes like this, if they get inlined by the compiler or not its fine by me. Simply by nature of C++ the headers have implementation detail, and for that matter most people shouldn't need to be in that code as the CMS interfaces define the core functionality that most users of AMQCPP will be using. The question to ask is does the code work, if yes is the answer, then making all these trivial changes is just pulling time away from implementing the current enhancement under development.
        Hide
        Albert Strasheim added a comment -

        Fair enough. I'll leave the headers alone and let you clean up the trailing whitespace if it becomes a problem for you.

        If you could apply my StompFrame-signmismatch.diff, you can resolve this issue.

        Thanks very much for your time.

        Show
        Albert Strasheim added a comment - Fair enough. I'll leave the headers alone and let you clean up the trailing whitespace if it becomes a problem for you. If you could apply my StompFrame-signmismatch.diff, you can resolve this issue. Thanks very much for your time.
        Timothy Bish made changes -
        Assignee Nathan Mittler [ nmittler ] Timothy Bish [ tabish121 ]
        Hide
        Timothy Bish added a comment -

        Applied Stomp Frame patch

        Show
        Timothy Bish added a comment - Applied Stomp Frame patch
        Timothy Bish made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Hide
        Albert Strasheim added a comment -

        I think I have two new reasons for possibly moving code from headers to cpp files: code size and build time.

        The latest activemq-cpp.lib is 41,112 KB, which seems rather... large.

        While we were chasing down the bug we were seeing related to AMQCPP-25, we had to make changes to functions that were implemented in headers, causing large parts of the library that didn't really change to be rebuilt.

        Show
        Albert Strasheim added a comment - I think I have two new reasons for possibly moving code from headers to cpp files: code size and build time. The latest activemq-cpp.lib is 41,112 KB, which seems rather... large. While we were chasing down the bug we were seeing related to AMQCPP-25 , we had to make changes to functions that were implemented in headers, causing large parts of the library that didn't really change to be rebuilt.
        Jeff Turner made changes -
        Project Import Fri Nov 26 23:25:50 EST 2010 [ 1290831950293 ]

          People

          • Assignee:
            Timothy Bish
            Reporter:
            Albert Strasheim
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development