Page MenuHome

Clang-Tidy: enable readability-non-const-parameter
ClosedPublic

Authored by Jacques Lucke (JacquesLucke) on Jul 3 2020, 7:01 PM.

Details

Summary

Search for Mute readability-non-const-parameter warning from clang-tidy. to see
some cases, where we don't want const (yet), but do get a warning.

Diff Detail

Repository
rB Blender

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Jul 3 2020, 7:01 PM
Jacques Lucke (JacquesLucke) created this revision.

Are those false-positives? Doesn't seem to be THAT many of them, and warning can be silenced using // NOLINT at the end of the line.

  • cleanup by using NOLINT

Yes, these are false positives.

In some cases, the parameter could be const, but the function is passed
as callback to another function that expects a non-const parameter.
Changing the signature does not work, because other callback functions
actually modify the parameter.

In other cases, clang-tidy does not seem to notice that the parameter
is assigned to a non-const variable. So if the parameter was const,
we would have to cast away the constness later on. In C++ we might be
able to put the const on the other side of the *, but that did not
work here.

To me it still feels like going const with some NOLINT exceptions is something we benefit from. What do you think?

To me it still feels like going const with some NOLINT exceptions is something we benefit from. What do you think?

+1 on more const. I think NOLINT is fine, if we can make it as likely as possible that no other warnings are silenced by it (i.e. by making sure that the lines that are marked as such are as small/simple as possible).

This should probably go hand in hand with an addition to the style guide, stating that if NOLINT is used, it should always come with a comment that explains why the linter is disabled for that line, and why that is the correct thing to do there.
In the cases in this patch (at least the ones I just looked at), this would mean a comment like "Implements the signature expected by WM_jobs_callbacks()", or even better, "Implements signature of wm_jobs_start_callback" (where wm_jobs_start_callback is a hypothetical typedef for that function signature).

I can replace most NOLINT with a const. Depending the case, the consequence is either that we would have to cast away the constness later, or that we have to cast the function to a different signature when it is used as callback. Both of these don't seem any better than just adding NOLINT there.

@Sybren A. Stüvel (sybren), sounds good to me.

  • Add comments explaining why NOLINT is necessary
This revision was not accepted when it landed; it landed in state Needs Review.Mon, Jul 13, 11:28 AM
This revision was automatically updated to reflect the committed changes.