Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
549633002:
Add support for listing to the glob package. (Closed)
|
Created:
6 years, 3 months ago by nweiz Modified:
6 years, 3 months ago Reviewers:
Bob Nystrom CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd support for listing to the glob package.
R=rnystrom@google.com
BUG=17093
Committed: https://code.google.com/p/dart/source/detail?r=40604
Patch Set 1 #Patch Set 2 : Don't run glob tests on the browser. #
Total comments: 49
Patch Set 3 : Code review changes #
Total comments: 4
Patch Set 4 : Code review changes #
Messages
Total messages: 15 (0 generated)
On 2014/09/05 22:03:29, nweiz wrote: DBC I was hoping to use this package from the browser, see here: https://github.com/dart-lang/chromedeveditor/issues/3254#issuecomment-53724718 If it imports `dart:io` I won't be able to do that :(.
On 2014/09/06 02:11:25, Sean Eagan wrote: > On 2014/09/05 22:03:29, nweiz wrote: > > DBC > > I was hoping to use this package from the browser, see here: > > > https://github.com/dart-lang/chromedeveditor/issues/3254#issuecomment-53724718 > > If it imports `dart:io` I won't be able to do that :(. Ideally it could use: https://github.com/dart-lang/files.dart But since that's on github, I guess pub can't use it, so perhaps this could go in a separate `glob.io` library i.e. `package:glob/io.dart`?
On 2014/09/06 02:11:25, Sean Eagan wrote: > On 2014/09/05 22:03:29, nweiz wrote: > > DBC > > I was hoping to use this package from the browser, see here: > > > https://github.com/dart-lang/chromedeveditor/issues/3254#issuecomment-53724718 > > If it imports `dart:io` I won't be able to do that :(. Ideally it could use: https://github.com/dart-lang/files.dart But since that's on github, I guess pub can't use it, so perhaps this could go in a separate `glob.io` library i.e. `package:glob/io.dart`?
On 2014/09/06 02:41:37, Sean Eagan wrote: > On 2014/09/06 02:11:25, Sean Eagan wrote: > > On 2014/09/05 22:03:29, nweiz wrote: > > > > DBC > > > > I was hoping to use this package from the browser, see here: > > > > > > https://github.com/dart-lang/chromedeveditor/issues/3254#issuecomment-53724718 > > > > If it imports `dart:io` I won't be able to do that :(. > > Ideally it could use: > > https://github.com/dart-lang/files.dart > > But since that's on github, I guess pub can't use it, so perhaps this could go > in a separate `glob.io` library i.e. `package:glob/io.dart`? We could potentially bring in the files package as an external dependency, but according to its readme it's not ready for prime time yet. There's finally work going on to support cross-platform libraries, which this will absolutely use once it exists. In the meantime this could use mirrors in the same way as the http package. I don't have time to implement that, but I'd be happy to take a patch for it.
https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dart File pkg/glob/lib/src/ast.dart (right): https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:34: OptionsNode expand() => new OptionsNode([new SequenceNode([this])]); How about "flattenOptions"? Using the same name as Iterable.expand() makes the method below super hard to read. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:64: return nextSequences.map((nextSequence) { A loop containing an expand call that's mapping each item from the outer loop that imperatively modifies a collection from the outer loop... this is pretty much impenetrable. Is this just concatenating the result of .expand() on all of the child nodes and then combining adjacent literals? Can you do that in two passes? https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:82: /// Splits this glob into components along its path separators. A before/after example would help a lot here. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:84: /// This should only be called once the glob has been [expand]ed. [context] is If this is only valid on certain states, maybe it would be cleaner to have a separate type to represent an expanded AstNode? Basically compile it to an IR? https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:89: var sequences = [[]]; I think this code will be easier to read if you add the component list to the outer list after its finished instead of right when you start. Something like: var components = []; var sequence; addNode(node) { if (sequence == null) sequence = []; sequence.add(node); } finishComponent() { components.add(new SequenceNode(sequence)); sequence = null; } And then use these. I found just doing a bunch of iterable method calls on a untyped nested list to be hard to reason about. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:102: var components = context.split(text); "nodeComponents"? https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:112: root = root.replaceAll("\\", "/"); Why switch to forward slashes on Windows? https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... File pkg/glob/lib/src/list_tree.dart (right): https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:16: /// that match that glob. The logic for turning a raw AstNode into a ListTree is split across the expand() methods in AstNode subclasses, AstNode.split() and the ctor here. I think it might be clearer to put that into a single compiler class so it's all in one place. Doing the review here, I find I have to bounce back and forth a lot and all of these methods are pretty deeply intertwined. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:74: assert(components.first.nodes.length == 1); Document this. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:78: root = '.'; How about just initializing root to "." and ditch the else clauses? https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:84: // The first [parent] represents the root directory itself. It may be null This looks like a good chunk to break out into a separate method. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:102: if (parent != null) { Breaking this out into a method would get rid of some of the nesting too. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:158: seen.add(entity.path); This will use buckets of memory on large directory listings. Ideas in order of decreasing goodness: 1. Certainly don't do any this in the common case where there's only a single call to Directory.list(); 2. Don't do this if none of the listed directories overlap. 3. Only store paths that are within the overlapping range of the listed directories. 4. Store the visited set as a tree of components instead of a set of paths. 5. Risky, but you could just store a fixed-size hash of the path instead of the full path. 6. Don't guarantee non-repeated results. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:187: /// This may be `null`, indicating that this should be listed recursively. Clarify what the second "this" means. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:243: /// Lists all entities within [dir] matching this node or its children. Document that this (and listSync()) may return duplicates. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:251: // the list. Why? https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:261: children.forEach((sequence, child) { How does this handle a glob that points to a file, like "foo/bar/baz" where baz is an actual file? https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:277: resultPool.add(child.list(p.join(dir, basename), Calling this recursively will create a tree of StreamControllers and StreamPools, won't it? https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:284: if (error is FileSystemException) return; I don't understand the motivation here. If a raw Directory.list() call would fail here, why shouldn't the glob list? If I glob a directory I don't have access to, I want it to yell at me, not let me think the directory is empty. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:302: // Don't spawn extra [Directory.listSync] calls when we already know exactly Long line. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/test/list_test.... File pkg/glob/test/list_test.dart (right): https://codereview.chromium.org/549633002/diff/20001/pkg/glob/test/list_test.... pkg/glob/test/list_test.dart:69: expect(list("foo/**"), completion(unorderedEquals( How about using "foo/ba**" here to validate that "**" mixes with literals correctly. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/test/list_test.... pkg/glob/test/list_test.dart:83: completion(unorderedEquals(["deep/a/b/c", "deep/a/b/c/d"]))); It would be good to have a couple of non-matching files here. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/test/list_test.... pkg/glob/test/list_test.dart:107: }); These could both benefit from a negative example.
On 2014/09/08 19:31:44, nweiz wrote: > On 2014/09/06 02:41:37, Sean Eagan wrote: > > On 2014/09/06 02:11:25, Sean Eagan wrote: > > > On 2014/09/05 22:03:29, nweiz wrote: > > > > > > DBC > > > > > > I was hoping to use this package from the browser, see here: > > > > > > > > > > https://github.com/dart-lang/chromedeveditor/issues/3254#issuecomment-53724718 > > > > > > If it imports `dart:io` I won't be able to do that :(. > > > > Ideally it could use: > > > > https://github.com/dart-lang/files.dart > > > > But since that's on github, I guess pub can't use it, so perhaps this could go > > in a separate `glob.io` library i.e. `package:glob/io.dart`? > > We could potentially bring in the files package as an external dependency, but > according to its readme it's not ready for prime time yet. > > There's finally work going on to support cross-platform libraries, which this > will absolutely use once it exists. In the meantime this could use mirrors in > the same way as the http package. I don't have time to implement that, but I'd > be happy to take a patch for it. Using mirrors though wouldn't solve the problem of the return type of `list` and `listSync` including `FileSystemEntity` from `dart:io. To be able to use it in the browser, the return type would need to use something like `FileSystemEntry` from the files package. Just a heads up, I filed http://dartbug.com/20940 for publishing this package, so I know when I can start using it.
On 2014/09/19 13:10:05, Sean Eagan wrote: > On 2014/09/08 19:31:44, nweiz wrote: > > On 2014/09/06 02:41:37, Sean Eagan wrote: > > > On 2014/09/06 02:11:25, Sean Eagan wrote: > > > > On 2014/09/05 22:03:29, nweiz wrote: > > > > > > > > DBC > > > > > > > > I was hoping to use this package from the browser, see here: > > > > > > > > > > > > > > https://github.com/dart-lang/chromedeveditor/issues/3254#issuecomment-53724718 > > > > > > > > If it imports `dart:io` I won't be able to do that :(. > > > > > > Ideally it could use: > > > > > > https://github.com/dart-lang/files.dart > > > > > > But since that's on github, I guess pub can't use it, so perhaps this could > go > > > in a separate `glob.io` library i.e. `package:glob/io.dart`? > > > > We could potentially bring in the files package as an external dependency, but > > according to its readme it's not ready for prime time yet. > > > > There's finally work going on to support cross-platform libraries, which this > > will absolutely use once it exists. In the meantime this could use mirrors in > > the same way as the http package. I don't have time to implement that, but I'd > > be happy to take a patch for it. > > Using mirrors though wouldn't solve the problem of the return type of `list` and > `listSync` including `FileSystemEntity` from `dart:io. To be able to use it in > the browser, the return type would need to use something like `FileSystemEntry` > from the files package. This depends on the specific solution for cross-platform code that ends up winning. There are some solutions where this just works and others where we'd probably have to remove FileSystemEntity from the type annotation. > Just a heads up, I filed http://dartbug.com/20940 for publishing this package, > so I know when I can start using it. Thanks! This was blocked on me, so hopefully Natalie will be about to get it out (in its current dart-io-only form) soon. - bob
https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dart File pkg/glob/lib/src/ast.dart (right): https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:34: OptionsNode expand() => new OptionsNode([new SequenceNode([this])]); On 2014/09/18 22:17:39, Bob Nystrom wrote: > How about "flattenOptions"? Using the same name as Iterable.expand() makes the > method below super hard to read. Done. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:64: return nextSequences.map((nextSequence) { On 2014/09/18 22:17:39, Bob Nystrom wrote: > A loop containing an expand call that's mapping each item from the outer loop > that imperatively modifies a collection from the outer loop... this is pretty > much impenetrable. > > Is this just concatenating the result of .expand() on all of the child nodes and > then combining adjacent literals? Can you do that in two passes? Done. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:82: /// Splits this glob into components along its path separators. On 2014/09/18 22:17:39, Bob Nystrom wrote: > A before/after example would help a lot here. Done. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:84: /// This should only be called once the glob has been [expand]ed. [context] is On 2014/09/18 22:17:39, Bob Nystrom wrote: > If this is only valid on certain states, maybe it would be cleaner to have a > separate type to represent an expanded AstNode? Basically compile it to an IR? Rather than doing that (which I think would add a lot of boilerplate for relatively little benefit), I'll just allow options here and document that it won't split on separators contained in them. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:89: var sequences = [[]]; On 2014/09/18 22:17:39, Bob Nystrom wrote: > I think this code will be easier to read if you add the component list to the > outer list after its finished instead of right when you start. Something like: > > var components = []; > var sequence; > > addNode(node) { > if (sequence == null) sequence = []; > sequence.add(node); > } > > finishComponent() { > components.add(new SequenceNode(sequence)); > sequence = null; > } > > And then use these. > > I found just doing a bunch of iterable method calls on a untyped nested list to > be hard to reason about. Done. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:102: var components = context.split(text); On 2014/09/18 22:17:39, Bob Nystrom wrote: > "nodeComponents"? This is referenced a lot more often than the previous variable, so I'd rather give it the shorter name. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/ast.dar... pkg/glob/lib/src/ast.dart:112: root = root.replaceAll("\\", "/"); On 2014/09/18 22:17:39, Bob Nystrom wrote: > Why switch to forward slashes on Windows? Above, we switched to backslashes to make [context.split] handle roots properly. That means that if there is a root, it'll still have backslashes, where forward slashes are required for globs. So we switch it back here. Added a comment explaining that. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... File pkg/glob/lib/src/list_tree.dart (right): https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:16: /// that match that glob. On 2014/09/18 22:17:40, Bob Nystrom wrote: > The logic for turning a raw AstNode into a ListTree is split across the expand() > methods in AstNode subclasses, AstNode.split() and the ctor here. I think it > might be clearer to put that into a single compiler class so it's all in one > place. Doing the review here, I find I have to bounce back and forth a lot and > all of these methods are pretty deeply intertwined. I factored them that way because it strikes me as *minimally* intertwined. [flattenOptions] and [split] are very well-defined Glob-to-Glob operations that can be understood in their own right completely separate from the compilation process. I guess I could move them here or into a new ListTreeCompiler class, but the recursion in [flattenOptions] would be more awkward and I at least would read them as more intertwined, by virtue of being in the same class. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:74: assert(components.first.nodes.length == 1); On 2014/09/18 22:17:40, Bob Nystrom wrote: > Document this. Done. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:78: root = '.'; On 2014/09/18 22:17:40, Bob Nystrom wrote: > How about just initializing root to "." and ditch the else clauses? Done. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:84: // The first [parent] represents the root directory itself. It may be null On 2014/09/18 22:17:40, Bob Nystrom wrote: > This looks like a good chunk to break out into a separate method. Done. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:102: if (parent != null) { On 2014/09/18 22:17:40, Bob Nystrom wrote: > Breaking this out into a method would get rid of some of the nesting too. How so? https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:158: seen.add(entity.path); On 2014/09/18 22:17:40, Bob Nystrom wrote: > This will use buckets of memory on large directory listings. Ideas in order of > decreasing goodness: > > 1. Certainly don't do any this in the common case where there's only a single > call to Directory.list(); > 2. Don't do this if none of the listed directories overlap. Done. > 3. Only store paths that are within the overlapping range of the listed > directories. > 4. Store the visited set as a tree of components instead of a set of paths. These seem like more work than is warranted. At that point, we might as well figure out the logic to avoid re-listing directories in the first place. > 5. Risky, but you could just store a fixed-size hash of the path instead of the > full path. > 6. Don't guarantee non-repeated results. Yuck. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:187: /// This may be `null`, indicating that this should be listed recursively. On 2014/09/18 22:17:40, Bob Nystrom wrote: > Clarify what the second "this" means. Done. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:243: /// Lists all entities within [dir] matching this node or its children. On 2014/09/18 22:17:40, Bob Nystrom wrote: > Document that this (and listSync()) may return duplicates. Done. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:251: // the list. On 2014/09/18 22:17:40, Bob Nystrom wrote: > Why? This was here to handle things like an unreadable directory nested within the stuff a glob is listing. I thought Directory.list just didn't traverse into unreadable directories, but it looks like I was wrong. Changed to match the Directory.list behavior. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:261: children.forEach((sequence, child) { On 2014/09/18 22:17:40, Bob Nystrom wrote: > How does this handle a glob that points to a file, like "foo/bar/baz" where baz > is an actual file? That's handled as a validator. The root "." node's children has a sequence "foo" with a child node. That child node has a sequence "bar" with another child node. That child node has no children, just a validator "baz". https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:277: resultPool.add(child.list(p.join(dir, basename), On 2014/09/18 22:17:40, Bob Nystrom wrote: > Calling this recursively will create a tree of StreamControllers and > StreamPools, won't it? That's right, although it won't create more than are necessary. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:284: if (error is FileSystemException) return; On 2014/09/18 22:17:40, Bob Nystrom wrote: > I don't understand the motivation here. If a raw Directory.list() call would > fail here, why shouldn't the glob list? If I glob a directory I don't have > access to, I want it to yell at me, not let me think the directory is empty. See above. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:302: // Don't spawn extra [Directory.listSync] calls when we already know exactly On 2014/09/18 22:17:40, Bob Nystrom wrote: > Long line. Done. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/test/list_test.... File pkg/glob/test/list_test.dart (right): https://codereview.chromium.org/549633002/diff/20001/pkg/glob/test/list_test.... pkg/glob/test/list_test.dart:69: expect(list("foo/**"), completion(unorderedEquals( On 2014/09/18 22:17:40, Bob Nystrom wrote: > How about using "foo/ba**" here to validate that "**" mixes with literals > correctly. Done. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/test/list_test.... pkg/glob/test/list_test.dart:83: completion(unorderedEquals(["deep/a/b/c", "deep/a/b/c/d"]))); On 2014/09/18 22:17:40, Bob Nystrom wrote: > It would be good to have a couple of non-matching files here. Done. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/test/list_test.... pkg/glob/test/list_test.dart:107: }); On 2014/09/18 22:17:41, Bob Nystrom wrote: > These could both benefit from a negative example. Done.
Code review changes
https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... File pkg/glob/lib/src/list_tree.dart (right): https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:102: if (parent != null) { On 2014/09/22 23:48:40, nweiz wrote: > On 2014/09/18 22:17:40, Bob Nystrom wrote: > > Breaking this out into a method would get rid of some of the nesting too. > > How so? I was thinking you could early return instead of all of the "else if"s but I guess that doesn't buy much. https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:277: resultPool.add(child.list(p.join(dir, basename), On 2014/09/22 23:48:40, nweiz wrote: > On 2014/09/18 22:17:40, Bob Nystrom wrote: > > Calling this recursively will create a tree of StreamControllers and > > StreamPools, won't it? > > That's right, although it won't create more than are necessary. Isn't only one necessary? Couldn't you create a single one of each and pass them into the recursive calls? https://codereview.chromium.org/549633002/diff/40001/pkg/glob/lib/src/list_tr... File pkg/glob/lib/src/list_tree.dart (right): https://codereview.chromium.org/549633002/diff/40001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:177: return pool.stream.where((entity) { How about just: if (!_canOverlap) return pool.stream; var seen = new Set(); return stream.where(...); I've seen .where() be noticeable bottleneck when listing directories. https://codereview.chromium.org/549633002/diff/40001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:191: var seen = _canOverlap ? new Set() : null; Ditto.
https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... File pkg/glob/lib/src/list_tree.dart (right): https://codereview.chromium.org/549633002/diff/20001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:277: resultPool.add(child.list(p.join(dir, basename), On 2014/09/23 00:22:04, Bob Nystrom wrote: > On 2014/09/22 23:48:40, nweiz wrote: > > On 2014/09/18 22:17:40, Bob Nystrom wrote: > > > Calling this recursively will create a tree of StreamControllers and > > > StreamPools, won't it? > > > > That's right, although it won't create more than are necessary. > > Isn't only one necessary? Couldn't you create a single one of each and pass them > into the recursive calls? I suppose so. It seems cleaner to me to keep the [list] API self-contained, so I'd rather not do that without evidence that it'll make a substantial performance difference. https://codereview.chromium.org/549633002/diff/40001/pkg/glob/lib/src/list_tr... File pkg/glob/lib/src/list_tree.dart (right): https://codereview.chromium.org/549633002/diff/40001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:177: return pool.stream.where((entity) { On 2014/09/23 00:22:05, Bob Nystrom wrote: > How about just: > > if (!_canOverlap) return pool.stream; > var seen = new Set(); > return stream.where(...); > > I've seen .where() be noticeable bottleneck when listing directories. Done. https://codereview.chromium.org/549633002/diff/40001/pkg/glob/lib/src/list_tr... pkg/glob/lib/src/list_tree.dart:191: var seen = _canOverlap ? new Set() : null; On 2014/09/23 00:22:05, Bob Nystrom wrote: > Ditto. Done.
Code review changes
lgtm
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 40604 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
