Clean up encoding-related code. NFC. #19717
Conversation
Fixing misc issues before modifying the surrounding code. Signed-off-by: Jakub Kuderski <jakub@nod-labs.com>
kuhar
requested review from
hanhanW,
MaheshRavishankar and
IanWood1
as code owners
| auto addRhsOp = addRhs.getDefiningOp(); | ||
| Value addLhs = addOp->getOperand(0); | ||
| Value addRhs = addOp->getOperand(1); | ||
| Operation *addLhsOp = addLhs.getDefiningOp(); |
There was a problem hiding this comment.
good IDEs/LSPs really help with this stuff - readability without one is important (code reviews/those just peeking at source/etc) but anyone modifying the code can reasonably be expected to have a working development environment or this isn't their biggest issue - tblgen-produced op/attr defs are :P
Readability could be improved with the * - I prefer auto * for pointers where the type is obvious instead of just auto as due to MLIR's weird mix of value and pointer types. But we generally don't want to churn code unless it helps or is making things more consistent. This PR on a single file doesn't much matter but it's a game of whack-a-mole as it'd be just as valid for someone to come in and turn Operation *op = ...getOp() into auto *op = and we don't want to have randomness. I'm mostly focused on consistency - if you can get upstream to use Operation * when calling a getDefiningOp() and all other cases where something is an op then we can match that style - but otherwise it's just inconsistency (file A is different than file B in the same directory arbitrarily). 75% of getDefiningOp calls upstream and in IREE use auto of some kind. If you think there's consensus upstream that auto is misused on a line with "Op" in it twice then we should do a large scale change upstream and in our code and make that a rule (I'd be for that - just don't like the inconsistency :).
There was a problem hiding this comment.
I was under the impression that we are following the llvm coding standards for the most parts, which cover the use of auto: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
Don’t “almost always” use auto, but do use auto with initializers like cast(...) or other places where the type is already obvious from the context.
To me these rules make sense and I find it useful to see types even without an IDE. This comes up quite often when sharing links to specific lines of code with other folks. And probably most importantly, in code reviews.
Fixing misc issues before modifying the surrounding code.