[turbofan] Improve JSON output

[turbofan] Improve JSON output

Committed: https://crrev.com/328cf76e353223b281d97c36f76203d08b9cc7a4
Cr-Commit-Position: refs/heads/master@{#26323}

[danno, 2015-01-28 14:47:09]

danno@chromium.org changed reviewers:
+ svenpanne@chromium.org

[danno, 2015-01-28 14:47:10]

PTAL

[Sven Panne, 2015-01-28 15:00:43]

LGTM with nits

https://codereview.chromium.org/882973002/diff/60001/src/compiler/graph-visua...
File src/compiler/graph-visualizer.h (right):

https://codereview.chromium.org/882973002/diff/60001/src/compiler/graph-visua...
src/compiler/graph-visualizer.h:33: explicit AsJSON(const Graph& g,
SourcePositionTable* p)
Nit: Remove explicit.

https://codereview.chromium.org/882973002/diff/60001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/882973002/diff/60001/src/compiler/pipeline.cc...
src/compiler/pipeline.cc:697: Vector<char> filename(buffer, sizeof(buffer));
Using EmbeddedVector is simpler here.

https://codereview.chromium.org/882973002/diff/60001/src/compiler/pipeline.cc...
src/compiler/pipeline.cc:713: Vector<char> full_filename(full_filename_buffer,
EmbeddedVector again.

https://codereview.chromium.org/882973002/diff/60001/src/ostreams.cc
File src/ostreams.cc (right):

https://codereview.chromium.org/882973002/diff/60001/src/ostreams.cc#newcode70
src/ostreams.cc:70: if (c.value == '\n') {
I think a 'comb-like' pattern plus inserting strings directly is a bit clearer:

   if (c.value == '\n') return os << "\\n";
   if (c.value == '\r') return os << "\\r";
   ...

Or perhaps even better:

   if (strstr("\n\r\'\"", c.value)) return os << '\\' << c.value;
   ...

[danno, 2015-01-28 15:30:12]

Feedback addressed, sending to CQ

https://codereview.chromium.org/882973002/diff/60001/src/compiler/graph-visua...
File src/compiler/graph-visualizer.h (right):

https://codereview.chromium.org/882973002/diff/60001/src/compiler/graph-visua...
src/compiler/graph-visualizer.h:33: explicit AsJSON(const Graph& g,
SourcePositionTable* p)
On 2015/01/28 15:00:43, Sven Panne wrote:
> Nit: Remove explicit.

Done.

https://codereview.chromium.org/882973002/diff/60001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/882973002/diff/60001/src/compiler/pipeline.cc...
src/compiler/pipeline.cc:697: Vector<char> filename(buffer, sizeof(buffer));
On 2015/01/28 15:00:43, Sven Panne wrote:
> Using EmbeddedVector is simpler here.

Done.

https://codereview.chromium.org/882973002/diff/60001/src/compiler/pipeline.cc...
src/compiler/pipeline.cc:713: Vector<char> full_filename(full_filename_buffer,
On 2015/01/28 15:00:43, Sven Panne wrote:
> EmbeddedVector again.

Done.

https://codereview.chromium.org/882973002/diff/60001/src/ostreams.cc
File src/ostreams.cc (right):

https://codereview.chromium.org/882973002/diff/60001/src/ostreams.cc#newcode70
src/ostreams.cc:70: if (c.value == '\n') {
On 2015/01/28 15:00:43, Sven Panne wrote:
> I think a 'comb-like' pattern plus inserting strings directly is a bit
clearer:
> 
>    if (c.value == '\n') return os << "\\n";
>    if (c.value == '\r') return os << "\\r";
>    ...
> 
> Or perhaps even better:
> 
>    if (strstr("\n\r\'\"", c.value)) return os << '\\' << c.value;
>    ...

Done.

[danno, 2015-01-28 15:30:17]

The CQ bit was checked by danno@chromium.org

[commit-bot: I haz the power, 2015-01-28 15:31:19]

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

[commit-bot: I haz the power, 2015-01-28 16:11:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-28 16:11:27]

Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/945)

[danno, 2015-01-29 10:50:50]

The CQ bit was checked by danno@chromium.org

[commit-bot: I haz the power, 2015-01-29 10:51:46]

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

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

The CQ bit was unchecked by commit-bot@chromium.org

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

Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/960)

[danno, 2015-01-29 11:44:12]

The CQ bit was checked by danno@chromium.org

[commit-bot: I haz the power, 2015-01-29 11:45:06]

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

[commit-bot: I haz the power, 2015-01-29 12:15:06]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-29 12:15:19]

Patchset 6 (id:??) landed as
https://crrev.com/328cf76e353223b281d97c36f76203d08b9cc7a4
Cr-Commit-Position: refs/heads/master@{#26323}

[titzer, 2015-02-02 17:25:36]

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

[titzer, 2015-02-02 17:25:37]

https://codereview.chromium.org/882973002/diff/100001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/882973002/diff/100001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:695: const char* mode) {
This is starting to get pretty unwieldy. Can we please move some of this code
out of pipeline.cc?

[Michael Starzinger, 2015-02-02 17:26:08]

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

[Michael Starzinger, 2015-02-02 17:26:09]

https://codereview.chromium.org/882973002/diff/100001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/882973002/diff/100001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:695: const char* mode) {
On 2015/02/02 17:25:36, titzer wrote:
> This is starting to get pretty unwieldy. Can we please move some of this code
> out of pipeline.cc?

Strong +1 here, this should be part of the graph-visualizer.

[danno, 2015-02-02 17:37:20]

I'll move this out, I'm working on an additional patch anyway.