This is an archive of the discontinued LLVM Phabricator instance.

[NFC][LoopIdiom] Make for loops more readable
ClosedPublic

Authored by yurai007 on Oct 19 2021, 7:53 AM.

Diff Detail

Event Timeline

yurai007 created this revision.Oct 19 2021, 7:53 AM
yurai007 requested review of this revision.Oct 19 2021, 7:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 19 2021, 7:53 AM
yurai007 added inline comments.Oct 19 2021, 7:57 AM
llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
667

That could be simplified a bit with 'seq' but extra headers need to included, so decided leave it as it is.

craig.topper added inline comments.Oct 19 2021, 10:06 AM
llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
667

I don't find this more readable.

I'm not sure what the L in LU was supposed to do. long is 32-bits on 64-bit Windows so if you were trying to make it the same size as size_t to match the return type of SL.size(), it didn't work.

yurai007 added inline comments.Oct 19 2021, 11:27 AM
llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
667

I don't find this more readable.

Well, it's less verbose but indeed maybe not really more readable.

I'm not sure what the L in LU was supposed to do. long is 32-bits on 64-bit Windows so if you were trying to make it the same size as size_t to match the return type of SL.size(), it didn't work.

Yes, that was my intention but I failed miserably. I will go back to previous loop version without auto.

nikic requested changes to this revision.Oct 19 2021, 12:53 PM
nikic added a subscriber: nikic.

Please limit your use of auto to only the cases explicitly listed in https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable, i.e. if the type is already otherwise mentioned (cast/dyn_cast) or abstracted anyway (iterator typedefs). There is more nuance to this policy, but as you tend to heavily overuse auto I will recommend that you stick to these explicitly allowed cases only.

This revision now requires changes to proceed.Oct 19 2021, 12:53 PM
yurai007 updated this revision to Diff 380952.Oct 20 2021, 7:42 AM
yurai007 edited the summary of this revision. (Show Details)
yurai007 marked an inline comment as done.

Addressed all comments.

nikic accepted this revision.Oct 20 2021, 8:03 AM

LGTM

This revision is now accepted and ready to land.Oct 20 2021, 8:03 AM
This revision was automatically updated to reflect the committed changes.