Remove explicit call to C++11 and various cleanups and safety/efficiency enhancements

[aadler]

See the commit notes but in order of decreasing importance:

1. Sets C++17 as (will be) requested by CRAN
2. Removed quoting C function name (and PACKAGE) in .Call since names are already passed due to forced registration
3. The above allows adding R_forceSymbols in the init for added safety
4. Removes unnecessary paste calls inside stop calls in nlopt.r
5. Uses safer seq_along than 1/2:length(x)
6. Updates CITATION to use bibentry as per ne CRAN request
7. Makes citation align more closely to https://nlopt.readthedocs.io/en/latest/Citing_NLopt/
8. Whitespace cleanup around some delimiters in nlopt.r
9. Remove comment in my entry in DESCRIPTION as if this PR is accepted I'll have done more than adjust the tests :)

[eddelbuettel]

Nice. I started that too for my packages, so far one it. I am actually torn over whether I think this is a good policy or not because there are still so many old systems (hello HPC users!) with older compilers. But of me thinks we may want to push back, part of me knows how hard it is to get change in once the wheels are in motion...

And I noticed the old .Call() pattern too. All good!

PS You want C++17 in the Subject though :wink:

[eddelbuettel]

Another issue is that we may not want C++17 -- we may just be better off not setting a standard. As I just wrote in RcppParallel (while Kevin independently made just that change):

One thing I have been wondering, though, is whether or not we should set a CXX_STD. "Way back when" we had to as the then-default compiler was insisting on C++98. Now we have modern C++ by default, and adding this feels like adding a constraint. Should we just leave it open? (IIRC) R 4.0.0 turned on C++11, R 4.1.0 turned to C++14, R 4.3.0 will turn on C++17. Good enough?

[aadler]

@eddelbuettel @astamm true, but WRE for R-devel does say "Different versions of R have used different default C++ standards, so for maximal portability a package should specify the standard it requires". Then again, it's not that C++17 is required but that C++11 is old. I think the next release of Rtools43 is also going to default to C++17 for what it is worth (See R Daily News of 2023-01-26 and 2023-01-27).

All that being said, I think leaving it out is fine so long as there are no explicit reliances on C++17 features, which I believe there are not.

[eddelbuettel]

That is all true but it is also true that nloptr, just like many other packages, will most likely work with C++11 and C++14 and C++17. So there is no 'require' here. I am thinking I will remove the requirement / standards setting from my packages. (This here is Aymeric's so I am just piping in from the side.)

[eddelbuettel]

I tested under r-devel: no setting a C++ standard does not trigger the warning one gets when picking C++11. So I think I am just going to remove it. May need to sleep over it once or twice more, and think it over. But it currently strikes me as the better approach.

[aadler]

I think you are correct, @eddelbuettel, since nloptr isn't relying on anything special in C++17, I removed reference to CXX from all three Makevars as discussed. The defaults are going to be 14 or 17 pretty soon anyway, so we should be good. Thanks.

[aadler]

Hi @astamm. SInce you haven't responded yet, I am taking the opportunity to do even more cleanup and tweaking. The commits should be self explanatory, together with the conversation with @eddelbuettel. So far, everything passes the 51 tests. However, as an old-time R curmudgeon, I do not have roxygen installed, so it would be very valuable if you would allow the workflow tests to proceed.

There are a couple of other changes I would like to make, but they would 1) be a more serious styleguide change or 2) require better testing, so I would appreciate your input.

1. Does it matter to you to more strongly enforce an 80 or 120 character limit? Personally, I am an 80 character devotee but I don't want to go through refactoring all the code where it makes no functional difference, only æsthetic, if you would not want it as such.
2. I think some of the calls can be a hair more efficient. For example, in global.R (line 185) I think hin <- function(x) (-1) * .hin(x) could be replaced by hin <- function(x) -.hin(x) but I wouldn't want to try it without a test suite.
3. Speaking of which, the coverage is only about 50%. I'm happy to write more tests when I have time

Thanks.

[astamm]

Thanks a lot @aadler. I agree with both of you about your earlier discussion @aadler @eddelbuettel. And I am happy with the further changes you suggest (both the aesthetics one and the efficiency one). I actually took over Jelmer for the maintenance and wanted to do some code reformatting / refactoring myself for quite some times so I am perfectly aligned with your ideas. I'll accept all the help I can get so if you find time to write up more tests, that will also be super appreciated. I'll try to invest some time into it as well in the near future.

[aadler]

OK. I clearly did too much in one PR. It looks like it may be due to the is.na --> anyNA, but in order to make things cleaner, I am going to close this pull request and start providing simpler ones so that this does not happen again. Is that ok with you @astamm?

[astamm]

Absolutely!