Fix -Wextra

[SoapGentoo]

Fixes for when building under -Wextra, particularly for users who want to compile their Rcpp code with -Wextra.

Checklist

• Code compiles correctly
• R CMD check still passes all tests
• Preferably, new tests were added which fail without the change
• Document the changes by file in ChangeLog

[eddelbuettel]

Thanks but please see the file Contributing.md -- prior discussion is always best before throwing a PR over the fence which touches eighteen files and comes from someone we never interacted with (as far as I can tell).

[eddelbuettel]

A priori, I am sympapthetic. I have used gcc and g++ for decades, and regularly use -Wall -pedantic. Both those options are mentioned repeatedly in (somewhat normative for R) manual Writing R Extensions -- but -Wetra is not. This extra option is also described as something that

enables a set of additional compiler warnings for code constructs that are not strictly errors but are questionable, prone to bugs, or hard to avoid.

so it is a little outside the norm. We are about to release 1.1.2 in out usual 'January and July' cadence but maybe we can look into this for the release thereafter, i.e. January 2027.

[SoapGentoo]

sure, I just thought it was a rather simple change, since it doesn't touch any real logic. Do you want me to close it or leave it open and rebase it later?

[eddelbuettel]

Can we maybe decompose it into individual warnings and among the team (with between us decades of gcc experience) see which may have merit and turn them on one by one? So yes maybe this is now the issue discussion we could have had.

You appear to be associated with gentoo. Does -Wextra get turned on by default for everything over there, or is this more your personal thing?

[SoapGentoo]

-Wextra enables -Wunused-parameter and -Wdeprecated-copy-with-user-provided-copy which get triggered: both fixes are in separate commits.

This is unrelated to Gentoo, this is something I'm seeing in our corporate codebase (where we do enable -Wextra everywhere)

[eddelbuettel]

I am quite religious about -Wall -pedantic, personally speaking. But I don't think I have seen -Wextra deployed widely. Happy to move towards it. Maybe we can do it can option at a time?

[SoapGentoo]

Sure, the biggest problem for the main codebase (not in the headers) is the [[Rcpp::export]] parsing: it would have to account for unused parameters there too, which is obviously tricky. Would you want to add code such that

// [[Rcpp::export]]
Rcpp::List foo(std::string a, std::string /*b*/) { ... }

works?

[eddelbuettel]

There should be none in the Rcpp package proper i.e. in src/ and inst/include. It will matter for client packages consuming Rcpp::compileAttributes() output. Here this only affects internal use in ephemeral files (e.g. unit tests) that should get not tickle the warning.

[kevinushey]

In principle I'm okay with this change, but IMHO it's not really about fixing -Wextra warnings, it's about whether we're willing to commit to doing this from now and into the future.

That probably also implies us building and testing Rcpp with -Wextra on CI, so we don't regress this in the future.

I also worry that there's some amount of divergence in what -Wextra means, based on the compiler used, its version, and so on -- since IIUC the set of compiler flags in -Wextra is itself a moving target.

My two cents...

[SoapGentoo]

yeah, just double-checked, they're fine (I saw the -Wcast-function-type-mismatch ones, those are obviously not fixable within Rcpp without R actually fixing them first). How do you want me to proceed?

@kevinushey oh it is, but -Wall is a moving target too, so...

[eddelbuettel]

.... which is a flag that R itself will complain about as 'not portable' (cf other discussion on -pedantic etc in WRE) if we added it to src/Makevars.

[Enchufa2]

It would have been nice to discuss these as separate issues. My two cents about the copy constructor.

In general, I prefer to solve warnings rather than just silencing them. Adding a default constructor does the latter, because it's something the compiler already does for us.

Having a custom assignment operator but not a copy constructor is what triggers the warning. So the question for me is whether we need a custom copy operator too, or if the assignment operator is needed at all. Correct me if I'm wrong, but if we remove it, the compiler generates a default one that delegates to the base class, which already handles the assignment nicely (i.e. shares the underlying SEXP, which has the tzone attribute already).

[Enchufa2]

As for -Wunused-parameter,

• Is this the result of a comprehensive search and therefore probably all cases? Or just the ones that popped as warnings in your pipeline?
• The [[Rcpp::export]] parser is pretty complex as it is, I don't think we want to support comments or attributes like [[maybe_unused]] for that matter. Unused parameters typically arise in the context of template specializations, but when a user exports a function to R, typically the function requires all the parameters. And in the very few edge cases, and if they want to silence those warnings, there are other ways to do so.

[SoapGentoo]

