Lazily format LanguageError messages (fix issue 15069).

Lazily format LanguageError messages (fix issue 15069).

R=iposva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=30308

[Ivan Posva, 2013-11-15 04:42:13]

https://codereview.chromium.org/68113028/diff/1/runtime/vm/class_finalizer.cc
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:12: #include "vm/parser.h"
This can be removed now, no?

https://codereview.chromium.org/68113028/diff/1/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/flow_graph.cc#newc...
runtime/vm/flow_graph.cc:1078: Script::Handle(function.script()),
Now that we report script and position, do we really need to get the function
name into the error message as well?

In any case you could use "LanguageError::NewFormatted(..., kFormat,
function_name, reason)" here, to avoid the duplication of the formatting code.

https://codereview.chromium.org/68113028/diff/1/runtime/vm/flow_graph_builder.cc
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/flow_graph_builder...
runtime/vm/flow_graph_builder.cc:3895: String::Handle(String::New(chars))));
ditto.

https://codereview.chromium.org/68113028/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler.cc:296: String::Handle(String::New(chars))));
ditto

https://codereview.chromium.org/68113028/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/object.cc#newcode1...
runtime/vm/object.cc:10728: NoGCScope no_gc;
What is this NoGCScope for?

https://codereview.chromium.org/68113028/diff/1/runtime/vm/object.cc#newcode1...
runtime/vm/object.cc:10818: const String& new_line =
String::Handle(String::New("\n"));
Please just use Symbols::NewLine() where we use this variable.

https://codereview.chromium.org/68113028/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/object.h#newcode3805
runtime/vm/object.h:3805: Heap::Space space = Heap::kNew);
I find it a bit strange that the space parameter trades places with the
message/format between this method and the the two below. Maybe we should have
the message always after the space parameter.

[siva, 2013-11-15 16:35:51]

https://codereview.chromium.org/68113028/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/raw_object.h#newco...
runtime/vm/raw_object.h:1100: RawString* formatted_message_;  // Incl. previous
error's formatted message.
Is there any value in storing the formattted message here? I would think we
would generate the formatted message and print it once.

Do you think there is a case were we will repeatedly want the formatted message?

[regis, 2013-11-15 18:27:38]

Thanks! PTAL

https://codereview.chromium.org/68113028/diff/1/runtime/vm/class_finalizer.cc
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/class_finalizer.cc...
runtime/vm/class_finalizer.cc:12: #include "vm/parser.h"
On 2013/11/15 04:42:13, Ivan Posva wrote:
> This can be removed now, no?

I tried earlier and got errors, so I did not remove it.
I am looking at it again and notice that DeoptimizeIfOwner needs to be imported
from code_generator.h, which was imported via parser.h.
So I replaced parser.h with code_generator.h.

https://codereview.chromium.org/68113028/diff/1/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/flow_graph.cc#newc...
runtime/vm/flow_graph.cc:1078: Script::Handle(function.script()),
On 2013/11/15 04:42:13, Ivan Posva wrote:
> Now that we report script and position, do we really need to get the function
> name into the error message as well?
> 
> In any case you could use "LanguageError::NewFormatted(..., kFormat,
> function_name, reason)" here, to avoid the duplication of the formatting code.

Done.

https://codereview.chromium.org/68113028/diff/1/runtime/vm/flow_graph_builder.cc
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/flow_graph_builder...
runtime/vm/flow_graph_builder.cc:3895: String::Handle(String::New(chars))));
On 2013/11/15 04:42:13, Ivan Posva wrote:
> ditto.

Done.

https://codereview.chromium.org/68113028/diff/1/runtime/vm/flow_graph_compile...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/flow_graph_compile...
runtime/vm/flow_graph_compiler.cc:296: String::Handle(String::New(chars))));
On 2013/11/15 04:42:13, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/68113028/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/object.cc#newcode1...
runtime/vm/object.cc:10728: NoGCScope no_gc;
On 2013/11/15 04:42:13, Ivan Posva wrote:
> What is this NoGCScope for?

To save a handle. Same as in String::NewFormatted.

https://codereview.chromium.org/68113028/diff/1/runtime/vm/object.cc#newcode1...
runtime/vm/object.cc:10818: const String& new_line =
String::Handle(String::New("\n"));
On 2013/11/15 04:42:13, Ivan Posva wrote:
> Please just use Symbols::NewLine() where we use this variable.

Done.

https://codereview.chromium.org/68113028/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/object.h#newcode3805
runtime/vm/object.h:3805: Heap::Space space = Heap::kNew);
On 2013/11/15 04:42:13, Ivan Posva wrote:
> I find it a bit strange that the space parameter trades places with the
> message/format between this method and the the two below. Maybe we should have
> the message always after the space parameter.

The space parameter is optional in most New calls and therefore has to be last.
This is not possible with vargs.
So I could switch the order and make it required, but better still, I can get
rid of this function variant.

https://codereview.chromium.org/68113028/diff/1/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/68113028/diff/1/runtime/vm/raw_object.h#newco...
runtime/vm/raw_object.h:1100: RawString* formatted_message_;  // Incl. previous
error's formatted message.
On 2013/11/15 16:35:51, siva wrote:
> Is there any value in storing the formattted message here? I would think we
> would generate the formatted message and print it once.
> 
> Do you think there is a case were we will repeatedly want the formatted
message?

It is not clear. On the other hand, the cost of storing one pointer in an error
object is not prohibitive.

[Ivan Posva, 2013-11-15 18:50:32]

LGTM -ip

[regis, 2013-11-15 18:54:32]

Committed patchset #2 manually as r30308 (presubmit successful).