[decorators] Add decorators context and access object. [v8/v8 : main]
Luis Pardo (Gerrit)
Luis Pardo added 1 comment![]( "Open in Gerrit")
@s...@chromium.org PTAL when you have a chance, this is a simple CL but a lot of wiring.
Related details
- Shu-yu Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 1 comment![]( "Open in Gerrit")
ACCESSORS(JSClassDecoratorContextObject, kind, Tagged<Object>, kKindOffset)I'd like to follow your suggestion and add a dcheck to verify that the map has not change, that would look something like:
```
ACCESSORS_CHECKED(JSClassElementDecoratorContextObject, kind, Tagged<Object>,
kKindOffset, IsJSClassDecoratorContextObjectMap(map())
```
But there isn't an automatically generated IsJSClassDecoratorContextObjectMap and I'm not sure what is the right way to code it since the map is cached in the native context.
Alternatively, I could write getters/setters that receive the native context as an argument and perform the dcheck. That's fine but I'd like us to agree that it is the best solution before adding that code.
Related details
- Shu-yu Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 1 comment![]( "Open in Gerrit")
Related details
- Shu-yu Guo
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 2 comments![]( "Open in Gerrit")
transitioning javascript builtin DecoratorAccessGet(The access objects for private methods and accessors must close around the brand and the object itself. I'll add 3 new builtins with the methods/accessors/auto-accessors CLS.
if (!HasProperty(obj, name)) {
ThrowTypeError(MessageTemplate::kInvalidPrivateMemberWrite, name);
}
SetProperty(obj, name, value);Performing 2 checks here is suboptimal, but I didn't find any CSA code that could be used to throw when the private property is not present. This CL is already XL, if we want to optimize this, I can add a TODO and do it as a follow up.
Related details
- Shu-yu Guo
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Luis Pardo
| Commit-Queue | +1 |
1 comment
@ol...@chromium.org Started rebasing this CLS. Regenerated the static roots with `gen-static-roots.py` but seems like I'm missing something for the "no wasm" versions. Any advice?
Related details
- Luis Pardo
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
@ol...@chromium.org Started rebasing this CLS. Regenerated the static roots with `gen-static-roots.py` but seems like I'm missing something for the "no wasm" versions. Any advice?
maybe you have an old version of that script? if you pass no args it should create all 4 versions we currently have. you have to check in the modified roots headers.
Related details
- Luis Pardo
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 1 comment![]( "Open in Gerrit")
Olivier Flückiger@ol...@chromium.org Started rebasing this CLS. Regenerated the static roots with `gen-static-roots.py` but seems like I'm missing something for the "no wasm" versions. Any advice?
maybe you have an old version of that script? if you pass no args it should create all 4 versions we currently have. you have to check in the modified roots headers.
Those 4 headers are already included in this CL but the bot is still failing. Am I adding the new roots correctly?
Related details
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 1 comment![]( "Open in Gerrit")
Olivier Flückiger@ol...@chromium.org Started rebasing this CLS. Regenerated the static roots with `gen-static-roots.py` but seems like I'm missing something for the "no wasm" versions. Any advice?
Luis Pardomaybe you have an old version of that script? if you pass no args it should create all 4 versions we currently have. you have to check in the modified roots headers.
Those 4 headers are already included in this CL but the bot is still failing. Am I adding the new roots correctly?
Ah, I see. some of the values are exported in the api and you need to manually update the value (i.e., copy the value from the static roots header to the place where the static assert fails). We should really improve that workflow at some point...
Related details
- Luis Pardo
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 2 comments![]( "Open in Gerrit")
Olivier Flückiger@ol...@chromium.org Started rebasing this CLS. Regenerated the static roots with `gen-static-roots.py` but seems like I'm missing something for the "no wasm" versions. Any advice?
Luis Pardomaybe you have an old version of that script? if you pass no args it should create all 4 versions we currently have. you have to check in the modified roots headers.
Olivier FlückigerThose 4 headers are already included in this CL but the bot is still failing. Am I adding the new roots correctly?
Ah, I see. some of the values are exported in the api and you need to manually update the value (i.e., copy the value from the static roots header to the place where the static assert fails). We should really improve that workflow at some point...
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 8 comments![]( "Open in Gerrit")
thanks. finally a first round of comments on this one.
TFS(PrivateGet, NeedsContext::kYes, kObject, kKey) \should we name this GetPrivateProperty to be more consistent
transitioning javascript builtin DecoratorAccessGet(The access objects for private methods and accessors must close around the brand and the object itself. I'll add 3 new builtins with the methods/accessors/auto-accessors CLS.
ok, can you leave a TODO?
if (Is<String>(name)) {It is a bit surprising that we have to check this here. At the very least we should have two different accessor implementations, one for private symbols and one for normal names. We then should create the one we need depending on the name when we create the context object.
if (!HasProperty(obj, name)) {
ThrowTypeError(MessageTemplate::kInvalidPrivateMemberWrite, name);
}
SetProperty(obj, name, value);Performing 2 checks here is suboptimal, but I didn't find any CSA code that could be used to throw when the private property is not present. This CL is already XL, if we want to optimize this, I can add a TODO and do it as a follow up.
Please add a TODO. That should definitely be fixed.
ACCESSORS(JSClassDecoratorContextObject, kind, Tagged<Object>, kKindOffset)I'd like to follow your suggestion and add a dcheck to verify that the map has not change, that would look something like:
```
ACCESSORS_CHECKED(JSClassElementDecoratorContextObject, kind, Tagged<Object>,
kKindOffset, IsJSClassDecoratorContextObjectMap(map())
```But there isn't an automatically generated IsJSClassDecoratorContextObjectMap and I'm not sure what is the right way to code it since the map is cached in the native context.
Alternatively, I could write getters/setters that receive the native context as an argument and perform the dcheck. That's fine but I'd like us to agree that it is the best solution before adding that code.
I am not sure I understand the question. If you mutate the context object, then it's map will change, so the dcheck you want to add will not hold anymore.
DECL_ACCESSORS(add_initializer, Tagged<Object>)Please add a TODO that we would want to eventually statically initialize this field.
DECL_ACCESSORS(access, Tagged<Object>)Please add a TODO that we would want to eventually statically initialize this field.
Related details
- Luis Pardo
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 3 comments![]( "Open in Gerrit")
ACCESSORS(JSClassDecoratorContextObject, kind, Tagged<Object>, kKindOffset)Olivier FlückigerI'd like to follow your suggestion and add a dcheck to verify that the map has not change, that would look something like:
```
ACCESSORS_CHECKED(JSClassElementDecoratorContextObject, kind, Tagged<Object>,
kKindOffset, IsJSClassDecoratorContextObjectMap(map())
```But there isn't an automatically generated IsJSClassDecoratorContextObjectMap and I'm not sure what is the right way to code it since the map is cached in the native context.
Alternatively, I could write getters/setters that receive the native context as an argument and perform the dcheck. That's fine but I'd like us to agree that it is the best solution before adding that code.
I am not sure I understand the question. If you mutate the context object, then it's map will change, so the dcheck you want to add will not hold anymore.
Exactly. These accessors should only be used before the object is exposed to JS (and the context could be mutated), so Shu suggested to add a CHECK so this would fail if used after the map changed.
DECL_ACCESSORS(add_initializer, Tagged<Object>)Please add a TODO that we would want to eventually statically initialize this field.
This could be statically initialized but the way we access the value in C++ benefits from doing it dynamically. See [this](https://chromium-review.googlesource.com/c/v8/v8/+/5837979/16#:~:text=Runtime_ApplyDecoratorsToFieldDefinition)
DECL_ACCESSORS(access, Tagged<Object>)Please add a TODO that we would want to eventually statically initialize this field.
Access functions depend on the name/symbol of the element, why do we want to statically initialize them?
Related details
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivier Flückiger (Gerrit)
Olivier Flückiger added 2 comments![]( "Open in Gerrit")
DECL_ACCESSORS(add_initializer, Tagged<Object>)Luis PardoPlease add a TODO that we would want to eventually statically initialize this field.
This could be statically initialized but the way we access the value in C++ benefits from doing it dynamically. See [this](https://chromium-review.googlesource.com/c/v8/v8/+/5837979/16#:~:text=Runtime_ApplyDecoratorsToFieldDefinition)
sorry, I meant to say "lazily"
DECL_ACCESSORS(access, Tagged<Object>)Luis PardoPlease add a TODO that we would want to eventually statically initialize this field.
Access functions depend on the name/symbol of the element, why do we want to statically initialize them?
- Luis Pardo
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 7 comments![]( "Open in Gerrit")
TFS(PrivateGet, NeedsContext::kYes, kObject, kKey) \should we name this GetPrivateProperty to be more consistent
Done
transitioning javascript builtin DecoratorAccessGet(Olivier FlückigerThe access objects for private methods and accessors must close around the brand and the object itself. I'll add 3 new builtins with the methods/accessors/auto-accessors CLS.
ok, can you leave a TODO?
Done
It is a bit surprising that we have to check this here. At the very least we should have two different accessor implementations, one for private symbols and one for normal names. We then should create the one we need depending on the name when we create the context object.
Done
if (!HasProperty(obj, name)) {
ThrowTypeError(MessageTemplate::kInvalidPrivateMemberWrite, name);
}
SetProperty(obj, name, value);Olivier FlückigerPerforming 2 checks here is suboptimal, but I didn't find any CSA code that could be used to throw when the private property is not present. This CL is already XL, if we want to optimize this, I can add a TODO and do it as a follow up.
Please add a TODO. That should definitely be fixed.
Done
Luis PardoPlease add a TODO that we would want to eventually statically initialize this field.
Olivier FlückigerThis could be statically initialized but the way we access the value in C++ benefits from doing it dynamically. See [this](https://chromium-review.googlesource.com/c/v8/v8/+/5837979/16#:~:text=Runtime_ApplyDecoratorsToFieldDefinition)
sorry, I meant to say "lazily"
Done
Luis PardoPlease add a TODO that we would want to eventually statically initialize this field.
Olivier FlückigerAccess functions depend on the name/symbol of the element, why do we want to statically initialize them?
sorry, I meant to say "lazily". thanks for clarifying
- Olivier Flückiger
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |