search

NOAA-FIMS / FIMS

The repository for development of FIMS
https://noaa-fims.github.io/FIMS/
GNU General Public License v3.0
16 stars 9 forks source link

[Bug]: Resolve C++ compile warnings #351

Closed ChristineStawitz-NOAA closed 1 year ago

ChristineStawitz-NOAA commented 1 year ago

Describe the bug

We should get rid of the warnings and notes that happen at compilation in MQ

To Reproduce

devtools::load_all()

Expected behavior

Expect no warnings

Screenshots

No response

Which OS are you seeing the problem on?

No response

Which browser are you seeing the problem on?

No response

Which version of FIMS are you seeing the problem on?

No response

Additional Context

No response

Code of Conduct

msupernaw commented 1 year ago

The -w flag removes all warnings on Unix and I believe Linux platforms. Not sure what flag is required for MakeVars.win.

Andrea-Havron-NOAA commented 1 year ago

@ChristineStawitz-NOAA and @msupernaw, should we be editing the Makevars.win file to ignore the warnings or fixing them? I think most are coming from a comparison between types size_t and int, e.g.: for (int j = 0; j < nages; j++) where nages is size_t

We could either declare j as size_t or cast nages to int inside the for loop expression to fix all the warnings.

ChristineStawitz-NOAA commented 1 year ago

Are the only warnings size_t vs int? If so I think we should fix. I thought when I filed this issue there were warnings that we didn't want to fix, but maybe those are gone now?

Thanks!

Andrea-Havron-NOAA commented 1 year ago

There are two other warnings:

msupernaw commented 1 year ago

@Andrea Havron - NOAA Federal @.***> We should fix warnings about mismatched types and other minor things in our code. After that, we can silence warnings for Eigen using the -w flag for g++/clang++/MinGW.

On Wed, Aug 2, 2023 at 11:49 AM Andrea-Havron-NOAA @.***> wrote:

There are two other warnings:

  • rcpp_population: for (size_t i = 0; i < log_init_naa.size(); i++): warning: comparison of integer expressions of different signedness: 'size_t{aka 'long long unsigned int and 'R_xlen_t{aka 'long long int-Wsign-compare
  • data_object: size_t jmax;: warning: fims::DataObject::jmaxl be initialized after [-Wreorder

— Reply to this email directly, view it on GitHub https://github.com/NOAA-FIMS/FIMS/issues/351#issuecomment-1662467018, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFUSECOOCTZGFJTN7Z3S3TXTJZHXANCNFSM6AAAAAAXEMGEMU . You are receiving this because you were mentioned.Message ID: @.***>

-- Matthew Supernaw Scientific Software Developer National Oceanic and Atmospheric Administration Office Of Science and Technology NOAA Fisheries | U.S. Department of Commerce Phone 248 - 396 - 7797

ChristineStawitz-NOAA commented 1 year ago

I think the data_object warning is when you change the orders of when jmax is initialized, maybe if the documentation has it in a different order than the functor?

msupernaw commented 1 year ago

You need to change the dimension member as the first thing initialized in DataObject. For example, you should change DataObject(size_t imax) : imax(imax), dimensions(1) to DataObject(size_t imax) : dimensions(1), imax(imax). You'l need to do this for all constructors.

Andrea-Havron-NOAA commented 1 year ago

Thanks! For the rcpp_population warning, should we cast log_init_naa.size() as size_t? log_init_naa is declared as an Rcpp::NumericVector

msupernaw commented 1 year ago

@Andrea-Havron-NOAA Yes, I think so. The Rcpp documentation says log_init_naa.size() returns a R_xlen_t type, which is typdefed here as either a ptrdiff_t or an int depending on the preprocessing macro. Both are signed integers.

ChristineStawitz-NOAA commented 1 year ago

I found this issue on it in Rcpp..https://github.com/RcppCore/Rcpp/pull/811

It looks like R_xlen_t type is not signed - is it possible turning the index to an int will fix this since an int is signed?

msupernaw commented 1 year ago

@ChristineStawitz-NOAA According to Rinternals.h, R_xlen_t is definitely signed. It's typedef'd as a ptrdiff_t or int and both are signed. If the documentation is not correct, the best way to tell is to print typeid(x).name() to standard out. If "m" is returned, it's a size_t, if "i" is returned, it's an integer.

msupernaw commented 1 year ago

Hmm, a quick test on my computer says it's a long!

Rcpp::NumericVector nv; Rcpp::Rcout<<"vector type: "<<typeid(nv.size()).name()<<std::endl;

Output: vector type: l

ChristineStawitz-NOAA commented 1 year ago

sorry typo above, I meant to say it is signed, not that it isn't signed.