Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce const-related warnings #7338

Merged

Conversation

mwoehlke-kitware
Copy link
Contributor

Description

Don't remove const-ness where there is no need to do so, and use const_cast when it is necessary (particularly in qgui). Refactor plotting to accept immutable data, and refactor some data marshaling to pass immutable points rather than mutable points to avoid unnecessary adaptation.

This significantly reduces -Wcast-qual warnings. Also turn that on by default to reduce the chances of regressions.

(Individual commits may have more details.)

Which blocks/areas does this affect?

This should have negligible to no impact on code generation, since not dropping const when not needed should have no effect, and replacing C-style casts with const_cast definitely should have no effect. I don't expect changing vector<double*> to vector<const double*> to have any effect either, as the vector still holds a mutable pointer in both cases. (Note that we're talking about mutable pointers to (im)mutable data; that is, the pointers are always mutable, the change is whether what they point to is const. I can readily imagine vector<T> and vector<T const> having different codegen, but I don't know any reason why vector<T*> and vector<T const*> would differ.) The usage never needed mutable data, so should not be affected.

Testing Done

Compiled locally with -Werror=cast-qual on Fedora 39 and ran ctest.

Checklist

I suspect the documentation is more user-facing and isn't affected by the changes to the plotting API, but as I'm not sure, I left that unchecked. If there's any documentation changes needed, I'd appreciate if someone can point me in the direction of what to change; thanks!

@willcode willcode marked this pull request as draft May 14, 2024 11:04
@willcode
Copy link
Member

Converted to draft - looks like a work in progress.

@mwoehlke-kitware mwoehlke-kitware force-pushed the reduce-const-related-warnings branch 3 times, most recently from b8c13e4 to 79d4ca2 Compare May 14, 2024 21:24
@mwoehlke-kitware mwoehlke-kitware marked this pull request as ready for review May 14, 2024 22:43
@mwoehlke-kitware
Copy link
Contributor Author

looks like a work in progress

There were a few warnings left in modules I didn't previously have building locally. Should be fixed now.

@willcode
Copy link
Member

Fedora 40 compiler seems to pick up a few more:

https://github.com/gnuradio/gnuradio/actions/runs/9086403135/job/24972029828?pr=7338#step:6:528

Don't remove const-ness where there is no need to do so, and use
const_cast when it is necessary. This significantly reduces -Wcast-qual
warnings.

Also, in jack_source, don't add const-ness when not necessary only to
have to cast it away.

Note that the changes in qtgui are incomplete. In particular, this does
not introduce any const_casts in qtgui, nor does it attempt to address
dropping const from point data. The former are a bit more questionable,
while the latter requires more invasive refactoring; accordingly, each
will be addressed in separate commits.

Signed-off-by: Matthew Woehlke <matthew.woehlke@kitware.com>
Change C-style casts in qtgui that cast away const-ness to use
const_cast instead. This makes it easier to find such casts, and more
obvious that something potentially questionable is happening.

Signed-off-by: Matthew Woehlke <matthew.woehlke@kitware.com>
Refactor plotting to accept immutable data, and refactor some data
marshaling to pass immutable points rather than mutable points to avoid
unnecessary adaptation.

Signed-off-by: Matthew Woehlke <matthew.woehlke@kitware.com>
The previous commits should fix the discarded qualifiers warnings. Turn
on -Wcast-qual to reduce the chances of new ones being introduced.

Signed-off-by: Matthew Woehlke <matthew.woehlke@kitware.com>
@mwoehlke-kitware
Copy link
Contributor Author

Fedora 40 compiler seems to pick up a few more

Interesting!

/__w/gnuradio/gnuradio/gr-blocks/lib/wavfile_sink_impl.cc:267:24: error: cast from 'const void *' to 'float *' drops const qualifier [-Werror,-Wcast-qual]
  267 |     auto in = (float**)&input_items[0];
      |                        ^

This definitely looks legitimate, but it seems GCC 13.2.1 doesn't detect it. Probably because it's a cast of void** rather than the more typical void*? (Given that the warning text seems to be bugged, that seems somewhat likely.)

Anyway, hopefully fixed! (Also, hopefully when you said "few" you really meant "one", although since it seems I need CI to find these, I guess we won't know for certain until it passes CI.)

@willcode
Copy link
Member

willcode commented May 15, 2024

Looks good. The other failures are unrelated. I just need to take a little time to read all the changes.

@mwoehlke-kitware
Copy link
Contributor Author

No hurry!

FYI, this does make breaking changes to:

  • [fec] generic_{en,de}coder::generic_work (existing overloads will break)
  • [qtgui] *DisplayPlot::plotNewData (breaking argument type change)
  • [qtgui] *UpdateEvent::get*Points (return type changed)

I'm not sure if any of those are "user visible", or if you care, but something to keep in mind when considering whether or not to back-port.

@willcode
Copy link
Member

Thanks, I'll make sure to look carefully at those. For 3.10 (really for the rest of the life of 3.X) anything that breaks backward compatibility probably will not be merged onto main, as we have no plans to make a new non-backward-compatible 3.X release. There's some gray area there if it's something we're pretty sure is used only in-tree.

The ones you mention are probably only used in-tree .

@willcode willcode added Hold Backport Hold off on backport and removed Needs Action labels May 21, 2024
@willcode willcode merged commit 9d77ad1 into gnuradio:main May 21, 2024
14 of 16 checks passed
@willcode
Copy link
Member

There is some possibility that an OOT subclasses the en/decoder but it seems unlikely enough that we can merge to main and see what happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport-3.10 Should be backported to 3.10 Cleanup CMake Hold Backport Hold off on backport
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants