Access ConstantSystem through ClosedWorld.

Access ConstantSystem through ClosedWorld.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/702500281b03ad5ed18c0c6b28d60ee60f905bf8

[Johnni Winther, 2016-12-19 10:40:39]

johnniwinther@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2016-12-20 00:58:40]

lgtm

https://codereview.chromium.org/2585223002/diff/1/pkg/compiler/lib/src/types/...
File pkg/compiler/lib/src/types/constants.dart (right):

https://codereview.chromium.org/2585223002/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/constants.dart:67: throw new
SpannableAssertionFailure(CURRENT_ELEMENT_SPANNABLE,
Is this a good general direction to think about? (instead of calling
internalError, throw and let the compiler catch it and map the message to
internalError?)

[Johnni Winther, 2016-12-20 08:35:49]

https://codereview.chromium.org/2585223002/diff/1/pkg/compiler/lib/src/types/...
File pkg/compiler/lib/src/types/constants.dart (right):

https://codereview.chromium.org/2585223002/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/constants.dart:67: throw new
SpannableAssertionFailure(CURRENT_ELEMENT_SPANNABLE,
On 2016/12/20 00:58:40, Siggi Cherem (dart-lang) wrote:
> Is this a good general direction to think about? (instead of calling
> internalError, throw and let the compiler catch it and map the message to
> internalError?)

The only difference is whether the stack-trace is prefixed by 'The compiler
crashed: Internal Error:' or 'The compiler crashed: Assertion failure:'. The
error pointing to the spannable position is in both case prefixed by 'Internal
Error:'.

Note that SpannableAssertionFailure is caught in withCurrentElement and is what
is thrown on a failure in 'invariant'.

I don't think it is worthwhile to propagate the 'DiagnosticReporter' purely for
assertions.

[Johnni Winther, 2016-12-20 08:50:38]

Description was changed from

==========
Access ConstantSystem through ClosedWorld.
==========

to

==========
Access ConstantSystem through ClosedWorld.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/702500281b03ad5ed18c0c6b28d60ee60f905bf8
==========

[Johnni Winther, 2016-12-20 08:50:40]

Committed patchset #1 (id:1) manually as
702500281b03ad5ed18c0c6b28d60ee60f905bf8 (presubmit successful).

[Siggi Cherem (dart-lang), 2016-12-20 15:28:59]

On 2016/12/20 08:35:49, Johnni Winther wrote:
>
https://codereview.chromium.org/2585223002/diff/1/pkg/compiler/lib/src/types/...
> File pkg/compiler/lib/src/types/constants.dart (right):
> 
>
https://codereview.chromium.org/2585223002/diff/1/pkg/compiler/lib/src/types/...
> pkg/compiler/lib/src/types/constants.dart:67: throw new
> SpannableAssertionFailure(CURRENT_ELEMENT_SPANNABLE,
> On 2016/12/20 00:58:40, Siggi Cherem (dart-lang) wrote:
> > Is this a good general direction to think about? (instead of calling
> > internalError, throw and let the compiler catch it and map the message to
> > internalError?)
> 
> The only difference is whether the stack-trace is prefixed by 'The compiler
> crashed: Internal Error:' or 'The compiler crashed: Assertion failure:'. The
> error pointing to the spannable position is in both case prefixed by 'Internal
> Error:'.
> 
> Note that SpannableAssertionFailure is caught in withCurrentElement and is
what
> is thrown on a failure in 'invariant'.
> 
> I don't think it is worthwhile to propagate the 'DiagnosticReporter' purely
for
> assertions.

sgtm - I was hoping we could treat all internal errors that way and also not
provide the reporter in those cases.

[Johnni Winther, 2016-12-21 08:20:11]

On 2016/12/20 15:28:59, Siggi Cherem (dart-lang) wrote:
> On 2016/12/20 08:35:49, Johnni Winther wrote:
> >
>
https://codereview.chromium.org/2585223002/diff/1/pkg/compiler/lib/src/types/...
> > File pkg/compiler/lib/src/types/constants.dart (right):
> > 
> >
>
https://codereview.chromium.org/2585223002/diff/1/pkg/compiler/lib/src/types/...
> > pkg/compiler/lib/src/types/constants.dart:67: throw new
> > SpannableAssertionFailure(CURRENT_ELEMENT_SPANNABLE,
> > On 2016/12/20 00:58:40, Siggi Cherem (dart-lang) wrote:
> > > Is this a good general direction to think about? (instead of calling
> > > internalError, throw and let the compiler catch it and map the message to
> > > internalError?)
> > 
> > The only difference is whether the stack-trace is prefixed by 'The compiler
> > crashed: Internal Error:' or 'The compiler crashed: Assertion failure:'. The
> > error pointing to the spannable position is in both case prefixed by
'Internal
> > Error:'.
> > 
> > Note that SpannableAssertionFailure is caught in withCurrentElement and is
> what
> > is thrown on a failure in 'invariant'.
> > 
> > I don't think it is worthwhile to propagate the 'DiagnosticReporter' purely
> for
> > assertions.
> 
> sgtm - I was hoping we could treat all internal errors that way and also not
> provide the reporter in those cases.

We should.