Update makefile

[Rick-Methot-NOAA]

updating my makefile

[e-perl-NOAA]

@Rick-Methot-NOAA if we are changing it in this file, we should change it across all the files that make ss3. I started working on doing this on this branch. I will also need to make a change to all the GitHub actions. I am in meetings all the rest of the day but will work on this early next week.

[Rick-Methot-NOAA]

This is the makefile I use locally, so it got pushed to github. We can explore alternatives to keep my local building more separated.

[e-perl-NOAA]

I think it would be fine to change the names to ss3 all around. So I'm happy to work on it to incorporate it across the board.

[e-perl-NOAA]

@Rick-Methot-NOAA we are now getting warnings for the following lines
ss.cpp:34714:5: warning: this ‘for’ clause does not guard... [-Wmisleading-indentation]
34714 | for (gg = 1; gg <= gender; gg++)
| ^~~
ss.cpp:34719:7: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘for’
34719 | SS2out << " mature_bio mature_num ";
| ^~~~~~

[Rick-Methot-NOAA]

@e-perl-NOAA What is the next step here?

[e-perl-NOAA]

I am happy to update the reference warning file and we can proceed with those warnings. I tried to debug myself and see if I could fix the warnings that it's giving, but I had no luck. In our meeting last week, you said that you would try to see if you could make any progress on getting rid of those warnings. Would you like to work on getting rid of those warnings before I merge?

[Rick-Methot-NOAA]

I do not think it a good practice to make changes to the tpl files in same commit as changes to the workflow. Can you back them out so I can see the original new warnings.

[e-perl-NOAA]

@Rick-Methot-NOAA the commits that I made when trying to see if I could resolve the warning have been reverted.

[e-perl-NOAA]

The warning in the artifact is:

In file included from ss.cpp:7:
In file included from ss.cpp:7:
ss.cpp: In member function ‘void model_parameters::write_bigoutput()’:
ss.cpp:34714:5: warning: this ‘for’ clause does not guard... [-Wmisleading-indentation]
34714 |     for (gg = 1; gg <= gender; gg++)
      |     ^~~
ss.cpp:34719:7: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘for’
34719 |       SS2out << " mature_bio mature_num ";
      |       ^~~~~~

To get this to pass (at least in the meantime to get these changes through), I would just need to replace the text in .github/workflows/reference_files/warning_ss_ref.txt with those warnings above. Which I can do if that is how you would like to move forward.

[Rick-Methot-NOAA]

I will fix that one loop with the guard warning.

[Rick-Methot-NOAA]

Did you recently change from compiling with C++14 to C++17? I am using C++14 locally.
When I compile locally with warnings, I get
compile
using C++17 with -Wall -Wextra

In file included from ss3.cpp:7:
C:\ADMB-13.1\include/admodel.h: In member function 'virtual void function_minimizer::report(const dvector&)':
C:\ADMB-13.1\include/admodel.h:1915:38: warning: unused parameter 'gradients' [-Wunused-parameter]
virtual void report(const dvector& gradients){;};
~~~~~~~~~~~~~~~^~~~~~~~~
In file included from ss3.cpp:7:
C:\ADMB-13.1\include/admodel.h: In member function 'virtual void param_init_d3array::save_value(const ofstream&, int, const dvector&, int&)':
C:\ADMB-13.1\include/admodel.h:2501:43: warning: unused parameter 'ofs' [-Wunused-parameter]
virtual void save_value(const ofstream& ofs, int prec,const dvector&,
~~~~~~~~~~~~~~~~^~~
C:\ADMB-13.1\include/admodel.h:2501:52: warning: unused parameter 'prec' [-Wunused-parameter]
virtual void save_value(const ofstream& ofs, int prec,const dvector&,
~~~~^~~~
C:\ADMB-13.1\include/admodel.h:2502:10: warning: unused parameter 'offset' [-Wunused-parameter]
int& offset){}
~~~~~^~~~~~

[iantaylor-NOAA]

@Rick-Methot-NOAA, the build-ss3-warnings workflow doesn't explicitly install C++. The recent change was adopting the ADMB Docker image, which I think has the C++ compiler included. I think the previous iteration of the workflow relied on whatever version of the compiler was included in the operating system set up for the GitHub Actions. So this may well have led to a switch in version and different warnings.

@johnoel can likely confirm the C++ compiler version included in the AMDB Docker images.

[Rick-Methot-NOAA]

I am seeing the same warnings with c++14 and c++17. They do not include the warning about for loop indentation not guarding following statement. So, still perplexed by that new warning coming from docker.
read about the warning here: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wmisleading-indentation

[johnoel]

Yes, C++17 will be the default for ADMB-13.2. All of the warning have been resolved and will now build clean. I will update the docker images soon for testing.

I remember seeing the bracket warnings. It is always good practice to add brackets to all loops and conditionals.

[iantaylor-NOAA]

Thanks @johnoel. Good to know the version.

@Rick-Methot-NOAA the build-warnings workflow is just running on linux, so maybe the indent warning is not checked in the same way on windows even within C++17. I just pushed a change with additional brackets to see if that makes it go away.

[e-perl-NOAA]

I went into codespaces to debug since the action and codespace are both on a linux machine. The warnings workflow needed to be updated to search for ss3.cpp string (previously was ss.cpp) which was a hard catch. The linux machine in codespaces is now also catching the warnings that you caught @Rick-Methot-NOAA. I updated the warnings reference file to contain those warnings so it should pass now.

[Rick-Methot-NOAA]

What still perplexes me is that the tpl contains many for loops that use the same indenting syntax as the one loop that was triggering the guard warning. That style of indenting seems to be standard C++ and may even have been intentional when Neal did the "prettify" code. Perhaps the prettify code could be modified and run again to add all explicit braces. I am OK with using braces in all new code.

[iantaylor-NOAA]

@Rick-Methot-NOAA, it's indeed mysterious why that one occurrence was flagged and not all the others. I don't see an issue with only adding the additional braces if/when the warning appears in the future rather than throughout all new code.

@e-perl-NOAA, I tried to modify the workflow in dcda1ac to get the github action to produce the full list of warnings, even if the strings didn't match what was expected, by skipping the section where the R code parses the warnings. I think that would help with debugging to have a failed test but an artifact created that provided more detail. However, I was shortsighted in my fix and it still failed when it got to this line https://github.com/nmfs-ost/ss3-source-code/blob/main/.github/workflows/build-ss3-warnings.yml#L90 which relies on the file created by the R parsing step. Do you think it's worth further work to make this fix work in the future or is the codespaces debugging process you went through adequate and so it's not worth the time for this alternative?

[Rick-Methot-NOAA]

Note that I have stopped using make_ss_safe locally. Instead I created a makefile that includes all warnings because the best place to see these warnings is when the code is being developed.
Make_SS_warn.zip

[iantaylor-NOAA]

@Rick-Methot-NOAA, it makes sense to see the warnings locally. Can we just put the Make_SS_warn.bat into the repo (and remove from gitignore) so it's available for all and easier to track and keep updated?

[Rick-Methot-NOAA]

can do

[johnoel]

Not official releases, but the docker images for ADMB-13.2pre can be used for testing.

https://hub.docker.com/r/johnoel/admb-13.2/tags

[e-perl-NOAA]

Thanks @johnoel, I'll make a separate issue for testing that.

[e-perl-NOAA]

@Rick-Methot-NOAA and @iantaylor-NOAA do we feel good about this PR now? Can I go ahead and merge?

[iantaylor-NOAA]

Looks good to me. Sorry it's been such a long process.

[Rick-Methot-NOAA]

OK to merge.

[e-perl-NOAA]

@msupernaw