Page MenuHomePhabricator

Sample some of the new enabled clang-tidy fixes (WIP)
Needs RevisionPublic

Authored by mehdi_amini on Dec 23 2021, 1:29 PM.

Diff Detail

Event Timeline

mehdi_amini created this revision.Dec 23 2021, 1:29 PM
mehdi_amini requested review of this revision.Dec 23 2021, 1:29 PM
rriddle requested changes to this revision.Dec 23 2021, 1:34 PM
rriddle added inline comments.
mlir/lib/Dialect/StandardOps/Transforms/DecomposeCallGraphTypes.cpp
64

I'm personally not much of a fan of needing to do this.

This revision now requires changes to proceed.Dec 23 2021, 1:34 PM
rriddle added inline comments.Dec 23 2021, 1:36 PM
mlir/lib/Dialect/StandardOps/Transforms/DecomposeCallGraphTypes.cpp
64

Doesn't really seem to have any benefit and just adds annoyance

mehdi_amini retitled this revision from Sample some of the new enabled clang-tidy fixes (NFC) to Sample some of the new enabled clang-tidy fixes (WIP).Dec 23 2021, 1:37 PM
mehdi_amini edited the summary of this revision. (Show Details)
mehdi_amini added inline comments.Dec 23 2021, 1:39 PM
mlir/lib/Dialect/StandardOps/Transforms/DecomposeCallGraphTypes.cpp
64

I wouldn't write it myself, but what if the tool does it for you?
I find it adds to the readability because you immediately know that an operand isn't used anywhere in the function body.

Anyway, that's why I opened this revision, to evaluate the various checks, in this case it is this one: https://clang.llvm.org/extra/clang-tidy/checks/misc-unused-parameters.html

I could also open one revision per check if you'd like: now I scripted it all :)

rriddle added inline comments.Dec 23 2021, 1:43 PM
mlir/lib/Dialect/StandardOps/Transforms/DecomposeCallGraphTypes.cpp
64

I think if a tool automatically did it (ala clang-format) I'd be +1. The main part I'd be annoyed with is having to write it/interact with it by hand. (though I generally rely on an IDE to tell me uses of a parameter)

rriddle added inline comments.Dec 23 2021, 1:45 PM
mlir/lib/Dialect/StandardOps/Transforms/DecomposeCallGraphTypes.cpp
64

If it isn't annoying to do, I think one revision per check(or group of related checks) would be good. Would allow much easier discussion for specific checks.

Mogball added inline comments.Dec 23 2021, 2:09 PM
mlir/lib/Dialect/StandardOps/Transforms/DecomposeCallGraphTypes.cpp
64

Even if it was done automatically, if I suddenly need a function argument, deleting the inline /* */ comments is very annoying. Also, I can't imagine the inferReturnTypes definitions will look great with this, because most of the time only 1 or 2 of the arguments are used (operands and attributes).

mlir/lib/Dialect/StandardOps/Transforms/FuncConversions.cpp
149

+1

mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
51

Yeah I think this looks terrible.

mlir/test/lib/Dialect/Affine/TestLoopPermutation.cpp
35

What's this one?