[S] Change in dart/sdk[main]: [vm/service]: expose Pointer<T> address in VM Service Protocol
Nourhan Hasan (Gerrit)
Nourhan Hasan has uploaded the change for review![]( "Open in Gerrit")
Commit message
[vm/service]: expose Pointer<T> address in VM Service Protocol
Pointer<T> instances were previously serialized as PlainInstance
with no address, making them opaque to all debugger clients.
Change information
- M pkg/observatory/lib/src/models/objects/instance.dart
- M pkg/observatory/lib/src/service/object.dart
- M pkg/vm_service/CHANGELOG.md
- M pkg/vm_service/java/version.properties
- M pkg/vm_service/lib/src/vm_service.dart
- M pkg/vm_service_interface/lib/src/vm_service_interface.dart
- M runtime/vm/object_service.cc
- M runtime/vm/service.h
- M runtime/vm/service/service.md
Related details
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 4 comments![]( "Open in Gerrit")
I don't see any tests.
Does the service protocol have a test suite where we can add a test?
pointer,Nit: All the other entries have a comment.
// Preserve RPCError exceptions as-is.unrelated change?
/// An instance of the dart:ffi Pointer<T> class.You might want to add a comment that `T` is erased at runtime.
Related details
- Ben Konyi
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nourhan Hasan (Gerrit)
Nourhan Hasan added 1 comment![]( "Open in Gerrit")
// Preserve RPCError exceptions as-is.unrelated change?
- Ben Konyi
- Daco Harkes
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daco Harkes (Gerrit)
Daco Harkes added 1 comment![]( "Open in Gerrit")
// Preserve RPCError exceptions as-is.Nourhan Hasanunrelated change?
This was removed when I ran: `dart tool/generate.dart`
You can use `git blame` to figure out why changes were made.
It looks like this was a fix recently: https://dart-review.googlesource.com/c/sdk/+/458040
And probably that fix did not run the generator, so the person forgot to run the generator most likely. I think the right thing to do here is to update the generator. (If it's a small fix, can be in this CL. Otherwise, its better as a separate CL.)
Related details
- Ben Konyi
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nourhan Hasan (Gerrit)
Nourhan Hasan added 1 comment![]( "Open in Gerrit")
I don't see any tests.
Does the service protocol have a test suite where we can add a test?
This is the file: pkg/vm_service/test/get_object_rpc_test.dart
I will add tests here.
Related details
- Ben Konyi
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nourhan Hasan (Gerrit)
Nourhan Hasan added 1 comment![]( "Open in Gerrit")
// Preserve RPCError exceptions as-is.Nourhan Hasanunrelated change?
Daco HarkesThis was removed when I ran: `dart tool/generate.dart`
You can use `git blame` to figure out why changes were made.
It looks like this was a fix recently: https://dart-review.googlesource.com/c/sdk/+/458040
And probably that fix did not run the generator, so the person forgot to run the generator most likely. I think the right thing to do here is to update the generator. (If it's a small fix, can be in this CL. Otherwise, its better as a separate CL.)
- Ben Konyi
- Daco Harkes
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 added 3 comments![]( "Open in Gerrit")
/// An instance of the dart:ffi Pointer<T> class.You might want to add a comment that `T` is erased at runtime.
Is it even worth mentioning the type argument?
const TypeArguments& type_args =As Daco mentioned, the type arguments are erased for pointers. I think we should just remove this.
Related details
- Daco Harkes
- Nourhan Hasan
Code-Owners
Code-Review
Commit-Message-Has-TEST
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |