[turbofan] Cleanup Schedule and related classes.

[turbofan] Cleanup Schedule and related classes.

- Move unit tests to schedule-unittests.cc.
- Remove pre-C++11 cruft.
- Fix some include weirdness.

R=mstarzinger@chromium.org

Committed: https://crrev.com/c9283148d1f9c81a23d6352138e84dd5b20f2952
Cr-Commit-Position: refs/heads/master@{#26220}

[Benedikt Meurer, 2015-01-22 12:05:48]

PTAL

[Michael Starzinger, 2015-01-22 12:15:29]

LGTM. But I have to say I am not a huge fan of the "auto" proliferation
everywhere. Is it really necessary? Does it really improve the situation?

https://codereview.chromium.org/864293002/diff/1/src/compiler/graph-visualize...
File src/compiler/graph-visualizer.cc (right):

https://codereview.chromium.org/864293002/diff/1/src/compiler/graph-visualize...
src/compiler/graph-visualizer.cc:573: for (auto predecessor :
current->predecessors()) {
remark: I know you all like the "auto" magic, but I would find "BasicBlock
predecessor" more readable. Just putting it out there, feel free to ignore me in
this regard. :)

https://codereview.chromium.org/864293002/diff/1/src/compiler/scheduler.cc
File src/compiler/scheduler.cc (right):

https://codereview.chromium.org/864293002/diff/1/src/compiler/scheduler.cc#ne...
src/compiler/scheduler.cc:1035: auto pred = block->predecessors().begin();
remark: Especially here, I have to think hard to figure out that "pred" is a
BasicBlock here. :/

[titzer, 2015-01-22 12:18:47]

titzer@chromium.org changed reviewers:
+ titzer@chromium.org

[titzer, 2015-01-22 12:18:48]

https://codereview.chromium.org/864293002/diff/1/src/compiler/schedule.h
File src/compiler/schedule.h (right):

https://codereview.chromium.org/864293002/diff/1/src/compiler/schedule.h#newc...
src/compiler/schedule.h:104: typedef Node* value_type;
What's this typedef for?

[titzer, 2015-01-22 12:24:59]

On 2015/01/22 12:15:29, Michael Starzinger wrote:
> LGTM. But I have to say I am not a huge fan of the "auto" proliferation
> everywhere. Is it really necessary? Does it really improve the situation?
> 
>
https://codereview.chromium.org/864293002/diff/1/src/compiler/graph-visualize...
> File src/compiler/graph-visualizer.cc (right):
> 
>
https://codereview.chromium.org/864293002/diff/1/src/compiler/graph-visualize...
> src/compiler/graph-visualizer.cc:573: for (auto predecessor :
> current->predecessors()) {
> remark: I know you all like the "auto" magic, but I would find "BasicBlock
> predecessor" more readable. Just putting it out there, feel free to ignore me
in
> this regard. :)
> 
> https://codereview.chromium.org/864293002/diff/1/src/compiler/scheduler.cc
> File src/compiler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/864293002/diff/1/src/compiler/scheduler.cc#ne...
> src/compiler/scheduler.cc:1035: auto pred = block->predecessors().begin();
> remark: Especially here, I have to think hard to figure out that "pred" is a
> BasicBlock here. :/

As per in-person discussion, my feeling on the auto is to only use it where it
is obvious from context what the type is:

for (auto block : x->blocks())  // clearly a Block of some kind
for (auto instr : x->instructions())  // clearly an Instruction of some kind

for (auto b : x->predecessors()) // less clear, better to make it explicit
for (BasicBlock* b : x->predecessors()) // now clear

[Benedikt Meurer, 2015-01-22 12:25:14]

On 2015/01/22 at 12:24:59, titzer wrote:
> On 2015/01/22 12:15:29, Michael Starzinger wrote:
> > LGTM. But I have to say I am not a huge fan of the "auto" proliferation
> > everywhere. Is it really necessary? Does it really improve the situation?
> > 
> >
https://codereview.chromium.org/864293002/diff/1/src/compiler/graph-visualize...
> > File src/compiler/graph-visualizer.cc (right):
> > 
> >
https://codereview.chromium.org/864293002/diff/1/src/compiler/graph-visualize...
> > src/compiler/graph-visualizer.cc:573: for (auto predecessor :
> > current->predecessors()) {
> > remark: I know you all like the "auto" magic, but I would find "BasicBlock
> > predecessor" more readable. Just putting it out there, feel free to ignore
me in
> > this regard. :)
> > 
> > https://codereview.chromium.org/864293002/diff/1/src/compiler/scheduler.cc
> > File src/compiler/scheduler.cc (right):
> > 
> >
https://codereview.chromium.org/864293002/diff/1/src/compiler/scheduler.cc#ne...
> > src/compiler/scheduler.cc:1035: auto pred = block->predecessors().begin();
> > remark: Especially here, I have to think hard to figure out that "pred" is a
> > BasicBlock here. :/
> 
> As per in-person discussion, my feeling on the auto is to only use it where it
is obvious from context what the type is:
> 
> for (auto block : x->blocks())  // clearly a Block of some kind
> for (auto instr : x->instructions())  // clearly an Instruction of some kind
> 
> for (auto b : x->predecessors()) // less clear, better to make it explicit
> for (BasicBlock* b : x->predecessors()) // now clear

SGTM

[Benedikt Meurer, 2015-01-22 12:29:21]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2015-01-22 12:30:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/864293002/20001

[commit-bot: I haz the power, 2015-01-22 13:01:08]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-22 13:01:23]

Patchset 2 (id:??) landed as
https://crrev.com/c9283148d1f9c81a23d6352138e84dd5b20f2952
Cr-Commit-Position: refs/heads/master@{#26220}

[Benedikt Meurer, 2015-01-22 13:48:02]

https://codereview.chromium.org/864293002/diff/1/src/compiler/graph-visualize...
File src/compiler/graph-visualizer.cc (right):

https://codereview.chromium.org/864293002/diff/1/src/compiler/graph-visualize...
src/compiler/graph-visualizer.cc:573: for (auto predecessor :
current->predecessors()) {
On 2015/01/22 12:15:29, Michael Starzinger wrote:
> remark: I know you all like the "auto" magic, but I would find "BasicBlock
> predecessor" more readable. Just putting it out there, feel free to ignore me
in
> this regard. :)

Done.