Skip to content

Conversation

woodfell
Copy link
Contributor

@woodfell woodfell commented Apr 21, 2021

Enable the clang-format cmake module and run during generation of C source/headers. .clang-format imported from Starling.

Adjust the C header template slightly to achieve the following:

  • documentation comments are attached to the struct for a message rather than the #define for the message ID
  • documentation comments for struct members are placed preceding the member rather than trailing to prevent excessive line breaks and poor formatting

Previous documentation structure was:

/* Documentation for struct
 *
 * xxx
 */
#define SBP_MSG_xxx (optional)
#define bitfield macros (optional)

typedef struct {
  u8 member; /**< Documentation for member which sometimes
* wraps like this
* [m/s] */
} xxx;

has been changed to

#define SBP_MSG_xxx (optional)
#define bitfield macros (optional)

/* Documentation for struct
 *
 * xxx
 */
typedef struct {
  /** 
   * Documentation for member which wraps
   * nicely [m/s]
   */
  u8 member;
} xxx;

There are no functional changes in this PR. The only places which were edited by hand are:

  • Makefile
  • c/CMakeLists.txt
  • generator/sbpg/targets/resources/sbp_messages_template.h
  • c/include/cpp/state.h (shuffling some // NOLINTs)

Suggestion for reviewing:
Review changes to Makefile, c/CMakeLists.txt, c/.clang-format, and generator/sbpg/targets/resources/sbp_messages_template.h. Copy them in to a fresh checkout of master and run make c. Then diff the repo to this branch to confirm that all other changes are as a result of clang-format.

Copy link
Contributor

@reimerix reimerix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current build image might not include clang-format, here is a snippet from running make -j8 c using the swiftnav/libsbp-build:2021.04.02 image on my machine:

CMake Warning at cmake/common/ClangFormat.cmake:122 (message):
  Could not find appropriate clang-format, targets disabled
Call Stack (most recent call first):
  cmake/common/ClangFormat.cmake:218 (early_exit)
  CMakeLists.txt:13 (swift_setup_clang_format)

@woodfell woodfell changed the title Turn on auto formatting [ESD-2009] Turn on auto formatting Apr 22, 2021
@silverjam
Copy link
Contributor

@woodfell please update the README.md/HOWTO.md to point to the new docker image tag

@woodfell woodfell requested a review from reimerix April 23, 2021 06:25
Copy link
Contributor

@reimerix reimerix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jbangelo
Copy link
Contributor

What's the motivation to remove the empty structs? I can't think of a reason to have included them in the first place, but removing them is a breaking change.

@woodfell
Copy link
Contributor Author

What's the motivation to remove the empty structs? I can't think of a reason to have included them in the first place, but removing them is a breaking change.

Oops, getting ahead of myself here. Empty structs are one of the problems in libsbp but I'll put them back in for the moment

@woodfell woodfell force-pushed the woodfell/formatting branch from e839333 to 1bf3afe Compare April 25, 2021 22:58
@woodfell woodfell merged commit 1f031dc into master Apr 26, 2021
@woodfell woodfell deleted the woodfell/formatting branch April 26, 2021 01:36

/**
* CN/0 of best point [dB Hz]
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is objectively more pretty before than it is after. It used to be visible in 5 lines, now it takes 25,.

woodfell pushed a commit that referenced this pull request Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants