Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1929063002:
pkg/vm_service_client: add getSourceReport to VMServiceReference (Closed)
Descriptionadd getSourceReport to VMServiceReference
closes https://github.com/dart-lang/vm_service_client/issues/6
R=nweiz@google.com
Committed: https://github.com/dart-lang/vm_service_client/commit/167eb9cdad5d216e160c8f4751ca288a7d05d3ac
Patch Set 1 #
Total comments: 7
Patch Set 2 : nits #Patch Set 3 : changelog oops #
Total comments: 104
Patch Set 4 : code review #
Total comments: 2
Patch Set 5 : rebase and final nits #Patch Set 6 : oops #
Total comments: 79
Patch Set 7 : nits #Patch Set 8 : tweak #
Total comments: 22
Patch Set 9 : nits #
Total comments: 2
Patch Set 10 : nits #
Total comments: 9
Patch Set 11 : nits #Patch Set 12 : nits #
Messages
Total messages: 21 (2 generated)
kevmoo@google.com changed reviewers: + nweiz@google.com
Please document all new public APIs. https://codereview.chromium.org/1929063002/diff/1/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1929063002/diff/1/lib/src/isolate.dart#newcod... lib/src/isolate.dart:354: Nit: Remove this line. https://codereview.chromium.org/1929063002/diff/1/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/1/lib/src/script.dart#newcode18 lib/src/script.dart:18: String getScriptId(VMScriptRef scriptRef) => scriptRef._id; If an RPC requires a script ID, it should be a method on script. https://codereview.chromium.org/1929063002/diff/1/lib/src/source_report.dart File lib/src/source_report.dart (right): https://codereview.chromium.org/1929063002/diff/1/lib/src/source_report.dart#... lib/src/source_report.dart:47: class VMSourceReportRange { These should be in different files.
PTAL https://codereview.chromium.org/1929063002/diff/1/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1929063002/diff/1/lib/src/isolate.dart#newcod... lib/src/isolate.dart:354: On 2016/04/28 19:44:00, nweiz wrote: > Nit: Remove this line. Done. https://codereview.chromium.org/1929063002/diff/1/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/1/lib/src/script.dart#newcode18 lib/src/script.dart:18: String getScriptId(VMScriptRef scriptRef) => scriptRef._id; On 2016/04/28 19:44:00, nweiz wrote: > If an RPC requires a script ID, it should be a method on script. Done. https://codereview.chromium.org/1929063002/diff/1/lib/src/source_report.dart File lib/src/source_report.dart (right): https://codereview.chromium.org/1929063002/diff/1/lib/src/source_report.dart#... lib/src/source_report.dart:47: class VMSourceReportRange { On 2016/04/28 19:44:00, nweiz wrote: > These should be in different files. Disagree. This is like BreakPoint and BreakPointLocation IMHO
https://codereview.chromium.org/1929063002/diff/1/lib/src/source_report.dart File lib/src/source_report.dart (right): https://codereview.chromium.org/1929063002/diff/1/lib/src/source_report.dart#... lib/src/source_report.dart:47: class VMSourceReportRange { On 2016/04/28 20:26:06, kevmoo wrote: > On 2016/04/28 19:44:00, nweiz wrote: > > These should be in different files. > > Disagree. This is like BreakPoint and BreakPointLocation IMHO VMBreakpointLocation should probably also be a separate file; it's not tightly coupled to VMBreakpoint. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:346: /// Generates a set of reports tied to source locations in an isolate. It's confusing that this says "a set of reports" but the return value a single report object. It's not clear what "tied to" means here. What information does the source report include? Which locations does it include information about? https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:349: /// information. We don't usually talk about "setting" parameters. Usually we say something like "If [includeCoverageReport] is `true`, the report includes coverage information." Describe what it means for coverage information to be included, in terms of the returned [VMSourceReport] object. Depending on the form the information takes, it might also be worth explaining what that field (or whatever) is if this is `false`. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:352: /// positions which correspond to possible breakpoints. "Provide" is usually refers to the caller providing something to the function; it's confusing to use it to refer to information that's returned from the function. As above, explain where these token positions are created in terms of the [VMSourceReport]. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:354: /// Set [forceCompilation] to `true` to force compilation of all functions in The parameter below is "forceCompile", not "forceCompilation". https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:355: /// the range of the report. Nit: Don't add unnecessary newlines within paragraphs. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:357: /// the running Dart program. Explain what the difference is between forcing compilation and not doing so, in terms of the returned object. Under what circumstances will what information be different? This may require some empirical experimentation if the VM service docs don't go into detail. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:361: bool forceCompile}) async { Give these default values and don't use "== true" below. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:371: var params = <String, dynamic>{'reports': reports}; This map literal has values, so it doesn't need to be explicitly typed. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:372: Nit: You probably don't need every line of code in the function to be separated by an empty line. https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart#new... lib/src/script.dart:93: int endTokenPos, These should come after [forceCompile]. That way the parameters that this call has in common with [Isolate.getSourceReport] are all together. To preserve the abstraction, rather than having the user pass these explicitly, they should probably pass a single VMSourceLocation. That was they can easily say "get the source report for this class" or whatever and we don't make them manually munge locations and risk getting it wrong. https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart#new... lib/src/script.dart:117: } Most of the same comments for the other method apply here as well. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.dart File lib/src/source_report.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:24: /// Represents a set of reports tied to source locations in an isolate. This should be a noun phrase rather than "Represents ..." (https://www.dartlang.org/effective-dart/documentation/#prefer-starting-librar...). As before, it's weird here that we're using the plural "reports" to decide the contents of a single object called a "report". Maybe something like "A report composed of coverage information about ranges of source code." https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:28: /// methods, constructors, etc.) The first paragraph of a doc comment should always be one sentence long (https://www.dartlang.org/effective-dart/documentation/#do-make-the-first-sent...). Also avoid abbreviations like "etc" (https://www.dartlang.org/effective-dart/documentation/#avoid-abbreviations-an...). Maybe something like "Ranges in the program's source, corresponding to executable chunks of code such as functions and methods." https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:33: /// Note that ranges may be duplicated, in the case of mixins. Somewhere in here you should mention what information is attached to the ranges. Otherwise a user reading this won't have a clear understanding of the purpose of this class or where its information is exposed. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:42: .toList(), These should be unmodifiable lists. See other classes' constructors for examples. Nit: it's a little confusing that the initializers are in the opposite order as the field declarations. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:51: /// It is part of a [VMSourceReport]. "It" -> "This" or "Each range" https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:55: final int scriptIndex; Automatically cross-reference this. Get rid of VMSourceReport.scripts and replace this with a VMScriptRef. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:61: final int endPos; Instead of having raw start and end positions, expose a VMSourceLocation. Then you can also make VMSourceReportRange.script a getter that just returns location.script. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:63: /// Has this range been compiled by the Dart VM? Don't phrase documentation as questions. Explain what this means, what information will be unavailable if it's not compiled, and how to force it to be compiled. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:69: /// `includeCoverageReport` is true and the range has been It's not clear what "includeCoverageReport" is referring to here. Explicitly say "the `includeCoverageReport` parameter to [VMIsolate.getSourceReport]". https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:71: final VMSourceReportCoverage coverage; I don't know if there's a lot of value in requiring another layer of indirection to get at the coverage information. It would probably be easier for the user if we just included hits and misses directly on the range, and nulled them out if the coverage information wasn't available. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:73: /// Possible breakpoint information for this range, represented as a "Possible breakpoint information" isn't very informative. Maybe something like "Token locations at which breakpoints can be set". https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:79: final List<int> possibleBreakpoints; Make this a list of VMScriptTokens. Same with hits and misses. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:95: /// The list is sorted. Sorted by what? https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:98: /// A list of token positions in a SourceReportRange which have not been "not been" -> "not yet been" https://codereview.chromium.org/1929063002/diff/40001/test/breakpoint_test.dart File test/breakpoint_test.dart (right): https://codereview.chromium.org/1929063002/diff/40001/test/breakpoint_test.da... test/breakpoint_test.dart:28: var stdout = new StreamQueue(isolate.stdout.transform(lines)); Why are you making these changes in this CL? https://codereview.chromium.org/1929063002/diff/40001/test/client_test.dart File test/client_test.dart (right): https://codereview.chromium.org/1929063002/diff/40001/test/client_test.dart#n... test/client_test.dart:24: expect(version.minor, greaterThanOrEqualTo(4)); Why are you making this change at all? https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... File test/source_report_test.dart (right): https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:1: // Copyright (c) 2015, the Dart project authors. Please see the AUTHORS file 2016 https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:11: VMIsolateRef isolate; There's no reason for this to be a top-level variable, as opposed to a local variable in the group whose setUp actually initializes it. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:17: client = null; Don't bother nulling out the client. None of the other tests do it. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:22: group('simple script', () { The group name plus the test name should form a sentence. See other test files for examples. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:27: """); Indent this correctly. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:31: await isolate.pause(); I'm not sure what you're going for here, but this is going to be a race condition regardless. There's no guarantee that the isolate will pause at any particular point relative to the prints. If you want to wait until the isolate pauses before exiting, use waitUntilPaused(). https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:45: (range) => range.scriptIndex == report.scripts.length - 1); There shouldn't actually be multiple ranges in the runAndConnect script, right? https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:48: expect(range.startPos, lessThan(range.endPos)); Assert that the source location is equal to the source location of the top-level function defined in the library. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:55: Nit: Having a newline between every statement isn't much more readable than having no newlines at all. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:68: expect(coverage.misses, isEmpty); These assertions are pretty loose—they don't assert anything about where the hits are, or whether running code affects its coverage state. It would be better to have some code that's intentionally not run and some that is, and assert that the hit tokens aren't in the not-run section and that the miss tokens are. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:84: expect(range.possibleBreakpoints, isNotEmpty); Actually assert what the breakpoints are. You can use VMScript.sourceLocation to get easily-verifiable representations in terms of the actual code. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:87: test("both options", () async { I don't think this is necessary. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:101: }); Test forceCompile as well. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:124: includeCoverageReport: true, includePossibleBreakpoints: true); Rather than grabbing source locations from a previous report, grab them from the library's members. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:161: }); Split this up into multiple tests with a shared setUp.
https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:346: /// Generates a set of reports tied to source locations in an isolate. On 2016/04/29 00:52:43, nweiz wrote: > It's confusing that this says "a set of reports" but the return value a single > report object. > > It's not clear what "tied to" means here. What information does the source > report include? Which locations does it include information about? Acknowledged. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:349: /// information. On 2016/04/29 00:52:43, nweiz wrote: > We don't usually talk about "setting" parameters. Usually we say something like > "If [includeCoverageReport] is `true`, the report includes coverage > information." > > Describe what it means for coverage information to be included, in terms of the > returned [VMSourceReport] object. Depending on the form the information takes, > it might also be worth explaining what that field (or whatever) is if this is > `false`. Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:352: /// positions which correspond to possible breakpoints. On 2016/04/29 00:52:43, nweiz wrote: > "Provide" is usually refers to the caller providing something to the function; > it's confusing to use it to refer to information that's returned from the > function. > > As above, explain where these token positions are created in terms of the > [VMSourceReport]. Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:354: /// Set [forceCompilation] to `true` to force compilation of all functions in On 2016/04/29 00:52:43, nweiz wrote: > The parameter below is "forceCompile", not "forceCompilation". Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:355: /// the range of the report. On 2016/04/29 00:52:43, nweiz wrote: > Nit: Don't add unnecessary newlines within paragraphs. Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:361: bool forceCompile}) async { On 2016/04/29 00:52:43, nweiz wrote: > Give these default values and don't use "== true" below. Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:371: var params = <String, dynamic>{'reports': reports}; On 2016/04/29 00:52:43, nweiz wrote: > This map literal has values, so it doesn't need to be explicitly typed. Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:372: On 2016/04/29 00:52:43, nweiz wrote: > Nit: You probably don't need every line of code in the function to be separated > by an empty line. Acknowledged. https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart#new... lib/src/script.dart:93: int endTokenPos, On 2016/04/29 00:52:43, nweiz wrote: > These should come after [forceCompile]. That way the parameters that this call > has in common with [Isolate.getSourceReport] are all together. Done > To preserve the abstraction, rather than having the user pass these explicitly, > they should probably pass a single VMSourceLocation. That was they can easily > say "get the source report for this class" or whatever and we don't make them > manually munge locations and risk getting it wrong. Disagree. There's no way to create a VMSourceLocation (or a VMSourceToken). A user may want to do math to skip everything before/after some range they've already seen. I may have a hit on 58 and a miss on 65. Totally valid to ask for a range starting at '59' as they are trying to hit a desired spot in the code. https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart#new... lib/src/script.dart:117: } On 2016/04/29 00:52:43, nweiz wrote: > Most of the same comments for the other method apply here as well. Acknowledged. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.dart File lib/src/source_report.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:24: /// Represents a set of reports tied to source locations in an isolate. On 2016/04/29 00:52:44, nweiz wrote: > This should be a noun phrase rather than "Represents ..." > (https://www.dartlang.org/effective-dart/documentation/#prefer-starting-librar...). > > As before, it's weird here that we're using the plural "reports" to decide the > contents of a single object called a "report". Maybe something like "A report > composed of coverage information about ranges of source code." Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:28: /// methods, constructors, etc.) On 2016/04/29 00:52:43, nweiz wrote: > The first paragraph of a doc comment should always be one sentence long > (https://www.dartlang.org/effective-dart/documentation/#do-make-the-first-sent...). > Also avoid abbreviations like "etc" > (https://www.dartlang.org/effective-dart/documentation/#avoid-abbreviations-an...). > > Maybe something like "Ranges in the program's source, corresponding to > executable chunks of code such as functions and methods." Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:33: /// Note that ranges may be duplicated, in the case of mixins. On 2016/04/29 00:52:44, nweiz wrote: > Somewhere in here you should mention what information is attached to the ranges. > Otherwise a user reading this won't have a clear understanding of the purpose of > this class or where its information is exposed. Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:42: .toList(), On 2016/04/29 00:52:44, nweiz wrote: > These should be unmodifiable lists. See other classes' constructors for > examples. > > Nit: it's a little confusing that the initializers are in the opposite order as > the field declarations. Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:51: /// It is part of a [VMSourceReport]. On 2016/04/29 00:52:43, nweiz wrote: > "It" -> "This" or "Each range" Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:55: final int scriptIndex; On 2016/04/29 00:52:43, nweiz wrote: > Automatically cross-reference this. Get rid of VMSourceReport.scripts and > replace this with a VMScriptRef. Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:61: final int endPos; On 2016/04/29 00:52:44, nweiz wrote: > Instead of having raw start and end positions, expose a VMSourceLocation. Then > you can also make VMSourceReportRange.script a getter that just returns > location.script. Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:63: /// Has this range been compiled by the Dart VM? On 2016/04/29 00:52:43, nweiz wrote: > Don't phrase documentation as questions. > > Explain what this means, what information will be unavailable if it's not > compiled, and how to force it to be compiled. Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:69: /// `includeCoverageReport` is true and the range has been On 2016/04/29 00:52:44, nweiz wrote: > It's not clear what "includeCoverageReport" is referring to here. Explicitly say > "the `includeCoverageReport` parameter to [VMIsolate.getSourceReport]". Done - did not refer to just the isolate method since there is now also a method on script https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:71: final VMSourceReportCoverage coverage; On 2016/04/29 00:52:43, nweiz wrote: > I don't know if there's a lot of value in requiring another layer of indirection > to get at the coverage information. It would probably be easier for the user if > we just included hits and misses directly on the range, and nulled them out if > the coverage information wasn't available. Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:73: /// Possible breakpoint information for this range, represented as a On 2016/04/29 00:52:44, nweiz wrote: > "Possible breakpoint information" isn't very informative. Maybe something like > "Token locations at which breakpoints can be set". Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:79: final List<int> possibleBreakpoints; On 2016/04/29 00:52:44, nweiz wrote: > Make this a list of VMScriptTokens. Same with hits and misses. Done. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:95: /// The list is sorted. On 2016/04/29 00:52:44, nweiz wrote: > Sorted by what? for ints, I think that's implicit. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:98: /// A list of token positions in a SourceReportRange which have not been On 2016/04/29 00:52:44, nweiz wrote: > "not been" -> "not yet been" Done. https://codereview.chromium.org/1929063002/diff/40001/test/breakpoint_test.dart File test/breakpoint_test.dart (right): https://codereview.chromium.org/1929063002/diff/40001/test/breakpoint_test.da... test/breakpoint_test.dart:28: var stdout = new StreamQueue(isolate.stdout.transform(lines)); On 2016/04/29 00:52:44, nweiz wrote: > Why are you making these changes in this CL? Acknowledged. https://codereview.chromium.org/1929063002/diff/40001/test/client_test.dart File test/client_test.dart (right): https://codereview.chromium.org/1929063002/diff/40001/test/client_test.dart#n... test/client_test.dart:24: expect(version.minor, greaterThanOrEqualTo(4)); On 2016/04/29 00:52:44, nweiz wrote: > Why are you making this change at all? Bumped version of SDK – assuming that the version of the service protocol will not drop below 3.4 https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... File test/source_report_test.dart (right): https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:1: // Copyright (c) 2015, the Dart project authors. Please see the AUTHORS file On 2016/04/29 00:52:45, nweiz wrote: > 2016 Done. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:11: VMIsolateRef isolate; On 2016/04/29 00:52:44, nweiz wrote: > There's no reason for this to be a top-level variable, as opposed to a local > variable in the group whose setUp actually initializes it. Done. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:17: client = null; On 2016/04/29 00:52:45, nweiz wrote: > Don't bother nulling out the client. None of the other tests do it. Done. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:22: group('simple script', () { On 2016/04/29 00:52:44, nweiz wrote: > The group name plus the test name should form a sentence. See other test files > for examples. Done. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:27: """); On 2016/04/29 00:52:44, nweiz wrote: > Indent this correctly. this is copied from other tests that already exist – how should it be indented? https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:31: await isolate.pause(); On 2016/04/29 00:52:45, nweiz wrote: > I'm not sure what you're going for here, but this is going to be a race > condition regardless. There's no guarantee that the isolate will pause at any > particular point relative to the prints. > > If you want to wait until the isolate pauses before exiting, use > waitUntilPaused(). I tried that and it never gets hit – I think because of the `new ReceivePort();` in `runAndConnect` Thoughts? https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:45: (range) => range.scriptIndex == report.scripts.length - 1); On 2016/04/29 00:52:45, nweiz wrote: > There shouldn't actually be multiple ranges in the runAndConnect script, right? Acknowledged. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:48: expect(range.startPos, lessThan(range.endPos)); On 2016/04/29 00:52:45, nweiz wrote: > Assert that the source location is equal to the source location of the top-level > function defined in the library. Done. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:55: On 2016/04/29 00:52:44, nweiz wrote: > Nit: Having a newline between every statement isn't much more readable than > having no newlines at all. Acknowledged. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:68: expect(coverage.misses, isEmpty); On 2016/04/29 00:52:44, nweiz wrote: > These assertions are pretty loose—they don't assert anything about where the > hits are, or whether running code affects its coverage state. It would be better > to have some code that's intentionally not run and some that is, and assert that > the hit tokens aren't in the not-run section and that the miss tokens are. Done. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:84: expect(range.possibleBreakpoints, isNotEmpty); On 2016/04/29 00:52:45, nweiz wrote: > Actually assert what the breakpoints are. You can use VMScript.sourceLocation to > get easily-verifiable representations in terms of the actual code. Done. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:87: test("both options", () async { On 2016/04/29 00:52:45, nweiz wrote: > I don't think this is necessary. Eh, I like to make sure someone in the future doesn't mess this up – I could imagine someone doing an if/else and not realizing they are breaking something https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:101: }); On 2016/04/29 00:52:45, nweiz wrote: > Test forceCompile as well. Done. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:124: includeCoverageReport: true, includePossibleBreakpoints: true); On 2016/04/29 00:52:45, nweiz wrote: > Rather than grabbing source locations from a previous report, grab them from the > library's members. Done. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:161: }); On 2016/04/29 00:52:44, nweiz wrote: > Split this up into multiple tests with a shared setUp. Done. https://codereview.chromium.org/1929063002/diff/60001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1929063002/diff/60001/lib/src/isolate.dart#ne... lib/src/isolate.dart:363: /// the running Dart program. still need to do this...
https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/isolate.dart#ne... lib/src/isolate.dart:357: /// the running Dart program. On 2016/04/29 00:52:43, nweiz wrote: > Explain what the difference is between forcing compilation and not doing so, in > terms of the returned object. Under what circumstances will what information be > different? This may require some empirical experimentation if the VM service > docs don't go into detail. Done. https://codereview.chromium.org/1929063002/diff/60001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1929063002/diff/60001/lib/src/isolate.dart#ne... lib/src/isolate.dart:363: /// the running Dart program. On 2016/04/29 23:36:56, kevmoo wrote: > still need to do this... Done.
https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart#new... lib/src/script.dart:93: int endTokenPos, On 2016/04/29 23:36:54, kevmoo wrote: > On 2016/04/29 00:52:43, nweiz wrote: > > These should come after [forceCompile]. That way the parameters that this call > > has in common with [Isolate.getSourceReport] are all together. > > Done > > > To preserve the abstraction, rather than having the user pass these > explicitly, > > they should probably pass a single VMSourceLocation. That was they can easily > > say "get the source report for this class" or whatever and we don't make them > > manually munge locations and risk getting it wrong. > > Disagree. There's no way to create a VMSourceLocation (or a VMSourceToken). > > A user may want to do math to skip everything before/after some range they've > already seen. > > I may have a hit on 58 and a miss on 65. > > Totally valid to ask for a range starting at '59' as they are trying to hit a > desired spot in the code. I don't understand how users would possibly get into a position where they're interested in non-semantic ranges like this. If they know they have a hit on 58 and a miss on 65, and those are part of the same function, they already have the source report for that whole function. It makes a lot of sense to get the source report for a given function or a given class, and we should make that as easy as possible. It's not clear there's any reason to get arbitrary ranges between tokens, especially when you can just ignore some results from a wider source report, so it's nor worth warping the API to accommodate that entirely theoretical use-case. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.dart File lib/src/source_report.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:33: /// Note that ranges may be duplicated, in the case of mixins. On 2016/04/29 23:36:55, kevmoo wrote: > On 2016/04/29 00:52:44, nweiz wrote: > > Somewhere in here you should mention what information is attached to the > ranges. > > Otherwise a user reading this won't have a clear understanding of the purpose > of > > this class or where its information is exposed. > > Done. I don't see this. https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:95: /// The list is sorted. On 2016/04/29 23:36:54, kevmoo wrote: > On 2016/04/29 00:52:44, nweiz wrote: > > Sorted by what? > > for ints, I think that's implicit. It's certainly not obvious whether it's ascending or descending. A better way to describe it would be to say "sorted in the order the tokens appear in the source text". https://codereview.chromium.org/1929063002/diff/40001/test/client_test.dart File test/client_test.dart (right): https://codereview.chromium.org/1929063002/diff/40001/test/client_test.dart#n... test/client_test.dart:24: expect(version.minor, greaterThanOrEqualTo(4)); On 2016/04/29 23:36:55, kevmoo wrote: > On 2016/04/29 00:52:44, nweiz wrote: > > Why are you making this change at all? > > Bumped version of SDK – assuming that the version of the service protocol will > not drop below 3.4 This isn't really a good reason to change existing code outside of the scope of the current CL. It also creates a precedent that this needs to continue to be updated every time we bump the SDK version. Just leave it as it was. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... File test/source_report_test.dart (right): https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:27: """); On 2016/04/29 23:36:55, kevmoo wrote: > On 2016/04/29 00:52:44, nweiz wrote: > > Indent this correctly. > > this is copied from other tests that already exist – how should it be indented? The same way it was indented in the tests you copied it from. The quotes act like brackets, so the prints should be indented two spaces in from the base indentation, and the close-quotes should be on the base indentation. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:31: await isolate.pause(); On 2016/04/29 23:36:55, kevmoo wrote: > On 2016/04/29 00:52:45, nweiz wrote: > > I'm not sure what you're going for here, but this is going to be a race > > condition regardless. There's no guarantee that the isolate will pause at any > > particular point relative to the prints. > > > > If you want to wait until the isolate pauses before exiting, use > > waitUntilPaused(). > > I tried that and it never gets hit – I think because of the `new ReceivePort();` > in `runAndConnect` > > Thoughts? The simplest solution would be to just add Isolate.current.kill() after the prints. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:55: On 2016/04/29 23:36:55, kevmoo wrote: > On 2016/04/29 00:52:44, nweiz wrote: > > Nit: Having a newline between every statement isn't much more readable than > > having no newlines at all. > > Acknowledged. You deleted one newline :p. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:87: test("both options", () async { On 2016/04/29 23:36:55, kevmoo wrote: > On 2016/04/29 00:52:45, nweiz wrote: > > I don't think this is necessary. > > Eh, I like to make sure someone in the future doesn't mess this up – I could > imagine someone doing an if/else and not realizing they are breaking something If you're going to test the cross-product of all options, make sure to test coverage && breakpoints && forceCompile, coverage && !breakpoints && forceCompile, and so on ;). We have to draw a line on what gets tested somewhere, and it's helpful to have a consistent notion of where that is. Generally it's good to draw that line *before* testing combinations of orthogonal flags. https://codereview.chromium.org/1929063002/diff/100001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1929063002/diff/100001/lib/src/isolate.dart#n... lib/src/isolate.dart:354: /// Otherwise, [VMSourceReportRange.coverage] is `null`. Reflow this text so there isn't unnecessary whitespace at the end of each line. Same goes for a bunch of other paragraphs. Small diffs are less important than readable comments. https://codereview.chromium.org/1929063002/diff/100001/lib/src/isolate.dart#n... lib/src/isolate.dart:368: bool includePossibleBreakpoints: false, It doesn't seem particularly useful to have the source report default to containing no information. I'd make both of these default to true, and allow users to narrow it if that's an optimization that makes sense for them. https://codereview.chromium.org/1929063002/diff/100001/lib/src/isolate.dart#n... lib/src/isolate.dart:382: } Good thing you tested this! :D https://codereview.chromium.org/1929063002/diff/100001/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/100001/lib/src/script.dart#ne... lib/src/script.dart:31: VMScriptToken newVMScriptTokenFromScript(VMScriptRef script, int position) { I think I'd call this "from position" rather than "from script". https://codereview.chromium.org/1929063002/diff/100001/lib/src/script.dart#ne... lib/src/script.dart:98: /// to a subrange of a script. What does it mean to pass only "start" or only "end"? https://codereview.chromium.org/1929063002/diff/100001/lib/src/script.dart#ne... lib/src/script.dart:117: params['forceCompile'] = true; Nit: put the results on the same line as the "if" if it'll all fit on the same line. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_locatio... File lib/src/source_location.dart (right): https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_locatio... lib/src/source_location.dart:17: VMSourceLocation newVMSourceLocationFromLocation( Similarly, I'd call this "FromPositions". https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_locatio... lib/src/source_location.dart:18: VMScriptRef script, int tokenPos, int endTokenPos) => Indent +4. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_locatio... lib/src/source_location.dart:35: VMSourceLocation._(VMScriptRef script, int tokenPos, int endTokenPos) Rather than changing the calling convention for the main constructor, create a new fromPositions constructor that the new top-level function forwards to. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... File lib/src/source_report.dart (right): https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:9: List<VMScriptToken> _getTokens(VMScriptRef script, Iterable<int> locations) { Make this a static method in VMSourceReport—that helps communicate where it's used, and keeps the definition closer to the use. Also, document it. "locations" should probably be "positions", to match the terminology in the service protocol documentation. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:13: locations.map((i) => newVMScriptTokenFromScript(script, i))); "i" -> "location" (or "position") https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:21: .map((scriptItem) => newVMScriptRef(scope, scriptItem)) Just call this variable "script", like in the other similar code in this package. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:26: .map((rangeItem) => _newSourceReportRange(rangeItem, scripts))); Don't do actual work in the "new*" functions. It makes this library confusingly different from all the others in the package, which makes it more difficult to figure out what's happening where. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:48: /// methods, constructors, etc.) Avoid abbreviations like "etc" (https://www.dartlang.org/effective-dart/documentation/#avoid-abbreviations-an...) Also add a period. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:53: /// * ranges may be duplicated, in the case of mixins. I think bulleted lists are generally harder to read than prose. It's certainly harder to ensure that the markdown won't break somehow. Are you confident that dartdoc will render this correctly? https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:64: VMScriptRef get script => location.script; Document all public members. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:68: /// `true` if this range been compiled by the Dart VM. I like to say "Whether" rather than "`true` if" to avoid starting a sentence with a lower-case identifier. Up to you, though. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:72: /// Use `forceCompile` in `getSourceReport` to ensure the target scripts are Make "getSourceReport" a link. Also below. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:76: /// Token locations at which breakpoints can be set. "Token locations" -> "Token positions" https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:78: /// Provided only when the when the `includePossibleBreakpoints` in Remove "the". https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:82: /// A sorted list of token positions in a [VMSourceReportRang] which have been "Rang" -> "Range" https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:85: /// Provided only when the when the `includeCoverageReport` in Remove "the" https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:92: /// Provided only when the Remove "the" https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... File test/source_report_test.dart (right): https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:12: print("one"); What is going on with this indentation? Why are the two prints indented differently? Either the quotes should be treated like brackets, in which case everything should be indented two spaces and the closing quotes should be at the base level; or this should be treated as a normal multiline string rather than a code block, in which case its base indentation should be 0. I prefer the former. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:37: test("and no options", () async { The group name plus the test name should be a sentence describing the behavior you're testing. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:51: expect(startLocation.column, 0); Testing the explicit line and column is brittle, and it's not obvious to a reader that these numbers are correct. You can get the actual source location of the main method through isolate.rootLibrary (see the script tests I added). Then you can just check that the corresponding source_span SourceLocations are ==. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:74: expect(hitLocations, [6, 7, 8, 14]); Add a comment explaining what actual lines these represent. Also below. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:118: VMSourceLocation mainFunctionLocation; I think just "mainLocation" would be fine here. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:120: VMSourceLocation unusedFunc2Location; "Func" -> "Function" https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:143: var mainFunc = await rootLib.functions['main'].load(); "Func" -> "Function" https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:148: unusedFunc2Location = unusedFunc2Location = unusedFunction2.location; Extra assignment. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:158: expect(firstRange.compiled, isFalse); Make assertions about the other properties as well. Also below. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:159: var firstRangeLocation = Why is this called "Location" if it's an offset? https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:161: expect(firstRangeLocation, lessThan(mainFunctionSpan.start.offset)); You can just do expect(script.sourceSpan(firstRange.location), lessThan(mainFunctionSpan))) same below. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:185: test("with tokenPos set", () async { It's not called "tokenPos" (or "endTokenPos") anymore. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:215: reason: 'Includes the range containing only unusedFunction2.'); Assert that this is in fact the range it contains.
https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart#new... lib/src/script.dart:93: int endTokenPos, On 2016/05/04 23:05:51, nweiz wrote: > On 2016/04/29 23:36:54, kevmoo wrote: > > On 2016/04/29 00:52:43, nweiz wrote: > > > These should come after [forceCompile]. That way the parameters that this > call > > > has in common with [Isolate.getSourceReport] are all together. > > > > Done > > > > > To preserve the abstraction, rather than having the user pass these > > explicitly, > > > they should probably pass a single VMSourceLocation. That was they can > easily > > > say "get the source report for this class" or whatever and we don't make > them > > > manually munge locations and risk getting it wrong. > > > > Disagree. There's no way to create a VMSourceLocation (or a VMSourceToken). > > > > A user may want to do math to skip everything before/after some range they've > > already seen. > > > > I may have a hit on 58 and a miss on 65. > > > > Totally valid to ask for a range starting at '59' as they are trying to hit a > > desired spot in the code. > > I don't understand how users would possibly get into a position where they're > interested in non-semantic ranges like this. If they know they have a hit on 58 > and a miss on 65, and those are part of the same function, they already have the > source report for that whole function. > > It makes a lot of sense to get the source report for a given function or a given > class, and we should make that as easy as possible. It's not clear there's any > reason to get arbitrary ranges between tokens, especially when you can just > ignore some results from a wider source report, so it's nor worth warping the > API to accommodate that entirely theoretical use-case. This was fixed – now uses tokens https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.dart File lib/src/source_report.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:33: /// Note that ranges may be duplicated, in the case of mixins. On 2016/05/04 23:05:51, nweiz wrote: > On 2016/04/29 23:36:55, kevmoo wrote: > > On 2016/04/29 00:52:44, nweiz wrote: > > > Somewhere in here you should mention what information is attached to the > > ranges. > > > Otherwise a user reading this won't have a clear understanding of the > purpose > > of > > > this class or where its information is exposed. > > > > Done. > > I don't see this. See latest patch https://codereview.chromium.org/1929063002/diff/40001/lib/src/source_report.d... lib/src/source_report.dart:95: /// The list is sorted. On 2016/05/04 23:05:51, nweiz wrote: > On 2016/04/29 23:36:54, kevmoo wrote: > > On 2016/04/29 00:52:44, nweiz wrote: > > > Sorted by what? > > > > for ints, I think that's implicit. > > It's certainly not obvious whether it's ascending or descending. A better way to > describe it would be to say "sorted in the order the tokens appear in the source > text". Done. https://codereview.chromium.org/1929063002/diff/40001/test/client_test.dart File test/client_test.dart (right): https://codereview.chromium.org/1929063002/diff/40001/test/client_test.dart#n... test/client_test.dart:24: expect(version.minor, greaterThanOrEqualTo(4)); On 2016/05/04 23:05:51, nweiz wrote: > On 2016/04/29 23:36:55, kevmoo wrote: > > On 2016/04/29 00:52:44, nweiz wrote: > > > Why are you making this change at all? > > > > Bumped version of SDK – assuming that the version of the service protocol > will > > not drop below 3.4 > > This isn't really a good reason to change existing code outside of the scope of > the current CL. It also creates a precedent that this needs to continue to be > updated every time we bump the SDK version. Just leave it as it was. Done. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... File test/source_report_test.dart (right): https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:27: """); On 2016/05/04 23:05:51, nweiz wrote: > On 2016/04/29 23:36:55, kevmoo wrote: > > On 2016/04/29 00:52:44, nweiz wrote: > > > Indent this correctly. > > > > this is copied from other tests that already exist – how should it be > indented? > > The same way it was indented in the tests you copied it from. The quotes act > like brackets, so the prints should be indented two spaces in from the base > indentation, and the close-quotes should be on the base indentation. Please look at the latest code. This is changed quite a bit. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:31: await isolate.pause(); On 2016/05/04 23:05:51, nweiz wrote: > On 2016/04/29 23:36:55, kevmoo wrote: > > On 2016/04/29 00:52:45, nweiz wrote: > > > I'm not sure what you're going for here, but this is going to be a race > > > condition regardless. There's no guarantee that the isolate will pause at > any > > > particular point relative to the prints. > > > > > > If you want to wait until the isolate pauses before exiting, use > > > waitUntilPaused(). > > > > I tried that and it never gets hit – I think because of the `new > ReceivePort();` > > in `runAndConnect` > > > > Thoughts? > > The simplest solution would be to just add Isolate.current.kill() after the > prints. Done. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:55: On 2016/05/04 23:05:51, nweiz wrote: > On 2016/04/29 23:36:55, kevmoo wrote: > > On 2016/04/29 00:52:44, nweiz wrote: > > > Nit: Having a newline between every statement isn't much more readable than > > > having no newlines at all. > > > > Acknowledged. > > You deleted one newline :p. Acknowledged. https://codereview.chromium.org/1929063002/diff/40001/test/source_report_test... test/source_report_test.dart:87: test("both options", () async { On 2016/05/04 23:05:51, nweiz wrote: > On 2016/04/29 23:36:55, kevmoo wrote: > > On 2016/04/29 00:52:45, nweiz wrote: > > > I don't think this is necessary. > > > > Eh, I like to make sure someone in the future doesn't mess this up – I > could > > imagine someone doing an if/else and not realizing they are breaking something > > If you're going to test the cross-product of all options, make sure to test > coverage && breakpoints && forceCompile, coverage && !breakpoints && > forceCompile, and so on ;). > > We have to draw a line on what gets tested somewhere, and it's helpful to have a > consistent notion of where that is. Generally it's good to draw that line > *before* testing combinations of orthogonal flags. Acknowledged. https://codereview.chromium.org/1929063002/diff/100001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1929063002/diff/100001/lib/src/isolate.dart#n... lib/src/isolate.dart:354: /// Otherwise, [VMSourceReportRange.coverage] is `null`. On 2016/05/04 23:05:51, nweiz wrote: > Reflow this text so there isn't unnecessary whitespace at the end of each line. > Same goes for a bunch of other paragraphs. Small diffs are less important than > readable comments. Done – mostly. I like to start new sentences on new lines. Easier to read the source and less likely to require cascading changes later https://codereview.chromium.org/1929063002/diff/100001/lib/src/isolate.dart#n... lib/src/isolate.dart:368: bool includePossibleBreakpoints: false, On 2016/05/04 23:05:51, nweiz wrote: > It doesn't seem particularly useful to have the source report default to > containing no information. I'd make both of these default to true, and allow > users to narrow it if that's an optimization that makes sense for them. Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/isolate.dart#n... lib/src/isolate.dart:382: } On 2016/05/04 23:05:51, nweiz wrote: > Good thing you tested this! :D Acknowledged. https://codereview.chromium.org/1929063002/diff/100001/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/100001/lib/src/script.dart#ne... lib/src/script.dart:31: VMScriptToken newVMScriptTokenFromScript(VMScriptRef script, int position) { On 2016/05/04 23:05:51, nweiz wrote: > I think I'd call this "from position" rather than "from script". Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/script.dart#ne... lib/src/script.dart:98: /// to a subrange of a script. On 2016/05/04 23:05:51, nweiz wrote: > What does it mean to pass only "start" or only "end"? I think I cleared it up https://codereview.chromium.org/1929063002/diff/100001/lib/src/script.dart#ne... lib/src/script.dart:117: params['forceCompile'] = true; On 2016/05/04 23:05:51, nweiz wrote: > Nit: put the results on the same line as the "if" if it'll all fit on the same > line. Acknowledged. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_locatio... File lib/src/source_location.dart (right): https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_locatio... lib/src/source_location.dart:17: VMSourceLocation newVMSourceLocationFromLocation( On 2016/05/04 23:05:51, nweiz wrote: > Similarly, I'd call this "FromPositions". Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_locatio... lib/src/source_location.dart:18: VMScriptRef script, int tokenPos, int endTokenPos) => On 2016/05/04 23:05:51, nweiz wrote: > Indent +4. Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_locatio... lib/src/source_location.dart:35: VMSourceLocation._(VMScriptRef script, int tokenPos, int endTokenPos) On 2016/05/04 23:05:51, nweiz wrote: > Rather than changing the calling convention for the main constructor, create a > new fromPositions constructor that the new top-level function forwards to. Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... File lib/src/source_report.dart (right): https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:9: List<VMScriptToken> _getTokens(VMScriptRef script, Iterable<int> locations) { On 2016/05/04 23:05:52, nweiz wrote: > Make this a static method in VMSourceReport—that helps communicate where it's > used, and keeps the definition closer to the use. > > Also, document it. > > "locations" should probably be "positions", to match the terminology in the > service protocol documentation. Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:13: locations.map((i) => newVMScriptTokenFromScript(script, i))); On 2016/05/04 23:05:52, nweiz wrote: > "i" -> "location" (or "position") Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:21: .map((scriptItem) => newVMScriptRef(scope, scriptItem)) On 2016/05/04 23:05:52, nweiz wrote: > Just call this variable "script", like in the other similar code in this > package. Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:26: .map((rangeItem) => _newSourceReportRange(rangeItem, scripts))); On 2016/05/04 23:05:52, nweiz wrote: > Don't do actual work in the "new*" functions. It makes this library confusingly > different from all the others in the package, which makes it more difficult to > figure out what's happening where. Acknowledged. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:48: /// methods, constructors, etc.) On 2016/05/04 23:05:52, nweiz wrote: > Avoid abbreviations like "etc" > (https://www.dartlang.org/effective-dart/documentation/#avoid-abbreviations-an...) > > Also add a period. Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:53: /// * ranges may be duplicated, in the case of mixins. On 2016/05/04 23:05:52, nweiz wrote: > I think bulleted lists are generally harder to read than prose. It's certainly > harder to ensure that the markdown won't break somehow. Are you confident that > dartdoc will render this correctly? Replaced with sentences. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:64: VMScriptRef get script => location.script; On 2016/05/04 23:05:52, nweiz wrote: > Document all public members. Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:68: /// `true` if this range been compiled by the Dart VM. On 2016/05/04 23:05:52, nweiz wrote: > I like to say "Whether" rather than "`true` if" to avoid starting a sentence > with a lower-case identifier. Up to you, though. Acknowledged. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:72: /// Use `forceCompile` in `getSourceReport` to ensure the target scripts are On 2016/05/04 23:05:52, nweiz wrote: > Make "getSourceReport" a link. Also below. I didn't do this intentionally – since there are two of these. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:76: /// Token locations at which breakpoints can be set. On 2016/05/04 23:05:52, nweiz wrote: > "Token locations" -> "Token positions" Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:78: /// Provided only when the when the `includePossibleBreakpoints` in On 2016/05/04 23:05:52, nweiz wrote: > Remove "the". Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:82: /// A sorted list of token positions in a [VMSourceReportRang] which have been On 2016/05/04 23:05:52, nweiz wrote: > "Rang" -> "Range" Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:85: /// Provided only when the when the `includeCoverageReport` in On 2016/05/04 23:05:52, nweiz wrote: > Remove "the" Done. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:92: /// Provided only when the On 2016/05/04 23:05:52, nweiz wrote: > Remove "the" Done. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... File test/source_report_test.dart (right): https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:12: print("one"); On 2016/05/04 23:05:52, nweiz wrote: > What is going on with this indentation? Why are the two prints indented > differently? > > Either the quotes should be treated like brackets, in which case everything > should be indented two spaces and the closing quotes should be at the base > level; or this should be treated as a normal multiline string rather than a code > block, in which case its base indentation should be 0. I prefer the former. Right? https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:37: test("and no options", () async { On 2016/05/04 23:05:52, nweiz wrote: > The group name plus the test name should be a sentence describing the behavior > you're testing. Done. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:51: expect(startLocation.column, 0); On 2016/05/04 23:05:52, nweiz wrote: > Testing the explicit line and column is brittle, and it's not obvious to a > reader that these numbers are correct. You can get the actual source location of > the main method through isolate.rootLibrary (see the script tests I added). Then > you can just check that the corresponding source_span SourceLocations are ==. Done. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:74: expect(hitLocations, [6, 7, 8, 14]); On 2016/05/04 23:05:52, nweiz wrote: > Add a comment explaining what actual lines these represent. Also below. Done. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:118: VMSourceLocation mainFunctionLocation; On 2016/05/04 23:05:52, nweiz wrote: > I think just "mainLocation" would be fine here. Done. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:120: VMSourceLocation unusedFunc2Location; On 2016/05/04 23:05:52, nweiz wrote: > "Func" -> "Function" Done. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:143: var mainFunc = await rootLib.functions['main'].load(); On 2016/05/04 23:05:53, nweiz wrote: > "Func" -> "Function" Done. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:148: unusedFunc2Location = unusedFunc2Location = unusedFunction2.location; On 2016/05/04 23:05:52, nweiz wrote: > Extra assignment. Done. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:158: expect(firstRange.compiled, isFalse); On 2016/05/04 23:05:53, nweiz wrote: > Make assertions about the other properties as well. Also below. Done. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:159: var firstRangeLocation = On 2016/05/04 23:05:53, nweiz wrote: > Why is this called "Location" if it's an offset? Acknowledged. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:161: expect(firstRangeLocation, lessThan(mainFunctionSpan.start.offset)); On 2016/05/04 23:05:53, nweiz wrote: > You can just do > > expect(script.sourceSpan(firstRange.location), lessThan(mainFunctionSpan))) > > same below. Sadly, `lessThan` only uses the `>` operator. We should fix that... https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:185: test("with tokenPos set", () async { On 2016/05/04 23:05:52, nweiz wrote: > It's not called "tokenPos" (or "endTokenPos") anymore. Acknowledged. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:215: reason: 'Includes the range containing only unusedFunction2.'); On 2016/05/04 23:05:52, nweiz wrote: > Assert that this is in fact the range it contains. Done.
Ping?
https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart#new... lib/src/script.dart:93: int endTokenPos, On 2016/05/05 20:46:25, kevmoo wrote: > On 2016/05/04 23:05:51, nweiz wrote: > > On 2016/04/29 23:36:54, kevmoo wrote: > > > On 2016/04/29 00:52:43, nweiz wrote: > > > > These should come after [forceCompile]. That way the parameters that this > > call > > > > has in common with [Isolate.getSourceReport] are all together. > > > > > > Done > > > > > > > To preserve the abstraction, rather than having the user pass these > > > explicitly, > > > > they should probably pass a single VMSourceLocation. That was they can > > easily > > > > say "get the source report for this class" or whatever and we don't make > > them > > > > manually munge locations and risk getting it wrong. > > > > > > Disagree. There's no way to create a VMSourceLocation (or a VMSourceToken). > > > > > > A user may want to do math to skip everything before/after some range > they've > > > already seen. > > > > > > I may have a hit on 58 and a miss on 65. > > > > > > Totally valid to ask for a range starting at '59' as they are trying to hit > a > > > desired spot in the code. > > > > I don't understand how users would possibly get into a position where they're > > interested in non-semantic ranges like this. If they know they have a hit on > 58 > > and a miss on 65, and those are part of the same function, they already have > the > > source report for that whole function. > > > > It makes a lot of sense to get the source report for a given function or a > given > > class, and we should make that as easy as possible. It's not clear there's any > > reason to get arbitrary ranges between tokens, especially when you can just > > ignore some results from a wider source report, so it's nor worth warping the > > API to accommodate that entirely theoretical use-case. > > This was fixed – now uses tokens What I'm saying is that it should take a single SourceLocation. https://codereview.chromium.org/1929063002/diff/100001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1929063002/diff/100001/lib/src/isolate.dart#n... lib/src/isolate.dart:354: /// Otherwise, [VMSourceReportRange.coverage] is `null`. On 2016/05/05 20:46:26, kevmoo wrote: > On 2016/05/04 23:05:51, nweiz wrote: > > Reflow this text so there isn't unnecessary whitespace at the end of each > line. > > Same goes for a bunch of other paragraphs. Small diffs are less important than > > readable comments. > > Done – mostly. > > I like to start new sentences on new lines. Easier to read the source and less > likely to require cascading changes later That's contrary to the style elsewhere in this package, in Dart, and indeed in English prose in general. Please just reflow it. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... File lib/src/source_report.dart (right): https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:26: .map((rangeItem) => _newSourceReportRange(rangeItem, scripts))); On 2016/05/05 20:46:26, kevmoo wrote: > On 2016/05/04 23:05:52, nweiz wrote: > > Don't do actual work in the "new*" functions. It makes this library > confusingly > > different from all the others in the package, which makes it more difficult to > > figure out what's happening where. > > Acknowledged. It doesn't look like this was addressed. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:64: VMScriptRef get script => location.script; On 2016/05/05 20:46:27, kevmoo wrote: > On 2016/05/04 23:05:52, nweiz wrote: > > Document all public members. > > Done. This member is still not documented. https://codereview.chromium.org/1929063002/diff/100001/lib/src/source_report.... lib/src/source_report.dart:72: /// Use `forceCompile` in `getSourceReport` to ensure the target scripts are On 2016/05/05 20:46:26, kevmoo wrote: > On 2016/05/04 23:05:52, nweiz wrote: > > Make "getSourceReport" a link. Also below. > > I didn't do this intentionally – since there are two of these. Linking to one of them is way better than linking to zero. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... File test/source_report_test.dart (right): https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:37: test("and no options", () async { On 2016/05/05 20:46:28, kevmoo wrote: > On 2016/05/04 23:05:52, nweiz wrote: > > The group name plus the test name should be a sentence describing the behavior > > you're testing. > > Done. "behaves correctly" doesn't actually describe any behavior. https://codereview.chromium.org/1929063002/diff/100001/test/source_report_tes... test/source_report_test.dart:118: VMSourceLocation mainFunctionLocation; On 2016/05/05 20:46:28, kevmoo wrote: > On 2016/05/04 23:05:52, nweiz wrote: > > I think just "mainLocation" would be fine here. > > Done. This comment was intended to also apply to other similarly-named variables. https://codereview.chromium.org/1929063002/diff/140001/lib/src/source_report.... File lib/src/source_report.dart (right): https://codereview.chromium.org/1929063002/diff/140001/lib/src/source_report.... lib/src/source_report.dart:35: /// Rranges may be duplicated, in the case of mixins. "Rranges" -> "Ranges" https://codereview.chromium.org/1929063002/diff/140001/lib/src/source_report.... lib/src/source_report.dart:65: /// Token positions in a [VMSourceReportRange] which have been executed. "a [VMSourceReportRange]" -> "this range" same below https://codereview.chromium.org/1929063002/diff/140001/lib/src/source_report.... lib/src/source_report.dart:82: factory VMSourceReportRange._fromJson(Map json, List<VMScriptRef> scripts) { Document this. https://codereview.chromium.org/1929063002/diff/140001/lib/src/source_report.... lib/src/source_report.dart:109: static List<VMScriptToken> _getTokens( Document this as well. https://codereview.chromium.org/1929063002/diff/140001/lib/src/source_report.... lib/src/source_report.dart:111: if (locations == null) return null; It shouldn't be possible for this to be null. It's not listed as optional in the documentation. Have you seen it be null in your testing? https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... File test/source_report_test.dart (right): https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... test/source_report_test.dart:40: test("behaves correctly with options off", () async { The bulk of this test case isn't about the lack of information that comes from turning options off, it's about testing the locations. The description should describe what the test is actually testing. https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... test/source_report_test.dart:71: test("behaves correctly without breakpoints", () async { This test case is primarily making assertions about coverage; its description shouldn't be about breakpoints. Same below. https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... test/source_report_test.dart:81: // represents the unique set of lines that are executed https://www.dartlang.org/effective-dart/documentation/#do-format-comments-lik... https://www.dartlang.org/effective-dart/documentation/#prefer-starting-variab... Also, this is not what I meant by "what lines these represent". I want you to describe the Dart code being executed in the lines listed below. I want to understand why there's one chunk of three lines followed by two lines that are two apart from one another. https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... test/source_report_test.dart:160: test("not force compiled", () async { You didn't update this test name, or any others in this group. https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... test/source_report_test.dart:162: includeCoverageReport: true, includePossibleBreakpoints: true); You don't need to set these to true anymore, here or anywhere else. https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... test/source_report_test.dart:179: expect(script.sourceSpan(lastRange.location), mainFunctionSpan); "equals(mainFunctionSpan)" Also below
https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart#new... lib/src/script.dart:93: int endTokenPos, On 2016/05/11 23:03:30, nweiz wrote: > On 2016/05/05 20:46:25, kevmoo wrote: > > On 2016/05/04 23:05:51, nweiz wrote: > > > On 2016/04/29 23:36:54, kevmoo wrote: > > > > On 2016/04/29 00:52:43, nweiz wrote: > > > > > These should come after [forceCompile]. That way the parameters that > this > > > call > > > > > has in common with [Isolate.getSourceReport] are all together. > > > > > > > > Done > > > > > > > > > To preserve the abstraction, rather than having the user pass these > > > > explicitly, > > > > > they should probably pass a single VMSourceLocation. That was they can > > > easily > > > > > say "get the source report for this class" or whatever and we don't make > > > them > > > > > manually munge locations and risk getting it wrong. > > > > > > > > Disagree. There's no way to create a VMSourceLocation (or a > VMSourceToken). > > > > > > > > A user may want to do math to skip everything before/after some range > > they've > > > > already seen. > > > > > > > > I may have a hit on 58 and a miss on 65. > > > > > > > > Totally valid to ask for a range starting at '59' as they are trying to > hit > > a > > > > desired spot in the code. > > > > > > I don't understand how users would possibly get into a position where > they're > > > interested in non-semantic ranges like this. If they know they have a hit on > > 58 > > > and a miss on 65, and those are part of the same function, they already have > > the > > > source report for that whole function. > > > > > > It makes a lot of sense to get the source report for a given function or a > > given > > > class, and we should make that as easy as possible. It's not clear there's > any > > > reason to get arbitrary ranges between tokens, especially when you can just > > > ignore some results from a wider source report, so it's nor worth warping > the > > > API to accommodate that entirely theoretical use-case. > > > > This was fixed – now uses tokens > > What I'm saying is that it should take a single SourceLocation. But one cannot create a SourceLocation from whole cloth. So we'd be limiting ourselves to requesting sections of code for which the API has already provided a location. One cannot say "given me everything before X or after Y" https://codereview.chromium.org/1929063002/diff/100001/lib/src/isolate.dart File lib/src/isolate.dart (right): https://codereview.chromium.org/1929063002/diff/100001/lib/src/isolate.dart#n... lib/src/isolate.dart:354: /// Otherwise, [VMSourceReportRange.coverage] is `null`. On 2016/05/11 23:03:31, nweiz wrote: > On 2016/05/05 20:46:26, kevmoo wrote: > > On 2016/05/04 23:05:51, nweiz wrote: > > > Reflow this text so there isn't unnecessary whitespace at the end of each > > line. > > > Same goes for a bunch of other paragraphs. Small diffs are less important > than > > > readable comments. > > > > Done – mostly. > > > > I like to start new sentences on new lines. Easier to read the source and less > > likely to require cascading changes later > > That's contrary to the style elsewhere in this package, in Dart, and indeed in > English prose in general. Please just reflow it. Markdown ignores single-line breaks – so no humans are harmed. It doc comment easier to maintain.. ..but okay https://codereview.chromium.org/1929063002/diff/140001/lib/src/source_report.... File lib/src/source_report.dart (right): https://codereview.chromium.org/1929063002/diff/140001/lib/src/source_report.... lib/src/source_report.dart:35: /// Rranges may be duplicated, in the case of mixins. On 2016/05/11 23:03:31, nweiz wrote: > "Rranges" -> "Ranges" Done. https://codereview.chromium.org/1929063002/diff/140001/lib/src/source_report.... lib/src/source_report.dart:65: /// Token positions in a [VMSourceReportRange] which have been executed. On 2016/05/11 23:03:31, nweiz wrote: > "a [VMSourceReportRange]" -> "this range" > > same below Done. https://codereview.chromium.org/1929063002/diff/140001/lib/src/source_report.... lib/src/source_report.dart:82: factory VMSourceReportRange._fromJson(Map json, List<VMScriptRef> scripts) { On 2016/05/11 23:03:31, nweiz wrote: > Document this. Done. https://codereview.chromium.org/1929063002/diff/140001/lib/src/source_report.... lib/src/source_report.dart:109: static List<VMScriptToken> _getTokens( On 2016/05/11 23:03:31, nweiz wrote: > Document this as well. Done. https://codereview.chromium.org/1929063002/diff/140001/lib/src/source_report.... lib/src/source_report.dart:111: if (locations == null) return null; On 2016/05/11 23:03:31, nweiz wrote: > It shouldn't be possible for this to be null. It's not listed as optional in the > documentation. Have you seen it be null in your testing? Acknowledged. https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... File test/source_report_test.dart (right): https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... test/source_report_test.dart:40: test("behaves correctly with options off", () async { On 2016/05/11 23:03:32, nweiz wrote: > The bulk of this test case isn't about the lack of information that comes from > turning options off, it's about testing the locations. The description should > describe what the test is actually testing. Done. https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... test/source_report_test.dart:71: test("behaves correctly without breakpoints", () async { On 2016/05/11 23:03:32, nweiz wrote: > This test case is primarily making assertions about coverage; its description > shouldn't be about breakpoints. Same below. Done. https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... test/source_report_test.dart:81: // represents the unique set of lines that are executed On 2016/05/11 23:03:31, nweiz wrote: > https://www.dartlang.org/effective-dart/documentation/#do-format-comments-lik... > https://www.dartlang.org/effective-dart/documentation/#prefer-starting-variab... > > Also, this is not what I meant by "what lines these represent". I want you to > describe the Dart code being executed in the lines listed below. I want to > understand why there's one chunk of three lines followed by two lines that are > two apart from one another. Acknowledged. https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... test/source_report_test.dart:160: test("not force compiled", () async { On 2016/05/11 23:03:32, nweiz wrote: > You didn't update this test name, or any others in this group. Acknowledged. https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... test/source_report_test.dart:162: includeCoverageReport: true, includePossibleBreakpoints: true); On 2016/05/11 23:03:31, nweiz wrote: > You don't need to set these to true anymore, here or anywhere else. Acknowledged. https://codereview.chromium.org/1929063002/diff/140001/test/source_report_tes... test/source_report_test.dart:179: expect(script.sourceSpan(lastRange.location), mainFunctionSpan); On 2016/05/11 23:03:31, nweiz wrote: > "equals(mainFunctionSpan)" > > Also below Done.
https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart#new... lib/src/script.dart:93: int endTokenPos, On 2016/05/12 05:22:34, kevmoo wrote: > On 2016/05/11 23:03:30, nweiz wrote: > > On 2016/05/05 20:46:25, kevmoo wrote: > > > On 2016/05/04 23:05:51, nweiz wrote: > > > > On 2016/04/29 23:36:54, kevmoo wrote: > > > > > On 2016/04/29 00:52:43, nweiz wrote: > > > > > > These should come after [forceCompile]. That way the parameters that > > this > > > > call > > > > > > has in common with [Isolate.getSourceReport] are all together. > > > > > > > > > > Done > > > > > > > > > > > To preserve the abstraction, rather than having the user pass these > > > > > explicitly, > > > > > > they should probably pass a single VMSourceLocation. That was they can > > > > easily > > > > > > say "get the source report for this class" or whatever and we don't > make > > > > them > > > > > > manually munge locations and risk getting it wrong. > > > > > > > > > > Disagree. There's no way to create a VMSourceLocation (or a > > VMSourceToken). > > > > > > > > > > A user may want to do math to skip everything before/after some range > > > they've > > > > > already seen. > > > > > > > > > > I may have a hit on 58 and a miss on 65. > > > > > > > > > > Totally valid to ask for a range starting at '59' as they are trying to > > hit > > > a > > > > > desired spot in the code. > > > > > > > > I don't understand how users would possibly get into a position where > > they're > > > > interested in non-semantic ranges like this. If they know they have a hit > on > > > 58 > > > > and a miss on 65, and those are part of the same function, they already > have > > > the > > > > source report for that whole function. > > > > > > > > It makes a lot of sense to get the source report for a given function or a > > > given > > > > class, and we should make that as easy as possible. It's not clear there's > > any > > > > reason to get arbitrary ranges between tokens, especially when you can > just > > > > ignore some results from a wider source report, so it's nor worth warping > > the > > > > API to accommodate that entirely theoretical use-case. > > > > > > This was fixed – now uses tokens > > > > What I'm saying is that it should take a single SourceLocation. > > But one cannot create a SourceLocation from whole cloth. So we'd be limiting > ourselves to requesting sections of code for which the API has already provided > a location. One cannot say "given me everything before X or after Y" That is not a use-case I'm concerned about. I'm tired of going around in circles about this. Please just make the change. https://codereview.chromium.org/1929063002/diff/160001/test/source_report_tes... File test/source_report_test.dart (right): https://codereview.chromium.org/1929063002/diff/160001/test/source_report_tes... test/source_report_test.dart:20: Isolate.current.kill();"""; Why did you make this change? If the trailing quotes are on the same line as the last line of code, you need to indent this zero spaces, since you're formatting it like a string literal rather than like a code block.
https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/40001/lib/src/script.dart#new... lib/src/script.dart:93: int endTokenPos, On 2016/05/17 20:54:41, nweiz wrote: > On 2016/05/12 05:22:34, kevmoo wrote: > > On 2016/05/11 23:03:30, nweiz wrote: > > > On 2016/05/05 20:46:25, kevmoo wrote: > > > > On 2016/05/04 23:05:51, nweiz wrote: > > > > > On 2016/04/29 23:36:54, kevmoo wrote: > > > > > > On 2016/04/29 00:52:43, nweiz wrote: > > > > > > > These should come after [forceCompile]. That way the parameters that > > > this > > > > > call > > > > > > > has in common with [Isolate.getSourceReport] are all together. > > > > > > > > > > > > Done > > > > > > > > > > > > > To preserve the abstraction, rather than having the user pass these > > > > > > explicitly, > > > > > > > they should probably pass a single VMSourceLocation. That was they > can > > > > > easily > > > > > > > say "get the source report for this class" or whatever and we don't > > make > > > > > them > > > > > > > manually munge locations and risk getting it wrong. > > > > > > > > > > > > Disagree. There's no way to create a VMSourceLocation (or a > > > VMSourceToken). > > > > > > > > > > > > A user may want to do math to skip everything before/after some range > > > > they've > > > > > > already seen. > > > > > > > > > > > > I may have a hit on 58 and a miss on 65. > > > > > > > > > > > > Totally valid to ask for a range starting at '59' as they are trying > to > > > hit > > > > a > > > > > > desired spot in the code. > > > > > > > > > > I don't understand how users would possibly get into a position where > > > they're > > > > > interested in non-semantic ranges like this. If they know they have a > hit > > on > > > > 58 > > > > > and a miss on 65, and those are part of the same function, they already > > have > > > > the > > > > > source report for that whole function. > > > > > > > > > > It makes a lot of sense to get the source report for a given function or > a > > > > given > > > > > class, and we should make that as easy as possible. It's not clear > there's > > > any > > > > > reason to get arbitrary ranges between tokens, especially when you can > > just > > > > > ignore some results from a wider source report, so it's nor worth > warping > > > the > > > > > API to accommodate that entirely theoretical use-case. > > > > > > > > This was fixed – now uses tokens > > > > > > What I'm saying is that it should take a single SourceLocation. > > > > But one cannot create a SourceLocation from whole cloth. So we'd be limiting > > ourselves to requesting sections of code for which the API has already > provided > > a location. One cannot say "given me everything before X or after Y" > > That is not a use-case I'm concerned about. > > I'm tired of going around in circles about this. Please just make the change. Acknowledged. https://codereview.chromium.org/1929063002/diff/160001/test/source_report_tes... File test/source_report_test.dart (right): https://codereview.chromium.org/1929063002/diff/160001/test/source_report_tes... test/source_report_test.dart:20: Isolate.current.kill();"""; On 2016/05/17 20:54:41, nweiz wrote: > Why did you make this change? > > If the trailing quotes are on the same line as the last line of code, you need > to indent this zero spaces, since you're formatting it like a string literal > rather than like a code block. Acknowledged. https://codereview.chromium.org/1929063002/diff/180001/lib/vm_service_client.... File lib/vm_service_client.dart (right): https://codereview.chromium.org/1929063002/diff/180001/lib/vm_service_client.... lib/vm_service_client.dart:132: factory VMServiceClient(StreamChannel<Object> channel) => This was blowing up with the latest strong-clean packages. Fixed to keep analyzer happy
https://codereview.chromium.org/1929063002/diff/180001/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/180001/lib/src/script.dart#ne... lib/src/script.dart:113: params['endTokenPos'] = location.end._position; A location without an end represents a single point in the program, whereas not passing the end token here represents everything after the token, so this isn't the correct behavior. I suspect nothing useful will happen if you pass a single-position location, so it should probably just throw an [ArgumentError]. Also document that behavior. https://codereview.chromium.org/1929063002/diff/180001/lib/vm_service_client.... File lib/vm_service_client.dart (right): https://codereview.chromium.org/1929063002/diff/180001/lib/vm_service_client.... lib/vm_service_client.dart:132: factory VMServiceClient(StreamChannel<Object> channel) => On 2016/05/17 22:12:01, kevmoo wrote: > This was blowing up with the latest strong-clean packages. Fixed to keep > analyzer happy This should definitely take a StreamChannel<String>. What analyzer issues were you seeing? https://codereview.chromium.org/1929063002/diff/180001/test/source_report_tes... File test/source_report_test.dart (right): https://codereview.chromium.org/1929063002/diff/180001/test/source_report_tes... test/source_report_test.dart:208: }); Test what happens with a point location (that is, one without an end token). I think you can get one from a field in a class.
https://codereview.chromium.org/1929063002/diff/180001/lib/src/script.dart File lib/src/script.dart (right): https://codereview.chromium.org/1929063002/diff/180001/lib/src/script.dart#ne... lib/src/script.dart:113: params['endTokenPos'] = location.end._position; On 2016/05/19 21:11:13, nweiz wrote: > A location without an end represents a single point in the program, whereas not > passing the end token here represents everything after the token, so this isn't > the correct behavior. I suspect nothing useful will happen if you pass a > single-position location, so it should probably just throw an [ArgumentError]. > Also document that behavior. Acknowledged. https://codereview.chromium.org/1929063002/diff/180001/lib/vm_service_client.... File lib/vm_service_client.dart (right): https://codereview.chromium.org/1929063002/diff/180001/lib/vm_service_client.... lib/vm_service_client.dart:132: factory VMServiceClient(StreamChannel<Object> channel) => On 2016/05/19 21:11:13, nweiz wrote: > On 2016/05/17 22:12:01, kevmoo wrote: > > This was blowing up with the latest strong-clean packages. Fixed to keep > > analyzer happy > > This should definitely take a StreamChannel<String>. What analyzer issues were > you seeing? INFO: The argument type 'JsonDocumentTransformer' cannot be assigned to the parameter type 'StreamChannelTransformer<String, dynamic>'. ([vm_service_client] lib/vm_service_client.dart:133) https://codereview.chromium.org/1929063002/diff/180001/test/source_report_tes... File test/source_report_test.dart (right): https://codereview.chromium.org/1929063002/diff/180001/test/source_report_tes... test/source_report_test.dart:208: }); On 2016/05/19 21:11:13, nweiz wrote: > Test what happens with a point location (that is, one without an end token). I > think you can get one from a field in a class. Acknowledged.
https://codereview.chromium.org/1929063002/diff/180001/lib/vm_service_client.... File lib/vm_service_client.dart (right): https://codereview.chromium.org/1929063002/diff/180001/lib/vm_service_client.... lib/vm_service_client.dart:132: factory VMServiceClient(StreamChannel<Object> channel) => On 2016/05/19 21:59:46, kevmoo wrote: > On 2016/05/19 21:11:13, nweiz wrote: > > On 2016/05/17 22:12:01, kevmoo wrote: > > > This was blowing up with the latest strong-clean packages. Fixed to keep > > > analyzer happy > > > > This should definitely take a StreamChannel<String>. What analyzer issues were > > you seeing? > > INFO: The argument type 'JsonDocumentTransformer' cannot be assigned to the > parameter type 'StreamChannelTransformer<String, dynamic>'. ([vm_service_client] > lib/vm_service_client.dart:133) Hmm, this might actually be a StreamChannel bug. Use jsonDocument.bind(channel) for now.
https://codereview.chromium.org/1929063002/diff/180001/lib/vm_service_client.... File lib/vm_service_client.dart (right): https://codereview.chromium.org/1929063002/diff/180001/lib/vm_service_client.... lib/vm_service_client.dart:132: factory VMServiceClient(StreamChannel<Object> channel) => On 2016/05/19 22:47:15, nweiz wrote: > On 2016/05/19 21:59:46, kevmoo wrote: > > On 2016/05/19 21:11:13, nweiz wrote: > > > On 2016/05/17 22:12:01, kevmoo wrote: > > > > This was blowing up with the latest strong-clean packages. Fixed to keep > > > > analyzer happy > > > > > > This should definitely take a StreamChannel<String>. What analyzer issues > were > > > you seeing? > > > > INFO: The argument type 'JsonDocumentTransformer' cannot be assigned to the > > parameter type 'StreamChannelTransformer<String, dynamic>'. > ([vm_service_client] > > lib/vm_service_client.dart:133) > > Hmm, this might actually be a StreamChannel bug. Use jsonDocument.bind(channel) > for now. Done.
lgtm
Description was changed from ========== add getSourceReport to VMServiceReference closes https://github.com/dart-lang/vm_service_client/issues/6 ========== to ========== add getSourceReport to VMServiceReference closes https://github.com/dart-lang/vm_service_client/issues/6 R=nweiz@google.com Committed: https://github.com/dart-lang/vm_service_client/commit/167eb9cdad5d216e160c8f4... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) manually as 167eb9cdad5d216e160c8f4751ca288a7d05d3ac (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
