[L] Change in dart/sdk[main]: [ Service ] Add expression evaluation support to dart_runtime_service_vm
Ben Konyi (Gerrit)
Ben Konyi voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Jessy Yameogo
- Nicholas Shahan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Shahan (Gerrit)
Nicholas Shahan added 3 comments![]( "Open in Gerrit")
Future<RpcResponse> evaluate(json_rpc.Parameters parameters);Are we hoping to align these parameters across the various implementations? Right now it's not obvious what the parameters are.
const kResult = 'result';
const kError = 'error';
const kCode = 'code';
const kMessage = 'message';
const kData = 'data';Should we define these somewhere so they can be reused by other runtime services?
OK to punt of that until we start implementing the next runtime service though.
final data = (converted[kError]! as Map<String, Object?>)[kData];Is the `kData` key not always present? This could warrant a comment here explaining why this isn't part of the destructuring case.
Related details
- Ben Konyi
- Jessy Yameogo
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Ben Konyi voted and added 3 comments![]( "Open in Gerrit")
Votes added by Ben Konyi
| Commit-Queue | +1 |
3 comments
Future<RpcResponse> evaluate(json_rpc.Parameters parameters);Are we hoping to align these parameters across the various implementations? Right now it's not obvious what the parameters are.
Yeah, these parameters will be identical across implementations. I passed the raw parameters here since then I could simply forward them directly to the internal RPCs and not have to rebuild a new `Parameters` object. However, it's probably better to be more explicit than to try and save a few cycles and small memory allocations.
I've gone ahead and also added some documentation explaining what the expected parameters are.
const kResult = 'result';
const kError = 'error';
const kCode = 'code';
const kMessage = 'message';
const kData = 'data';Should we define these somewhere so they can be reused by other runtime services?
OK to punt of that until we start implementing the next runtime service though.
I'm not sure we'll actually need to. The main reason these are defined here is because we're parsing full JSON-RPC responses from the native VM and transforming them into a returned result or thrown RpcException that `package:json_rpc_2` can handle.
For the other services, we (probably) won't have this same transformation and will most likely be building the responses from scratch in Dart code. If we need these constants, we can find a better home for them then 😊
final data = (converted[kError]! as Map<String, Object?>)[kData];Is the `kData` key not always present? This could warrant a comment here explaining why this isn't part of the destructuring case.
That's correct, `kData` isn't always present. I'll add a comment.
Related details
- Jessy Yameogo
- Nicholas Shahan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Shahan (Gerrit)
Nicholas Shahan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nicholas Shahan
| Code-Review | +1 |
1 comment
String toString() => logger.name;Related details
- Ben Konyi
- Jessy Yameogo
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Konyi (Gerrit)
Ben Konyi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ben Konyi
| Commit-Queue | +2 |
1 comment
Is it intentional to use the `logger.name` in the `Client.toString()`?
Yes, since I wanted the same name as the logger. I've gone ahead and inverted this so `toString() => 'Client ($name)'` and `Logger get logger => Logger(toString())` which is hopefully a bit clearer.
Related details
- Jessy Yameogo
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: pkg/dart_runtime_service_vm/lib/src/vm_expression_evaluator.dart
Insertions: 2, Deletions: 1.
@@ -177,7 +177,8 @@
try {
if (externalClient != null) {
logger.info(
- 'Found external $kExternalCompileExpressionRpc service: $externalClient',
+ 'Found external $kExternalCompileExpressionRpc service: '
+ '$externalClient',
);
result = await externalClient.sendRequest(
method: kExternalCompileExpressionRpc,
```
```
The name of the file: pkg/dart_runtime_service/lib/src/clients.dart
Insertions: 2, Deletions: 2.
@@ -44,7 +44,7 @@
final DartRuntimeServiceBackend backend;
/// The logger to be used when handling requests from this client.
- Logger get logger => Logger('Client ($name)');
+ Logger get logger => Logger(toString());
/// A [Future] that completes when [close] is invoked.
late final Future<void> done;
@@ -173,7 +173,7 @@
late String _name;
@override
- String toString() => logger.name;
+ String toString() => 'Client ($name)';
}
/// Used for keeping track and managing clients that are connected to a given
```
Change information
[ Service ] Add expression evaluation support to dart_runtime_service_vm
Brings pass rate of package:vm_service tests to ~85%
TEST=Manual
Change-Id: I2335d98358362110add93c2acfe51c3fc1385237
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/487560
Reviewed-by: Nicholas Shahan <nsh...@google.com>
Commit-Queue: Ben Konyi <bko...@google.com>
- M pkg/dart_runtime_service/lib/dart_runtime_service.dart
- M pkg/dart_runtime_service/lib/src/clients.dart
- M pkg/dart_runtime_service/lib/src/dart_runtime_service.dart
- M pkg/dart_runtime_service/lib/src/dart_runtime_service_backend.dart
- M pkg/dart_runtime_service/lib/src/dart_runtime_service_rpcs.dart
- M pkg/dart_runtime_service/lib/src/event_streams.dart
- A pkg/dart_runtime_service/lib/src/expression_evaluator.dart
- M pkg/dart_runtime_service/lib/src/rpc_exceptions.dart
- M pkg/dart_runtime_service/lib/src/utils.dart
- M pkg/dart_runtime_service/test/utils/mocks.dart
- M pkg/dart_runtime_service_vm/lib/dart_runtime_service_vm.dart
- M pkg/dart_runtime_service_vm/lib/src/native_bindings.dart
- A pkg/dart_runtime_service_vm/lib/src/vm_expression_evaluator.dart
- M pkg/vm_service/test/evaluate_optimized_out_variable_test.dart
- M pkg/vm_service/test/evaluate_uninitialized_late_variable_test.dart
- M pkg/vm_service/test/external_service_registration_test.dart
- M pkg/vm_service/test/get_instances_as_list_rpc_expression_evaluation_on_internal_test.dart
- M pkg/vm_service/test/stream_subscription_test.dart
- M runtime/bin/dartutils.cc
- M runtime/bin/dartutils.h
Code-Review: +1 by Nicholas Shahan
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |