Subzero: Replace pointers to allocation functions in stores

Subzero: Replace pointers to allocation functions in stores

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
R=kschimpf@google.com, stichnot@chromium.org

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

[tlively, 2016-08-17 23:28:12]

tlively@google.com changed reviewers:
+ kschimpf@google.com, mcgrathr@chromium.org, stichnot@chromium.org

[Jim Stichnoth, 2016-08-18 13:17:19]

lgtm

https://codereview.chromium.org/2256903003/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2256903003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:395: assert(!DataName.empty());
I just realized that if you use this pattern:
  S = N.toStringOrEmpty();
  assert(!S.empty());
You can probably simplify it to:
  S = N.toString();

The main difference is that the former does an assertion failure while the
latter does a report_fatal_error.  There's another minor difference between
these two, in that this will give an assertion failure:
  N = StringID::createWithString(Ctx, "");
  S = N.toStringOrEmpty();
  assert(!S.empty());
while this will succeed:
  N = StringID::createWithString(Ctx, "");
  S = N.toString();
but I doubt this is really an issue for your case.

[Karl, 2016-08-18 14:17:01]

lgtm

https://codereview.chromium.org/2256903003/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2256903003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:409: }
Minor nit. It appears that instrument load/store look nearly identical. Should
they use a common method?

[tlively, 2016-08-18 15:17:48]

https://codereview.chromium.org/2256903003/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2256903003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:395: assert(!DataName.empty());
On 2016/08/18 13:17:19, stichnot wrote:
> I just realized that if you use this pattern:
>   S = N.toStringOrEmpty();
>   assert(!S.empty());
> You can probably simplify it to:
>   S = N.toString();
> 
> The main difference is that the former does an assertion failure while the
> latter does a report_fatal_error.  There's another minor difference between
> these two, in that this will give an assertion failure:
>   N = StringID::createWithString(Ctx, "");
>   S = N.toStringOrEmpty();
>   assert(!S.empty());
> while this will succeed:
>   N = StringID::createWithString(Ctx, "");
>   S = N.toString();
> but I doubt this is really an issue for your case.

Done.

https://codereview.chromium.org/2256903003/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:409: }
On 2016/08/18 14:17:01, Karl wrote:
> Minor nit. It appears that instrument load/store look nearly identical. Should
> they use a common method?

Done.

[Karl, 2016-08-18 15:22:56]

lgtm

[tlively, 2016-08-18 17:37:49]

Description was changed from

==========
Subzero: Replace pointers to allocation functions in stores

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
==========

to

==========
Subzero: Replace pointers to allocation functions in stores

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
R=kschimpf@google.com, stichnot@chromium.org

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

[tlively, 2016-08-18 17:37:51]

Committed patchset #2 (id:20001) manually as
75f5229f8b4ac3985f4e31d0402e01510b065720 (presubmit successful).