-
Notifications
You must be signed in to change notification settings - Fork 29.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tls_wrap: inherit from the AsyncWrap
first
#4268
Conversation
`WrapperInfo` casts pointer in JS object's internal field to `AsyncWrap`. This approach fails miserably for `TLSWrap` because it was inhereted from the `StreamBase` first, creating different kind of `vtable` for the whole class. Reorder parent classes to put `AsyncWrap` first. Fix: nodejs#4250
I'm not exactly sure how to test it, perhaps @bnoordhuis has some suggestions? R=@trevnorris or @bnoordhuis |
@indutny Thanks |
CI is green |
LGTM I am curious tho, could changing the order here potentially break anything? |
@jasnell I don't think so. From user perspective this class is not exposed, from the core perspective all casts seems to be proper on C++ side. |
Landed in e0bb118, thank you! |
`WrapperInfo` casts pointer in JS object's internal field to `AsyncWrap`. This approach fails miserably for `TLSWrap` because it was inhereted from the `StreamBase` first, creating different kind of `vtable` for the whole class. Reorder parent classes to put `AsyncWrap` first. Fix: #4250 PR-URL: #4268 Reviewed-By: James M Snell <jasnell@gmail.com>
Presumably it's a simple check that there's no static cast elsewhere to a |
`WrapperInfo` casts pointer in JS object's internal field to `AsyncWrap`. This approach fails miserably for `TLSWrap` because it was inhereted from the `StreamBase` first, creating different kind of `vtable` for the whole class. Reorder parent classes to put `AsyncWrap` first. Fix: #4250 PR-URL: #4268 Reviewed-By: James M Snell <jasnell@gmail.com>
`WrapperInfo` casts pointer in JS object's internal field to `AsyncWrap`. This approach fails miserably for `TLSWrap` because it was inhereted from the `StreamBase` first, creating different kind of `vtable` for the whole class. Reorder parent classes to put `AsyncWrap` first. Fix: #4250 PR-URL: #4268 Reviewed-By: James M Snell <jasnell@gmail.com>
`WrapperInfo` casts pointer in JS object's internal field to `AsyncWrap`. This approach fails miserably for `TLSWrap` because it was inhereted from the `StreamBase` first, creating different kind of `vtable` for the whole class. Reorder parent classes to put `AsyncWrap` first. Fix: #4250 PR-URL: #4268 Reviewed-By: James M Snell <jasnell@gmail.com>
`WrapperInfo` casts pointer in JS object's internal field to `AsyncWrap`. This approach fails miserably for `TLSWrap` because it was inhereted from the `StreamBase` first, creating different kind of `vtable` for the whole class. Reorder parent classes to put `AsyncWrap` first. Fix: nodejs#4250 PR-URL: nodejs#4268 Reviewed-By: James M Snell <jasnell@gmail.com>
WrapperInfo
casts pointer in JS object's internal field toAsyncWrap
. This approach fails miserably forTLSWrap
because it wasinhereted from the
StreamBase
first, creating different kind ofvtable
for the whole class.Reorder parent classes to put
AsyncWrap
first.Fix: #4250