Add the ability to have multiple wasm intrinsic modules (e.g. for Emscripten support). STACKED on Emscripten version PR. #32
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -504,6 +504,21 @@ void replaceHeader(Http::HeaderMap* map, absl::string_view key, absl::string_vie | |||
| map->addCopy(lower_key, std::string(value)); | |||
| } | |||
|
|
|||
| const uint8_t* decodeVarint(const uint8_t* start, const uint8_t* end, uint32_t* out) { | |||
There was a problem hiding this comment.
Could you make this function at this moment?
There was a problem hiding this comment.
Could you clarify? Do you think the function should be moved?
There was a problem hiding this comment.
Sorry... The word "static" is eaten by mistake -_-b
I think we could declare the function static so that no other component should use decodeVarint
If there is further usage, it indicates we should extract this function to a util class.
There was a problem hiding this comment.
it is in an anonymous namespace.
| start++; | ||
| } | ||
| if (start < end) { | ||
| ret |= (*start & 0x7f); |
There was a problem hiding this comment.
IMO not necessary. Also I don't see a great number of such asserts in the envoy codebase so I don't believe it is the style. The asserts I see seem to be concentrated in particularly complex code (e.g. the connection manager).
There was a problem hiding this comment.
Sure. Ignore my comment. Please follow the style.
| wavm->envoy_functions_.emplace_back( | ||
| new Intrinsics::Function(wavm->envoy_module_, functionName.data(), reinterpret_cast<void*>(f), | ||
| inferEnvoyFunctionType(f), IR::CallingConvention::intrinsic)); | ||
| wavm->envoyFunctions_.emplace_back(new Intrinsics::Function( |
There was a problem hiding this comment.
| wavm->envoyFunctions_.emplace_back(new Intrinsics::Function( | |
| wavm->envoyFunctions_.emplace_back(std::make_unique<Intrinsics::Function>( |
There was a problem hiding this comment.
In this case I don't see how that is an improvement. It is clear that the pointer is not lost, and in the second case (your suggestion) it provides the additional bit of information that the container is of unique_ptr but that bit of information is not salient useful at this point.
There was a problem hiding this comment.
Agree that at this moment the pointer is not lost. It reduces the risk when we make further changes.
Meanwhile my suggestion is self explaining: no extra thinking for any one doesn't have the full knowledge of envoyFunctions_
| WAVM::Runtime::ModuleInstance* moduleInstance() { return moduleInstance_; } | ||
| WAVM::Runtime::ModuleInstance* envoyModuleInstance() { return moduleInstance_; } | ||
| void makeModule(absl::string_view name) override; | ||
| absl::string_view getUserSection(absl::string_view name, bool* present) override; | ||
|
|
||
| void GetFunctions(); | ||
| void RegisterCallbacks(); | ||
|
|
||
| bool hasInstantiatedModule_ = false; |
There was a problem hiding this comment.
IMHO all the belows should be private...
There was a problem hiding this comment.
This class is entirely within this file so it is well encapsulated already and the purpose of 'private' is to provide proper encapsulation which has already been achieved. I'll change it to a struct.
There was a problem hiding this comment.
Thanks for the explanation. I should add nit prefix for this comment
jplevyak
force-pushed
the
wasm-module
branch
from
98480eb to
5db7376
Compare
jplevyak
changed the title
Signed-off-by: Yan Avlasov <[email protected]>
envoyproxy#32902) Revert "Begin process of removing singleton use by StringMatcher (envoyproxy#32829)" This reverts commit b7818b0. Signed-off-by: Ryan Northey <[email protected]>
…32908) * Revert "Revert "Begin process of removing singleton use by StringMatcher (#32… (envoyproxy#32902)" This reverts commit 9ac0dd8. Signed-off-by: Greg Greenway <[email protected]> * try increasing shard count to attempt fix of test timeout Signed-off-by: Greg Greenway <[email protected]> --------- Signed-off-by: Greg Greenway <[email protected]>
Add the ability to have multiple wasm intrinsic modules (e.g. for Emscripten support).