Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2913013003:
Move enableNative to target, turn it on by default (Closed)
|
Created:
3 years, 6 months ago by Siggi Cherem (dart-lang) Modified:
3 years, 6 months ago Reviewers:
ahe CC:
reviews_dartlang.org, dart-fe-team+reviews_google.com Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionMove enableNative to target, turn it on by default
R=ahe@google.com
Committed: https://github.com/dart-lang/sdk/commit/682011c7b296dc82af079477b11fdc74b3220bc5
Patch Set 1 #
Total comments: 12
Patch Set 2 : cl comments + rebase #
Messages
Total messages: 12 (3 generated)
Patchset #1 (id:1) has been deleted
sigmund@google.com changed reviewers: + ahe@google.com
https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right): https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/body_builder.dart:914: enableNative && let me know if you want to drop this optimization now (for the VM this is always true). I kept it because it helps with dart2js https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/target_implementation.dart (right): https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/target_implementation.dart:103: bool enableNative(SourceLibraryBuilder library) => true; currently I use SourceLibraryBuilder because this is only relevant when dealing with sources. Makes sense also that this is the first type where we define `isPatch`.
lgtm https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right): https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/body_builder.dart:914: enableNative && On 2017/05/30 20:33:59, Siggi Cherem (dart-lang) wrote: > let me know if you want to drop this optimization now (for the VM this is always > true). > > I kept it because it helps with dart2js It's not an optimization anymore. I think we should add a boolean to this class: final isPlatformLibrary = library.uri.scheme == "dart"; https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/target_implementation.dart (right): https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/target_implementation.dart:98: /// because of existing support for "dart-ext:" native extensions, but targets I would really like to get this only enabled for dart-ext libraries. Could you add a TODO mentioning this bug: https://github.com/dart-lang/sdk/issues/29763 ? https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/target_implementation.dart:103: bool enableNative(SourceLibraryBuilder library) => true; On 2017/05/30 20:33:59, Siggi Cherem (dart-lang) wrote: > currently I use SourceLibraryBuilder because this is only relevant when dealing > with sources. Makes sense also that this is the first type where we define > `isPatch`. I'm not sure the builder hierarchies make a lot of sense anymore, but using SourceLibraryBuilder here goes against my original intent. I'm also not convinced that "source" is precondition for "isPatch". I can easily imagine scenarios where reading a patch file from a .dill file would make sense, although such scenarios would be useful only to Dart platform library developers. Anyway, you don't need to change anything, but we do need to have a design discussion about the builder hierarchies, and find some easy guidelines to follow. Otherwise, we risk ending up in a similar situation as dart2js did relying too much on AST nodes throughout its pipeline. https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/test/subp... File pkg/front_end/test/subpackage_relationships_test.dart (right): https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/test/subp... pkg/front_end/test/subpackage_relationships_test.dart:57: 'lib/src/fasta/source', OK. Since this is the first use of source in this directory, I'm going to say it's a really bad idea to use SourceLibraryBuilder.
https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/target_implementation.dart (right): https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/target_implementation.dart:103: bool enableNative(SourceLibraryBuilder library) => true; On 2017/05/31 09:07:50, ahe wrote: > On 2017/05/30 20:33:59, Siggi Cherem (dart-lang) wrote: > > currently I use SourceLibraryBuilder because this is only relevant when > dealing > > with sources. Makes sense also that this is the first type where we define > > `isPatch`. > > I'm not sure the builder hierarchies make a lot of sense anymore, but using > SourceLibraryBuilder here goes against my original intent. > > I'm also not convinced that "source" is precondition for "isPatch". I can easily > imagine scenarios where reading a patch file from a .dill file would make sense, > although such scenarios would be useful only to Dart platform library > developers. > > Anyway, you don't need to change anything, but we do need to have a design > discussion about the builder hierarchies, and find some easy guidelines to > follow. Otherwise, we risk ending up in a similar situation as dart2js did > relying too much on AST nodes throughout its pipeline. I was hesitant too. The main reason I thought Source* made sense here, and not a generic builder, was that I felt "enableNative" was only relevant when we are parsing and actually processing sources. That being said, I do like to keep APIs simple and I would be happy adding isPatch to LibraryBuilder instead. I actually did it locally, but didn't include in my CL. Let me know if you prefer that at this time, and I'll include that change with this CL.
https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/target_implementation.dart (right): https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/target_implementation.dart:103: bool enableNative(SourceLibraryBuilder library) => true; On 2017/05/31 14:25:43, Siggi Cherem (dart-lang) wrote: > On 2017/05/31 09:07:50, ahe wrote: > > On 2017/05/30 20:33:59, Siggi Cherem (dart-lang) wrote: > > > currently I use SourceLibraryBuilder because this is only relevant when > > dealing > > > with sources. Makes sense also that this is the first type where we define > > > `isPatch`. > > > > I'm not sure the builder hierarchies make a lot of sense anymore, but using > > SourceLibraryBuilder here goes against my original intent. > > > > I'm also not convinced that "source" is precondition for "isPatch". I can > easily > > imagine scenarios where reading a patch file from a .dill file would make > sense, > > although such scenarios would be useful only to Dart platform library > > developers. > > > > Anyway, you don't need to change anything, but we do need to have a design > > discussion about the builder hierarchies, and find some easy guidelines to > > follow. Otherwise, we risk ending up in a similar situation as dart2js did > > relying too much on AST nodes throughout its pipeline. > > > I was hesitant too. The main reason I thought Source* made sense here, and not a > generic builder, was that I felt "enableNative" was only relevant when we are > parsing and actually processing sources. That being said, I do like to keep APIs > simple and I would be happy adding isPatch to LibraryBuilder instead. I actually > did it locally, but didn't include in my CL. > > Let me know if you prefer that at this time, and I'll include that change with > this CL. > > That's up to you. An alternative is to do a type test when isPatch is needed.
On 2017/05/31 14:31:25, ahe wrote: > https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... > File pkg/front_end/lib/src/fasta/target_implementation.dart (right): > > https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... > pkg/front_end/lib/src/fasta/target_implementation.dart:103: bool > enableNative(SourceLibraryBuilder library) => true; > On 2017/05/31 14:25:43, Siggi Cherem (dart-lang) wrote: > > On 2017/05/31 09:07:50, ahe wrote: > > > On 2017/05/30 20:33:59, Siggi Cherem (dart-lang) wrote: > > > > currently I use SourceLibraryBuilder because this is only relevant when > > > dealing > > > > with sources. Makes sense also that this is the first type where we define > > > > `isPatch`. > > > > > > I'm not sure the builder hierarchies make a lot of sense anymore, but using > > > SourceLibraryBuilder here goes against my original intent. > > > > > > I'm also not convinced that "source" is precondition for "isPatch". I can > > easily > > > imagine scenarios where reading a patch file from a .dill file would make > > sense, > > > although such scenarios would be useful only to Dart platform library > > > developers. > > > > > > Anyway, you don't need to change anything, but we do need to have a design > > > discussion about the builder hierarchies, and find some easy guidelines to > > > follow. Otherwise, we risk ending up in a similar situation as dart2js did > > > relying too much on AST nodes throughout its pipeline. > > > > > > I was hesitant too. The main reason I thought Source* made sense here, and not > a > > generic builder, was that I felt "enableNative" was only relevant when we are > > parsing and actually processing sources. That being said, I do like to keep > APIs > > simple and I would be happy adding isPatch to LibraryBuilder instead. I > actually > > did it locally, but didn't include in my CL. > > > > Let me know if you prefer that at this time, and I'll include that change with > > this CL. > > > > > > That's up to you. An alternative is to do a type test when isPatch is needed. Done.
https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right): https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/body_builder.dart:914: enableNative && On 2017/05/31 09:07:50, ahe wrote: > On 2017/05/30 20:33:59, Siggi Cherem (dart-lang) wrote: > > let me know if you want to drop this optimization now (for the VM this is > always > > true). > > > > I kept it because it helps with dart2js > > It's not an optimization anymore. I think we should add a boolean to this class: > > final isPlatformLibrary = library.uri.scheme == "dart"; done. Any reason why not take it a step further and simply add: final isBuiltInLibrary = library.uri.scheme == 'dart' && library.uri.path == '_builtin';? https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/target_implementation.dart (right): https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/target_implementation.dart:98: /// because of existing support for "dart-ext:" native extensions, but targets On 2017/05/31 09:07:50, ahe wrote: > I would really like to get this only enabled for dart-ext libraries. Could you > add a TODO mentioning this bug: https://github.com/dart-lang/sdk/issues/29763 ? Done. https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/target_implementation.dart:103: bool enableNative(SourceLibraryBuilder library) => true; On 2017/05/31 14:31:25, ahe wrote: > On 2017/05/31 14:25:43, Siggi Cherem (dart-lang) wrote: > > On 2017/05/31 09:07:50, ahe wrote: > > > On 2017/05/30 20:33:59, Siggi Cherem (dart-lang) wrote: > > > > currently I use SourceLibraryBuilder because this is only relevant when > > > dealing > > > > with sources. Makes sense also that this is the first type where we define > > > > `isPatch`. > > > > > > I'm not sure the builder hierarchies make a lot of sense anymore, but using > > > SourceLibraryBuilder here goes against my original intent. > > > > > > I'm also not convinced that "source" is precondition for "isPatch". I can > > easily > > > imagine scenarios where reading a patch file from a .dill file would make > > sense, > > > although such scenarios would be useful only to Dart platform library > > > developers. > > > > > > Anyway, you don't need to change anything, but we do need to have a design > > > discussion about the builder hierarchies, and find some easy guidelines to > > > follow. Otherwise, we risk ending up in a similar situation as dart2js did > > > relying too much on AST nodes throughout its pipeline. > > > > > > I was hesitant too. The main reason I thought Source* made sense here, and not > a > > generic builder, was that I felt "enableNative" was only relevant when we are > > parsing and actually processing sources. That being said, I do like to keep > APIs > > simple and I would be happy adding isPatch to LibraryBuilder instead. I > actually > > did it locally, but didn't include in my CL. > > > > Let me know if you prefer that at this time, and I'll include that change with > > this CL. > > > > > > That's up to you. An alternative is to do a type test when isPatch is needed. turns out that dart2js doesn't have any `native` in patch files either, so I switchd back to LibraryBuilder and don't use isPatch anywhere.
Description was changed from ========== Move enableNative to target, turn it on by default ========== to ========== Move enableNative to target, turn it on by default R=ahe@google.com Committed: https://github.com/dart-lang/sdk/commit/682011c7b296dc82af079477b11fdc74b3220bc5 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) manually as 682011c7b296dc82af079477b11fdc74b3220bc5 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right): https://codereview.chromium.org/2913013003/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/kernel/body_builder.dart:914: enableNative && On 2017/05/31 20:26:14, Siggi Cherem (dart-lang) wrote: > On 2017/05/31 09:07:50, ahe wrote: > > On 2017/05/30 20:33:59, Siggi Cherem (dart-lang) wrote: > > > let me know if you want to drop this optimization now (for the VM this is > > always > > > true). > > > > > > I kept it because it helps with dart2js > > > > It's not an optimization anymore. I think we should add a boolean to this > class: > > > > final isPlatformLibrary = library.uri.scheme == "dart"; > > done. Any reason why not take it a step further and simply add: > final isBuiltInLibrary = library.uri.scheme == 'dart' && library.uri.path == > '_builtin';? That's a good idea. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
