Choose a style guide for SS3 code

[k-doering-NOAA]

Issues #293 and #163 brought up the fact that we don't have an agreed upon style guide for the ss3 codebase. I think we could probably choose a c++ one, but we may also need to add conventions given that tpls can look a little different. Some questions that come to mind are:

• Is a semicolon required after each statement when declaring variables? I think you don't needed it for certain sections in the tpl, but maybe we should choose 1 convention.
• Should 1 statement for loops and if statements always be included in curly braces {}?
• Whitespace conventions: this is probably answered in a style guide, but when should there be whitespace and when should there not (e.g., is for(a=1;a<10;a++) or for (a = 1; a < 10; a++) the correct style?)

Perhaps @nschindler-noaa has thoughts on this to start?

[Rick-Methot-NOAA]

I am good with adding semicolons at end of lines, even if unnecessary for that part of the tpl. But all of this is in the pats of the tpl that the style tool cannot be used on, yet.
Good with adding {} even when not needed. Can the style tool be this?
Undecided on white space. It is going to be very hard to remember to add these spaces if we require everything to be delimited by white space.

[k-doering-NOAA]

Thanks for noting your preferences, @Rick-Methot-NOAA! I think since you are the primary code contributor, that matters a lot to keeping style consistent.

I think Neal said that the style tool could add {}, but that we didn't have the setting turned on last time?

I wonder if the way to get started on this would be to

• Rick chooses one of the style guides linked in this section: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#configurable-format-style-options .
• @Rick-Methot-NOAA and @nschindler-noaa could go through it and decide if any changes are needed for our group?
• Finally, perhaps Neal can then make sure the desired settings are selected in the .clang-format file to make sure our styling tool is consistent with our style guide?

To throw in my 2c about whitespace preferences, I find code a lot more readable with more whitespace. But I can get over it if needed. :)

[iantaylor-NOAA]

With regard to whitespace conventions, I can add that with regard to style of R code, the first time the automatic styler added whitespace throughout the r4ss functions, I found them immediately easier to read, but I still often forget to type them when adding new code. However, I don't see that mismatch as a big deal because the github action just adds them for me. Yes, the ideal would be that we could agree on a style that's easy to adhere to, but I don't think it would be a problem if the humans never added whitespaces and they were added every time by ClangFormat.

[k-doering-NOAA]

One good thing I learned is compiler warnings do get generated if you modify something from

if (the_value == 0)
  set_me = 1;

to

if (the_value == 0)
  set_me = 1;
  new_setting = 2;

when what was meant is:

if (the_value == 0)
{
  set_me = 1;
  new_setting = 2;
}

There will be a compiler warning generated when using -Wall and our warnings github action will flag increased warnings :

ss.cpp:12294:9: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
12294 |         if (the_value == 0)
      |         ^~
ss.cpp:12296:11: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
12296 |           new_setting = 2;
      |           ^~~~~~~~~

That being said, I learned this because I made this mistake, so it does illustrate it's an easy thing to do on accident! I'm also not sure if this warning is triggered if the indentation is not the same as in the example.

[Rick-Methot-NOAA]

I would never want to rely on indentation as basis for assuming that consecutive statements belong together in an IF clause.

[k-doering-NOAA]

I would never want to rely on indentation as basis for assuming that consecutive statements belong together in an IF clause.

Yes, completely agree. I still advocate for curly braces in all cases, even when they technically aren't needed.

[Rick-Methot-NOAA]

I think (hope) that I was consistent in omitting the {} only when statement was on same line as the IF:
IF (...) statement
so did the ambiguity arise when the styler put statement on the next line?

[k-doering-NOAA]

Yes, you were consistent! The warnings were caused by me adding lines while working on the console output... I double checked and no similar warnings are in the main branch. I since fixed on the branch I was working on, because github actions caught that there were more warnings.

The warnings are caused when trying to use two statements instead of one (which doesn't work correctly) while the brackets are eliminated. I think the compiler is trying to be helpful here and guess that the editor messed up the syntax!

EDIT: note there is no ambiguity caused by the styler putting the statement on the next line.

[k-doering-NOAA]

Link to the draft google doc where we are working on the style guide: https://docs.google.com/document/d/1q11sZl4rcgx1Cs8wkOs8dPAaIpkEAGrKNb3b94bfbdU/edit?usp=sharing

[nschindler-noaa]

Great! I just found out that we can turn clang-format on and off internally in the code. So, something like this LOCAL_CALCS // clang-format on some code and comments . . . // clang-format off END_CALCS Works! I tried it on SS_param.tpl (which did not work with clang-format before) and it compiled. Now, I just need to adjust the .clang-format file.

On Wed, Jun 22, 2022 at 10:36 AM Kathryn Doering ***@***.***> wrote: Link to the draft google doc where we are working on the style guide: https://docs.google.com/document/d/1q11sZl4rcgx1Cs8wkOs8dPAaIpkEAGrKNb3b94bfbdU/edit?usp=sharing — Reply to this email directly, view it on GitHub <#311 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ARUZACTBCC75OMJTSDWBKS3VQNFKBANCNFSM5VWFCCRA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Neal Schindler Programmer Analyst, Caelum NWFSC (206) 860-3407 cell (206) 909-0505

=(((`< >=(((`< >=(((`<

=(((`<