• This was from running GCC 15 and Clang 21 on the codebase with -Wextra.
• Agreed, I wouldn't try and put this in the parser either.

[eddelbuettel]

On balance, suggest to

• close this PR
• investigate warning by warning post 1.1.2 release
• ex ante, allowing Rcpp client packages to compile with -Wextra is good
• to be seen if our use of [[Rcpp::exports]] attributes throws a wrench

Comments?

[Enchufa2]

Agree to do this post 1.1.2 release. I would open an issue to track all this, and then we need:

• A PR to activate -Wextra, although probably this requires changes in your r-ci repo, right? Maybe exposing an option to activate this.
• A PR to remove the assignment constructors, probably with some more tests.
• A PR for the unused parameters. The current tests probably don't cover all instances, so this looks like the perfect task for the AI.

[eddelbuettel]

We'll deal with CI once we get to new PRs. For now, let's close this is neither the right time nor the right approach.

[SoapGentoo]

@eddelbuettel @Enchufa2 should I send in a PR with just parts of this PR?

[eddelbuettel]

@SoapGentoo Can we start with an issue and agree what we want changed and why before we get to a how? See the discussion above: -Wextra is simply not something everybody puts here.

I just added it on a lark on my box directly in src/Makevars. This seems to result in two sets of warnings from

• -Wcast-function-type
• -Wunused-parameter

(NB: I am on Ubuntu 26.04 and use the g++ 16 that comes with it ie 16.0.1 20260322.)

But as discussed, compiling the package itself does not hit all the code in it. We probably would have to catch a compilation under unit tests too to find more. Or rely on agents to scan the code and then review. That might suggest addressing it one warning at a time.

[SoapGentoo]

• -Wdeprecated-copy-with-user-provided-copy will likely trigger consuming code
• -Wunused-parameter will trigger in consuming code too.

Fixing both of these should be easy and catchable with those warnings. -Wcast-function-type is probably a bit too much in the weeds, given how function pointers need to be cast around R APIs.

[eddelbuettel]

Two points raised in your last comment:

a) We would need to go from "will likely" to actually do :)

b) Unless all warnings go away, why are we even doing this?

[SoapGentoo]

I still think making two of the warnings errors and fixing it is strictly better than the status quo, but if it isn't 100% guaranteed to fix the downstream issue for you, let's leave it then?

[eddelbuettel]

I think you misunderstood. Let me try to rephrase.

Adding -Wextra to Rcpp's own build was a simple step, and I would like for it to be quiet too. What comes up there are now-known and easily reproducible instances (but also only two for 'unused var'). R itself, and CRAN, and my users such as us often set extra warning so in principle we are for this. But as discussed, R and CRAN do not set -Wextra and do not set all possible flags that gcc/g++ offer. For example if we added alll the different cast warning hell breaks loose in R land because of the pervasive and of right now unavoidable doube-to-int casts in many places.

So I see several possible steps and goals as mentioned above. Patching only parts of what -Wextra alerts us to seems to be a bit of a non-goal.

I also said we should discuss this as an issue. Maybe we can start that if you, say, open an issue about -Wunused-parameter. Ideally, we would find a way to address this in client packages too so we should discuss that there.

But let's please also take a moment and stop now and not rapid-fire add even more posts before @Enchufa2 and @kevinushey have a chance to catch up and chime in.

[Enchufa2]

I think that the first task would be to be able to catch warnings from tests. As discussed, they have a pretty good coverage, and this would help us not only with this, but also with #1488 and future issues like that. I see two possible approaches:

1. Set verbose=TRUE in all sourceCpp, cppFunction calls and so on. Then postprocess the output and fail if warnings are found.
2. Just set -Werror, turning all warnings into errors.

I think the second is the cleanest. Once we set on a mechanism to add compilation flags (i.e. manually, or some variable exposed by r-ci to activate this and add -Werror automatically), we could further activate -Wextra to address what's reported here, -Wconversion-warnings, and also any -Wno-<whatever> that we might want to whitelist.

Then we'll have a pretty good picture of what user code might be triggering, and we can address those warnings.

[eddelbuettel]

Adjusting CI is fairly easy as the yaml is just source code. My simply have to set warnings as errors, eg by cat-ing into a file ~/.Rprofile or alike. We can do that, and I concur that our tests have decent coverage, and are easier to operationalise than all reverse dependencies. A branch may be suitable for this for now, and later maybe an CI file with an opt-in as this will likely get noisy.

Shall we create a new issue 'Aiming at -Wextra compliance' with suitable sub-issues?

[Enchufa2]

Shall we create a new issue 'Aiming at -Wextra compliance' with suitable sub-issues?

Sure!