Page MenuHomePhabricator

Demo fix for misc-unused-parameters (WIP)
Needs ReviewPublic

Authored by mehdi_amini on Dec 23 2021, 2:20 PM.

Diff Detail

Event Timeline

mehdi_amini created this revision.Dec 23 2021, 2:20 PM
mehdi_amini requested review of this revision.Dec 23 2021, 2:20 PM

This one is a no vote from me:

  • inline comments /*foo*/ are annoying to edit
  • function signatures with a /*bunch*/ /*of*/ /*commented*/ /*out*/ arguments is ugly
  • +1 for automatically removing unused function arguments though

I find the approach in
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions useful: omit what is obvious, make explicit and named what is not (well if nowhere used, not staged, and no planned use, then removing better). I like it being explicit when unused and having local context of names. It does put the load on whomever updates the function (to use variable) or signature (if one adds params) to edit, and that's good IMHO (it can get out of date though, as nothing flags it until actually used). Updating frequency is unknown (but if one clang-rewrite it, it is simple to mechanically do). There are tradeoffs, but documented locally is clearer to me.

  • inline comments /*foo*/ are annoying to edit

Code is written once, read multiple times :)
And how many times would you actually need to edit the comment though? (that is, how often will you introduce a new use of an argument unused until now?).

  • +1 for automatically removing unused function arguments though

We would need to patch clang-tidy to add an option to not comment out and only remove, it does not exist right now.

I'm +1 on removing unused arguments, but the argument name comments still feel a bit annoying to write/interact with. Though it doesn't look like there are too many of them, so maybe not that bad in the long run. Are the comment params something that clang-tidy can flag for mismatch? i.e. if the declaration of the argument is changed, does the comment get flagged for being out-of-date?

I'm +1 on removing unused arguments, but the argument name comments still feel a bit annoying to write/interact with.

D116488 for an alternative.

Though it doesn't look like there are too many of them, so maybe not that bad in the long run. Are the comment params something that clang-tidy can flag for mismatch? i.e. if the declaration of the argument is changed, does the comment get flagged for being out-of-date?

Maybe not, but we can likely implement this as well :)

I'm +1 on removing unused arguments, but the argument name comments still feel a bit annoying to write/interact with.

D116488 for an alternative.

D116501 for the application with the new option

I'm +1 on removing unused arguments, but the argument name comments still feel a bit annoying to write/interact with. Though it doesn't look like there are too many of them, so maybe not that bad in the long run. Are the comment params something that clang-tidy can flag for mismatch? i.e. if the declaration of the argument is changed, does the comment get flagged for being out-of-date?

That seems easy to add and mechanical to update if it isn't there. I think the workflow is probably, copy the signature you want to implement, use what you do, run clang-tidy (so you never really have to even write these). Then upon updates we rerun clang-tidy. Now I do run clang-tidy and clang-format often, so no change in my workflow really :-) this inconvenience is easy to address in the tooling.

Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 2:50 PM