Move RMA::Label out of the class, so it can be forward declared.

Move RMA::Label out of the class, so it can be forward declared.

R=bmeurer@chromium.org, mstarzinger@chromium.org
BUG=chromium:508898
LOG=Y

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

[vogelheim, 2015-11-27 13:59:40]

Description was changed from

==========
Move RMA::Label out of the class, so it can be forward declared.

R=bmeuerer@chromium.org, mstarzinger@chromium.org
BUG=
==========

to

==========
Move RMA::Label out of the class, so it can be forward declared.

R=bmeurer@chromium.org, mstarzinger@chromium.org
BUG=
==========

[vogelheim, 2015-11-27 13:59:41]

vogelheim@chromium.org changed reviewers:
+ bmeurer@chromium.org
- bmeuerer@chromium.org

[vogelheim, 2015-11-27 14:00:28]

Please take a look.

https://codereview.chromium.org/1477413002/diff/1/test/cctest/compiler/test-b...
File test/cctest/compiler/test-branch-combine.cc (left):

https://codereview.chromium.org/1477413002/diff/1/test/cctest/compiler/test-b...
test/cctest/compiler/test-branch-combine.cc:16: typedef
RawMachineAssembler::Label MLabel;
I removed all the typedefs, since RawLabel -> MLabel doesn't seem like much of a
gain. Let me if you disagree.

[vogelheim, 2015-11-27 14:33:26]

Description was changed from

==========
Move RMA::Label out of the class, so it can be forward declared.

R=bmeurer@chromium.org, mstarzinger@chromium.org
BUG=
==========

to

==========
Move RMA::Label out of the class, so it can be forward declared.

R=bmeurer@chromium.org, mstarzinger@chromium.org
BUG=chromium:508898
LOG=Y
==========

[Michael Starzinger, 2015-11-27 17:10:27]

LGTM, thanks, just one super-OCD-nit. :)

https://codereview.chromium.org/1477413002/diff/1/src/compiler/raw-machine-as...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1477413002/diff/1/src/compiler/raw-machine-as...
src/compiler/raw-machine-assembler.h:25: class RawLabel {
super-nit: Can we move this class down to the bottom of the file and only
forward-declare it here? Because I think the long description of what the RMA is
and how it works should stay at the top of this file, so that people see it when
they stumble into this file.

https://codereview.chromium.org/1477413002/diff/1/test/cctest/compiler/test-b...
File test/cctest/compiler/test-branch-combine.cc (left):

https://codereview.chromium.org/1477413002/diff/1/test/cctest/compiler/test-b...
test/cctest/compiler/test-branch-combine.cc:16: typedef
RawMachineAssembler::Label MLabel;
On 2015/11/27 14:00:28, vogelheim wrote:
> I removed all the typedefs, since RawLabel -> MLabel doesn't seem like much of
a
> gain. Let me if you disagree.

Acknowledged. SGTM, I agree!

[vogelheim, 2015-11-27 17:21:14]

https://codereview.chromium.org/1477413002/diff/1/src/compiler/raw-machine-as...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1477413002/diff/1/src/compiler/raw-machine-as...
src/compiler/raw-machine-assembler.h:25: class RawLabel {
On 2015/11/27 17:10:27, Michael Starzinger wrote:
> super-nit: Can we move this class down to the bottom of the file and only
> forward-declare it here? Because I think the long description of what the RMA
is
> and how it works should stay at the top of this file, so that people see it
when
> they stumble into this file.

Done.

[Benedikt Meurer, 2015-11-28 19:51:31]

Two mini nits. Otherwise LGTM.

Looking at your patch now, I think I'd prefer the name RawMachineLabel for
consistency with RawMachineAssembler. But I'm perfectly fine with RawLabel as
well. :-)

https://codereview.chromium.org/1477413002/diff/20001/src/compiler/raw-machin...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1477413002/diff/20001/src/compiler/raw-machin...
src/compiler/raw-machine-assembler.h:639: class RawLabel {
Nit: Can we mark it as final?

https://codereview.chromium.org/1477413002/diff/20001/src/compiler/raw-machin...
src/compiler/raw-machine-assembler.h:641: RawLabel() : block_(NULL),
used_(false), bound_(false) {}
Nit: Can we put the implementation of both constructor and destructor to the .cc
file?

[vogelheim, 2015-11-30 09:55:16]

https://codereview.chromium.org/1477413002/diff/20001/src/compiler/raw-machin...
File src/compiler/raw-machine-assembler.h (right):

https://codereview.chromium.org/1477413002/diff/20001/src/compiler/raw-machin...
src/compiler/raw-machine-assembler.h:639: class RawLabel {
On 2015/11/28 19:51:30, Benedikt Meurer wrote:
> Nit: Can we mark it as final?

Done.

https://codereview.chromium.org/1477413002/diff/20001/src/compiler/raw-machin...
src/compiler/raw-machine-assembler.h:641: RawLabel() : block_(NULL),
used_(false), bound_(false) {}
On 2015/11/28 19:51:31, Benedikt Meurer wrote:
> Nit: Can we put the implementation of both constructor and destructor to the
.cc
> file?

Done.

[vogelheim, 2015-11-30 09:55:34]

On 2015/11/28 19:51:31, Benedikt Meurer wrote:
> Looking at your patch now, I think I'd prefer the name RawMachineLabel for
> consistency with RawMachineAssembler. But I'm perfectly fine with RawLabel as
> well. :-)

Done.

[vogelheim, 2015-11-30 11:24:47]

The CQ bit was checked by vogelheim@chromium.org

[vogelheim, 2015-11-30 11:24:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/1477413002/#ps60001
(title: "RawLabel -> RawMachineLabel")

[commit-bot: I haz the power, 2015-11-30 11:24:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1477413002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1477413002/60001

[commit-bot: I haz the power, 2015-11-30 11:28:52]

Description was changed from

==========
Move RMA::Label out of the class, so it can be forward declared.

R=bmeurer@chromium.org, mstarzinger@chromium.org
BUG=chromium:508898
LOG=Y
==========

to

==========
Move RMA::Label out of the class, so it can be forward declared.

R=bmeurer@chromium.org, mstarzinger@chromium.org
BUG=chromium:508898
LOG=Y
==========

[commit-bot: I haz the power, 2015-11-30 11:28:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-30 11:29:30]

Description was changed from

==========
Move RMA::Label out of the class, so it can be forward declared.

R=bmeurer@chromium.org, mstarzinger@chromium.org
BUG=chromium:508898
LOG=Y
==========

to

==========
Move RMA::Label out of the class, so it can be forward declared.

R=bmeurer@chromium.org, mstarzinger@chromium.org
BUG=chromium:508898
LOG=Y

Committed: https://crrev.com/d3ba9afee28f1b515b176ce40a1b4b18f9be1f68
Cr-Commit-Position: refs/heads/master@{#32400}
==========

[commit-bot: I haz the power, 2015-11-30 11:29:31]

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