Draft: Add a lint job #166

turnrye · 2023-08-16T12:24:52Z

What

When I was contributing, I realized that code styles could be replaced with just enforcing clang-format. There appears to have been a previous integration done, but it's not been maintained. Let's fix that by adding a CI job. That way format can just be on auto-pilot and we don't need to be concerned, letting us remove the entire code_style page from the website.

Why

To make contributing easier, both for the contributor and the maintainer.

How

This PR leverages where meson automatically creates the clang-format and clang-format-check target if the host has clang-format installed.

The timing of landing this PR is critical -- just before it lands, catch this branch up with wherever it's being merged, and then mason setup build; ninja -C build clang-format needs to run and have the resulted checked in. Otherwise builds will be blocked.

Note this requires clang-format-16 due to the complex standards this project has for AlignTrailingComments.

Next steps

I think we should evaluate changing the clang-format configuration. The configuration used was already present, but it didn't appear to be getting followed.
There's a wrapper script present in the repo too, I think we should remove that to avoid any confusion about entry (do it via meson setup build; ninja -C build clang-format)
This supersedes the clang-format portion of Remove default include paths #96 by @carpikes

silseva · 2023-08-16T13:34:29Z

Hi!
Thanks for the PR! About the current clang-format configuration: I started working on a version more adherent to the code stile of the OpenRTX codebase but, due to lack of time, I never manage to complete it (also because fine-tuning everything is a long process). I have to search where I left the modified file and attach it here.

Btw, something I tried to configure is to avoid the linter aggressively reorder the whitespaces in the struct definitions. For example this file became this after a lint pass, something I consider a regression in the code style. If you know a wayt to tell clang-format to not bother with these alignment, please let me know.

turnrye · 2023-08-16T13:43:11Z

Hi! Thanks for the PR! About the current clang-format configuration: I started working on a version more adherent to the code stile of the OpenRTX codebase but, due to lack of time, I never manage to complete it (also because fine-tuning everything is a long process). I have to search where I left the modified file and attach it here.

Btw, something I tried to configure is to avoid the linter aggressively reorder the whitespaces in the struct definitions. For example this file became this after a lint pass, something I consider a regression in the code style. If you know a wayt to tell clang-format to not bother with these alignment, please let me know.

Thanks @silseva ! Looking forward to your guidance on where these changes are undesired and clang-format needs more configuration.

Regarding the whitespace concern you mention, I can fix this. I'll push up a change in a few minutes,

openrtx/include/core/datetime.h

.github/workflows/lint.yml

openrtx/include/calibration/calibInfo_GDx.h

openrtx/include/core/gps.h

openrtx/include/fonts/adafruit/FreeSans10pt7b.h

silseva · 2023-08-16T16:51:24Z

Here you are the clang-format file I began to tweak. As you'll see, I set the PenaltyBreakComment to an insanely high value to prevent the lint to break a line when the comment overflows the 80 line limit by some characters. Rationale for this is:

comments are... comments, I don't consider it crucial to have them fall within the 80 column limit;
there are places where the lint pass makes the code worse than before because it breaks the line in an insane place, like here (before it was this).

Thanks for the work done so far!
clang-format.txt

turnrye · 2023-08-17T01:04:04Z

@silseva when you get a moment, please re-review and point me to some examples of the patterns of issues that you see.

I do notice right now where the following rule from the website is being contradicted:

A space must be put between angle brackets and template argument in C++ code

Unfortunately it does not appear that this is compatible with clang-format. The formatter will always remove the spaces.

There are some changes this introduces to typedef names collapsing onto the end of the struct's definition block, but I'm also not seeing a way to change this with clang-format.

Other than these, I'm not seeing other coding styles that are obstructed by this change. But C/C++ is not my experience, and so I'm sure that I'm missing things.

silseva · 2023-08-17T08:21:07Z

openrtx/include/calibration/calibInfo_GDx.h

-    bandCalData_t data[2];      // Calibration data for VHF (index 0) and UHF (index 1) bands
-    freq_t vhfCalPoints[8];     // VHF calibration points for both TX power and mod1Amplitude
+    bandCalData_t
+        data[2]; // Calibration data for VHF (index 0) and UHF (index 1) bands


Is there a way to avoid having the lint doing this? Ok that the line goes above the 80 column limit, but breaking it like this make it unreadable. Probably the only way to fix this behavior is to split the comment (but geez).

silseva · 2023-08-17T08:22:37Z

openrtx/include/calibration/calibInfo_Mod17.h

-}
-mod17Calib_t;
+    uint8_t mic_gain;
+    uint8_t tx_invert : 1, rx_invert : 1, _padding : 6;


