Issue 262253002: Add flag to AnalysisOptions to be able to disable deferred loading.

Issue 262253002: Add flag to AnalysisOptions to be able to disable deferred loading. (Closed)

Created:
6 years, 7 months ago by jwren

Modified:
6 years, 7 months ago

Reviewers:
Brian Wilkerson

CC:
reviews_dartlang.org, ricow1

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Add flag to AnalysisOptions to be able to easily disable deferred loading. R=brianwilkerson@google.com Committed: https://code.google.com/p/dart/source/detail?r=35831

Patch Set 2 : review and rebase with be #

Patch Set 5 : Updated status file so that that the build doesn't break #

Patch Set 6 : rebase with bleeding_edge, updated status files not to break build #

Created: 6 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1413 lines, -673 lines)			Patch
M	editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/context/AnalysisOptions.java	View		1 chunk	+7 lines, -0 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisContextImpl.java	View	1 2	2 chunks	+2 lines, -0 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java	View	1	4 chunks	+24 lines, -0 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/task/ParseDartTask.java	View		2 chunks	+4 lines, -1 line	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/parser/Parser.java	View	1	4 chunks	+21 lines, -1 line	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/parser/ParserErrorCode.java	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/context/AnalysisContextFactory.java	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/parser/ParserTestCase.java	View	1	2 chunks	+2 lines, -0 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/CompileTimeErrorCodeTest.java	View	1 2 3	15 chunks	+448 lines, -448 lines	0 comments	Download
A	editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/DeferredLoadingTest.java	View	1 2 3	1 chunk	+668 lines, -0 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/HintCodeTest.java	View	1 2 3	2 chunks	+14 lines, -14 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/NonErrorResolverTest.java	View	1 2 3	4 chunks	+52 lines, -52 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/NonHintCodeTest.java	View	1 2 3	2 chunks	+13 lines, -13 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/StaticWarningCodeTest.java	View	1 2 3	5 chunks	+142 lines, -141 lines	0 comments	Download
M	editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/resolver/TestAll.java	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	tests/language/language_analyzer.status	View	1 2 3 4	1 chunk	+13 lines, -3 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

jwren

The change in semantics of having the error CTEC.IMPORT_OF_NON_LIBRARY change to a warning (SWC.IMPORT_OF_NON_LIBRARY) is ...

6 years, 7 months ago (2014-05-05 19:34:44 UTC) #1

Brian Wilkerson

LGTM https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java File editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java (right): https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java#newcode79 editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java:79: private boolean enableDeferredLoading = true; This might need ...

6 years, 7 months ago (2014-05-05 19:50:58 UTC) #2

LGTM

https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java
(right):

https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java:79:
private boolean enableDeferredLoading = true;
This might need to default to 'false' for now. In either case, the value should
be initialized via a public static constant, something like:
  private boolean enableDeferredLoading = DEFAULT_ENABLE_DEFERRED_LOADING;
so that the default value can be used in other places.

https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java:126:
public boolean getEnableDeferredLoading() {
I think we also need a setter.

https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/parser/Parser.java
(right):

https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/parser/Parser.java:99:
private boolean parseDeferredLibraries = true;
This should be initialized from the suggested static field in
AnalysisOptionsImpl so that the default is the same everywhere.

https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/parser/Parser.java:4221:
reportErrorForCurrentToken(ParserErrorCode.UNEXPECTED_TOKEN,
currentToken.getLexeme());
We might want a more specific error message, like
  "Deferred imports are not supported"
with a correction of something like
  "Use the flag ... to enable deferred imports"
(Do we display corrections anywhere?)

jwren

https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java File editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java (right): https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java#newcode79 editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java:79: private boolean enableDeferredLoading = true; On 2014/05/05 19:50:58, Brian ...

6 years, 7 months ago (2014-05-05 22:54:12 UTC) #3

https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java
(right):

https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java:79:
private boolean enableDeferredLoading = true;
On 2014/05/05 19:50:58, Brian Wilkerson wrote:
> This might need to default to 'false' for now. In either case, the value
should
> be initialized via a public static constant, something like:
>   private boolean enableDeferredLoading = DEFAULT_ENABLE_DEFERRED_LOADING;
> so that the default value can be used in other places.

Done.

https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/context/AnalysisOptionsImpl.java:126:
public boolean getEnableDeferredLoading() {
On 2014/05/05 19:50:58, Brian Wilkerson wrote:
> I think we also need a setter.

Done.

https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/parser/Parser.java
(right):

https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/parser/Parser.java:99:
private boolean parseDeferredLibraries = true;
On 2014/05/05 19:50:58, Brian Wilkerson wrote:
> This should be initialized from the suggested static field in
> AnalysisOptionsImpl so that the default is the same everywhere.

Done.

https://codereview.chromium.org/262253002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/parser/Parser.java:4221:
reportErrorForCurrentToken(ParserErrorCode.UNEXPECTED_TOKEN,
currentToken.getLexeme());
Done.
We do not display the corrections anywhere.
After a flag has been added to enable deferred loading (now that the default is
to not have it) we can revisit the error message to indicate the flag to use to
enable it.

On 2014/05/05 19:50:58, Brian Wilkerson wrote:
> We might want a more specific error message, like
>   "Deferred imports are not supported"
> with a correction of something like
>   "Use the flag ... to enable deferred imports"
> (Do we display corrections anywhere?)

jwren

6 years, 7 months ago (2014-05-06 21:17:53 UTC) #4

Message was sent while issue was closed.

Committed patchset #6 manually as r35831 (presubmit successful).

Expand Messages | Collapse Messages