Subzero: Do class definition cleanups for assembler files too.

Subzero: Do class definition cleanups for assembler files too.

Similar to https://codereview.chromium.org/656123003/, but cover some of
the assembler files which were avoided to avoid conflicts.

BUG=none
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=f76fd3774379179427a228c29d38cc3ca1d904c1

[jvoung (off chromium), 2014-10-16 17:15:49]

jvoung@chromium.org changed reviewers:
+ kschimpf@chromium.org, stichnot@chromium.org

[Karl, 2014-10-16 17:23:25]

kschimpf@google.com changed reviewers:
+ kschimpf@google.com

[Karl, 2014-10-16 17:23:26]

https://codereview.chromium.org/643903006/diff/1/src/assembler_ia32.h
File src/assembler_ia32.h (right):

https://codereview.chromium.org/643903006/diff/1/src/assembler_ia32.h#newcode102
src/assembler_ia32.h:102: Operand(const Operand &other) :
length_(other.length_), fixup_(other.fixup_) {
Jim suggested doing

    // Operand(const Operand &other) = delete;

instead to make it explicit that we are assuming the default constructor. A
commented version of the delete for assignment clarifies that we are using the
default assignment.

[Roland McGrath, 2014-10-16 17:26:27]

The = delete comment looks odd to me, because it seems to imply that
uncommenting it would be the same, which is not the case.
Perhaps an = default comment would be clearer.

[Jim Stichnoth, 2014-10-16 18:19:53]

lgtm

I like Roland's suggestion.

https://codereview.chromium.org/643903006/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/643903006/diff/1/src/assembler.h#newcode125
src/assembler.h:125: #if defined(DEBUG)
Should this be "#if !defined(NDEBUG)"?  I.e., is this supposed to be similar to
assert()?

https://codereview.chromium.org/643903006/diff/1/src/assembler.h#newcode159
src/assembler.h:159: #endif
My personal preference for commenting large #if regions:

#if foo
#endif // foo

#if foo
#else // !foo
#endif // !foo

https://codereview.chromium.org/643903006/diff/1/src/assembler_ia32.h
File src/assembler_ia32.h (right):

https://codereview.chromium.org/643903006/diff/1/src/assembler_ia32.h#newcode7
src/assembler_ia32.h:7: //===- subzero/src/assembler_ia32.h - Assembler for
x86-32 ----------------===//
Add the emacs c++-mode marker here?
*- C++ -*

https://codereview.chromium.org/643903006/diff/1/src/assembler_ia32.h#newcode102
src/assembler_ia32.h:102: Operand(const Operand &other) :
length_(other.length_), fixup_(other.fixup_) {
On 2014/10/16 17:23:26, Karl wrote:
> Jim suggested doing
> 
>     // Operand(const Operand &other) = delete;
> 
> instead to make it explicit that we are assuming the default constructor. A
> commented version of the delete for assignment clarifies that we are using the
> default assignment.

True, but here we're explicitly not using the defaults

[jvoung (off chromium), 2014-10-16 19:02:37]

https://codereview.chromium.org/643903006/diff/1/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/643903006/diff/1/src/assembler.h#newcode125
src/assembler.h:125: #if defined(DEBUG)
On 2014/10/16 18:19:53, stichnot wrote:
> Should this be "#if !defined(NDEBUG)"?  I.e., is this supposed to be similar
to
> assert()?

Ah yes, I forgot to change this when first bringing in the code.

Yes this is for an assert build, and it handily caught an instance where I
forgot to ensurecapacity for the segment override prefix. Added an ensure
capacity there.

https://codereview.chromium.org/643903006/diff/1/src/assembler.h#newcode159
src/assembler.h:159: #endif
On 2014/10/16 18:19:53, stichnot wrote:
> My personal preference for commenting large #if regions:
> 
> #if foo
> #endif // foo
> 
> #if foo
> #else // !foo
> #endif // !foo

Done, I think -- all these double negations w/ !NDEBUG!

https://codereview.chromium.org/643903006/diff/1/src/assembler_ia32.h
File src/assembler_ia32.h (right):

https://codereview.chromium.org/643903006/diff/1/src/assembler_ia32.h#newcode7
src/assembler_ia32.h:7: //===- subzero/src/assembler_ia32.h - Assembler for
x86-32 ----------------===//
On 2014/10/16 18:19:53, stichnot wrote:
> Add the emacs c++-mode marker here?
> *- C++ -*

Done. I think technically it should be on the first line? I moved that up to the
first line.

https://codereview.chromium.org/643903006/diff/1/src/assembler_ia32.h#newcode102
src/assembler_ia32.h:102: Operand(const Operand &other) :
length_(other.length_), fixup_(other.fixup_) {
On 2014/10/16 18:19:53, stichnot wrote:
> On 2014/10/16 17:23:26, Karl wrote:
> > Jim suggested doing
> > 
> >     // Operand(const Operand &other) = delete;
> > 
> > instead to make it explicit that we are assuming the default constructor. A
> > commented version of the delete for assignment clarifies that we are using
the
> > default assignment.
> 
> True, but here we're explicitly not using the defaults

Right, it doesn't quite need to memmove everything in the 6-element encoding
array, just up to the length.

[Jim Stichnoth, 2014-10-16 22:21:06]

still lgtm

[jvoung (off chromium), 2014-10-16 22:39:28]

Committed patchset #4 (id:60001) manually as
f76fd3774379179427a228c29d38cc3ca1d904c1 (presubmit successful).