We should prevent the lint from doing this

Right now this is happening because there are no comments after the field definition. The other cases where the newlines are maintained are there because of the comments. I'm unclear on whether we can force these to new lines without comments (or disabling clang-format for the block).

Will investigate feasibility here more later.

silseva · 2023-08-17T08:24:37Z

openrtx/include/core/cps.h

-
-
+    uint8_t rxToneEn : 1,           //< RX CTC/DCS tone enable
+        rxTone       : 7;           //< RX CTC/DCS tone index


All the fields should stay aligned, like it was before. This makes them more readable

Help me understand what I'm looking at here, is this called a "multiple adjacent bit field"?

Haven't cracked how to do this yet.

silseva · 2023-08-17T09:42:18Z

openrtx/include/core/graphics.h

@@ -223,8 +221,11 @@ uint8_t gfx_getFontHeight(fontSize_t size);
 * @param buf: char buffer
 * @return text width and height as point_t coordinates
 */
-point_t gfx_printBuffer(point_t start, fontSize_t size, textAlign_t alignment,
-                        color_t color, const char *buf);
+point_t gfx_printBuffer(point_t start,


Probably is better to avoid having the pile of arguments, when they are more than three the linted version becomes a bit more messy than the original one

silseva · 2023-08-17T09:43:17Z

openrtx/include/core/ringbuf.hpp

 class RingBuffer
 {
-public:


Is it possible to leave the private, public and protected fields with indentation zero (as they where before)?

silseva · 2023-08-17T09:47:10Z

openrtx/include/core/gps.h

-    float      tmg_true;             // Course over ground, degrees, true
-}
-gps_t;
+    datetime_t timestamp;       // Timestamp of the latest GPS update


Is it possible to tell the lint to do the opposite of what is doing here? Instead of chewing up the extra spaces, I think is better, for readability, to have all the field names aligned.

silseva · 2023-08-17T09:48:18Z

openrtx/include/core/state.h

@@ -35,32 +35,31 @@
 */
 typedef struct
 {


Just another example: removing the extra spaces broke the alignment of the fields, making the code less clear

silseva · 2023-08-17T09:49:27Z

openrtx/include/core/voicePrompts.h

@@ -198,23 +198,21 @@ typedef enum
    vpAnnounceLessCommonSymbols         = 0x10,
    vpAnnounceASCIIValueForUnknownChars = 0x20,
    vpAnnouncePhoneticRendering         = 0x40,
-}
-vpFlags_t;
+} vpFlags_t;

 /**
 * Queuing flags determining if speech is interrupted, played immediately,
 * whether prompts are queued for values, etc.
 */
 typedef enum
 {


Urgh! Also here: please, lint, leave the alignment as it was :)

silseva · 2023-08-17T09:50:34Z

openrtx/include/protocols/M17/M17CodePuncturing.hpp

@@ -36,22 +37,73 @@ namespace M17
 /**
 * Puncture matrix for linx setup frame.
 */
-static constexpr auto LSF_PUNCTURE = std::experimental::make_array< uint8_t >


Here the lint went crazy!

Haha, going to have to use a //clang-format off here.

silseva · 2023-08-17T09:51:42Z

openrtx/include/protocols/M17/M17Constants.hpp

-static constexpr syncw_t EOT_SYNC_WORD    = {0x55, 0x5D};  // End of transmission sync word
+static constexpr syncw_t LSF_SYNC_WORD    = {0x55, 0xF7}; // LSF sync word
+static constexpr syncw_t BERT_SYNC_WORD   = {0xDF, 0x55}; // BERT data sync word
+static constexpr syncw_t STREAM_SYNC_WORD = {0xFF,


Why did the lint break this? Again for the 80 column overflow?

Yes, but we can force the comment to wrap instead.

silseva · 2023-08-17T09:52:43Z

openrtx/include/protocols/M17/M17Demodulator.hpp

-    static constexpr float  CONV_STATS_ALPHA       = 0.005f;
-    static constexpr float  CONV_THRESHOLD_FACTOR  = 3.40;
-    static constexpr int16_t QNT_SMA_WINDOW        = 8;
+    static constexpr size_t M17_RX_SAMPLE_RATE = 24000;


Ok that we overflowed the 80 columns, but it was more readable... Probably we have to add lint exceptions here and there

silseva · 2023-08-17T09:53:22Z

openrtx/include/protocols/M17/M17Demodulator.hpp


    /*
     * Buffers
     */
-    std::unique_ptr< int16_t[] > baseband_buffer; ///< Buffer for baseband audio handling.


Also here we have a readability regression

silseva · 2023-08-17T10:01:07Z

I did a round of revision and, while doing it, I remembered why at some point I gave up with adjusting the clang-format file: I found that in some occasions the lint behavior is way too rigid and leads to code worse than before in terms of readability.
Probably is better to have the clang-format ready and apply it by hand before opening a PR or merging a branch into master, instead of having it run as a build hook. In this way we have it doing all the heavy lifting and we just have to tweak the remaining spots of code style by hand. The alternative may be to place "no lint" directives around in the code, but it does not look very elegant to me...

Comments/opinions are welcome!

turnrye · 2023-08-17T11:26:45Z

I did a round of revision and, while doing it, I remembered why at some point I gave up with adjusting the clang-format file: I found that in some occasions the lint behavior is way too rigid and leads to code worse than before in terms of readability. Probably is better to have the clang-format ready and apply it by hand before opening a PR or merging a branch into master, instead of having it run as a build hook. In this way we have it doing all the heavy lifting and we just have to tweak the remaining spots of code style by hand. The alternative may be to place "no lint" directives around in the code, but it does not look very elegant to me...

Comments/opinions are welcome!

Thanks for the detailed feedback. I'll work on these items next.

As far as the rigidity of the formatter, I agree that at times that results in less readable code. The tradeoff, that contributors and reviewers no longer have to be concerned with formatting as they make changes, is quite empowering. In most of my past projects we've found that the compromise is worth it -- but that doesn't necessarily mean that it is here.

Let's see how close I can continue to make this PR before we really consider that question. If in the end it is still unclear what path to take, there are other options that we can take too (optional? just formatting the changed code?).

…and clang-format-ignore to be a little more courteous

…ang-format

turnrye force-pushed the chore/lint branch 2 times, most recently from 67bc810 to 75b2ae8 Compare August 16, 2023 12:30

turnrye force-pushed the chore/lint branch from 75b2ae8 to 5e9ae70 Compare August 16, 2023 14:22

turnrye changed the title ~~Add a lint job~~ Draft: Add a lint job Aug 16, 2023

turnrye marked this pull request as draft August 16, 2023 14:31

turnrye commented Aug 16, 2023

View reviewed changes

turnrye force-pushed the chore/lint branch 3 times, most recently from d5f4fc8 to f5f14d0 Compare August 16, 2023 22:36

turnrye marked this pull request as ready for review August 17, 2023 01:04

silseva reviewed Aug 17, 2023

View reviewed changes

turnrye marked this pull request as draft August 17, 2023 11:17

turnrye force-pushed the chore/lint branch from f5f14d0 to c4ca05f Compare August 17, 2023 11:33

turnrye added 2 commits August 17, 2023 09:50

chore(ci): add an action for clang-format-check; update clang-format …

20f40cd

…and clang-format-ignore to be a little more courteous

chore(format): run rm -rf build; meson setup build; ninja -C build cl…

961a3ba

…ang-format

turnrye force-pushed the chore/lint branch from c4ca05f to 961a3ba Compare August 17, 2023 14:51

n1zzo force-pushed the master branch 5 times, most recently from 2d75e11 to 04d6d01 Compare October 9, 2023 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Add a lint job #166

Draft: Add a lint job #166

turnrye commented Aug 16, 2023 •

edited

Loading

silseva commented Aug 16, 2023

turnrye commented Aug 16, 2023

silseva commented Aug 16, 2023

turnrye commented Aug 17, 2023

silseva Aug 17, 2023

silseva Aug 17, 2023

turnrye Aug 17, 2023

silseva Aug 17, 2023

turnrye Aug 17, 2023

silseva Aug 17, 2023

turnrye Aug 17, 2023

silseva Aug 17, 2023

silseva Aug 17, 2023

silseva Aug 17, 2023

silseva Aug 17, 2023

silseva Aug 17, 2023

silseva Aug 17, 2023

turnrye Aug 17, 2023

silseva Aug 17, 2023

turnrye Aug 17, 2023

silseva Aug 17, 2023

silseva Aug 17, 2023

silseva commented Aug 17, 2023

turnrye commented Aug 17, 2023



		uint8_t rxToneEn : 1, //< RX CTC/DCS tone enable
		rxTone : 7; //< RX CTC/DCS tone index

Draft: Add a lint job #166

Are you sure you want to change the base?

Draft: Add a lint job #166

Conversation

turnrye commented Aug 16, 2023 • edited Loading

What

Why

How

Next steps

silseva commented Aug 16, 2023

turnrye commented Aug 16, 2023

silseva commented Aug 16, 2023

turnrye commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silseva commented Aug 17, 2023

turnrye commented Aug 17, 2023

turnrye commented Aug 16, 2023 •

edited

Loading