Page MenuHomePhabricator

Refactoring with range-based for, NFC
ClosedPublic

Authored by chenwj on Apr 30 2017, 7:27 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

chenwj created this revision.Apr 30 2017, 7:27 AM
chenwj added reviewers: MatzeB, kparzysz, spatel.
chenwj edited subscribers, added: llvm-commits; removed: MatzeB, kparzysz, spatel.
spatel edited edge metadata.May 1 2017, 2:58 PM

IMO, this is going beyond LLVM's normal 'auto' usage prefs:
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

If you can convert it to a range-loop, then you should specify the range element type including 'const' where appropriate. So something like this:

for (const SDValue &Op : NI->op_values())

Some of the diffs use regular loops and only replace the iterator type with an 'auto'. That should be a separate NFC patch. But it's not clear to me if that also goes too far into 'auto'.

chenwj added a comment.EditedMay 2 2017, 5:21 AM

IMO, this is going beyond LLVM's normal 'auto' usage prefs:
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

If you can convert it to a range-loop, then you should specify the range element type including 'const' where appropriate. So something like this:

for (const SDValue &Op : NI->op_values())

Some of the diffs use regular loops and only replace the iterator type with an 'auto'. That should be a separate NFC patch. But it's not clear to me if that also goes too far into 'auto'.

By beyond, you mean those originally were const_iterator or all of them? FWIK, to make auto as const_iterator, we need the container provides cbegin()/cend(). Or you know other way to make it happen?

Thanks.

spatel added a comment.May 2 2017, 6:25 AM

IMO, this is going beyond LLVM's normal 'auto' usage prefs:
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

If you can convert it to a range-loop, then you should specify the range element type including 'const' where appropriate. So something like this:

for (const SDValue &Op : NI->op_values())

Some of the diffs use regular loops and only replace the iterator type with an 'auto'. That should be a separate NFC patch. But it's not clear to me if that also goes too far into 'auto'.

By beyond, you mean those originally were const_iterator or all of them? FWIK, to make auto as const_iterator, we need the container provides cbegin()/cend(). Or you know other way to make it happen?

Thanks.

By "beyond LLVM's normal 'auto' usage prefs", I mean that we would generally prefer this:

for (SDep &Pred : SU->Preds) {

instead of this:

for (auto &Pred : SU->Preds) {

because it makes the code clearer to state the element type. That's the first diff in this patch.

The const-ness is a separate issue. If you can use this:

for (const SDep &Pred : SU->Preds) {

Then that is even better because a reader of the code can immediately know that Pred is not being written in that loop.

FWIW, I don't think I've ever made changes to these files, so if there's some reason to differ from the normal LLVM usage here, let me know.

chenwj updated this revision to Diff 97644.May 3 2017, 7:47 AM

Use SDep & or const SDep & rather than auto.

chenwj added a comment.May 3 2017, 7:48 AM

@spatel I update the code. please take a look, thanks. :-)

kparzysz added inline comments.May 3 2017, 8:26 AM
lib/CodeGen/SelectionDAG/ResourcePriorityQueue.cpp
495 ↗(On Diff #97644)

This could also be a range-for.

kparzysz added inline comments.May 3 2017, 8:30 AM
lib/CodeGen/SelectionDAG/ScheduleDAGFast.cpp
750 ↗(On Diff #97644)

Same here: for (const SDNode *U : Glue->uses()).

chenwj updated this revision to Diff 97813.EditedMay 4 2017, 5:38 AM
chenwj marked 2 inline comments as done.

@kparzysz All done. :-)

kparzysz added inline comments.May 4 2017, 6:05 AM
lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
523 ↗(On Diff #97813)

Seems like the autos in this file have not been converted to types. I somehow missed this the last time.

chenwj updated this revision to Diff 97816.May 4 2017, 6:33 AM
This comment was removed by chenwj.
kparzysz edited edge metadata.May 4 2017, 6:35 AM

@kparzysz Fixed!

You only fixed one, my comment was about the whole file.

chenwj updated this revision to Diff 97817.EditedMay 4 2017, 6:40 AM

@kparzysz There are two places I think using auto should be okay, replacing other auto with type.

kparzysz accepted this revision.May 4 2017, 6:42 AM

LGTM.

This revision is now accepted and ready to land.May 4 2017, 6:42 AM
chenwj added a comment.May 4 2017, 6:43 AM

I have no right to commit, need someone's help. :-)

This revision was automatically updated to reflect the committed changes.