Page MenuHomePhabricator

Apply clang-tidy fixes for misc-unused-parameters to MLIR (NFC)
Needs ReviewPublic

Authored by mehdi_amini on Jan 2 2022, 2:09 PM.

Details

Diff Detail

Event Timeline

mehdi_amini created this revision.Jan 2 2022, 2:09 PM
mehdi_amini requested review of this revision.Jan 2 2022, 2:10 PM
bondhugula added inline comments.Jan 3 2022, 7:39 PM
mlir/lib/Dialect/Affine/IR/AffineOps.cpp
148

Isn't there a readability benefit to having the parameter named here albeit unused? Or:

..., bool /*woudBeCloned*/

Do we need to drop the arg names on interface method implementations also albeit unused? The comment on wouldBeCloned above can also be dropped if we are dropping this.

aartbik added inline comments.Jan 5 2022, 1:00 PM
mlir/lib/Dialect/Affine/IR/AffineOps.cpp
148

I am also a bit old-fashioned that I like to see a parameter name in the *implementation* of a method (not the declaration per se), but if this is the style going forward, am okay with the change of course.

jpienaar added inline comments.
mlir/lib/Dialect/Affine/IR/AffineOps.cpp
148

Agreed, I think dropping biases to folks using IDEs & results in an indirection when reading the code (and indeed then keeping the comment above is even less defensible as those same IDEs would provide it too and contrary to the parameter names, it can't be automatically updated). So not particularly fond of this style change.

mehdi_amini added inline comments.Jan 13 2022, 11:11 AM
mlir/lib/Dialect/Affine/IR/AffineOps.cpp
148

I also prefer the variable name available than this variant.

I didn't get your point in the parenthesis here Jacques, can you expand?

rriddle added inline comments.Jan 13 2022, 11:13 AM
mlir/lib/Dialect/Affine/IR/AffineOps.cpp
148

+1 on keeping the variable names. I'm okay with the other patch (given that there aren't that many hits in practice), though I would like clang-tidy to alert on mismatch.