Skip to content

Conversation

@jplevyak
Copy link
Contributor

Signed-off-by: John Plevyak [email protected]

Signed-off-by: John Plevyak <[email protected]>
@jplevyak jplevyak requested a review from PiotrSikora March 18, 2020 00:25
Signed-off-by: John Plevyak <[email protected]>
Copy link

@yxue yxue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if we can add some CI task for the repo.

wasm_vm_test.cc Outdated
TestNullVmPlugin *test_null_vm_plugin_ = nullptr;
RegisterNullVmPluginFactory register_test_null_vm_plugin("test_null_vm_plugin", []() {
auto plugin = std::make_unique<TestNullVmPlugin>();
test_null_vm_plugin_ = plugin.get();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to capture the variable in the lambda.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't because it is a global. Do you think we should for style reasons?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK. Nit: renaming the variable to make it look like a global variable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@yxue
Copy link

yxue commented Mar 25, 2020

I am wondering if we can add some CI task for the repo.

Sorry, I didn't notice the workflow/cpp.yaml file you added in the PR.

Signed-off-by: John Plevyak <[email protected]>
Signed-off-by: John Plevyak <[email protected]>
Signed-off-by: John Plevyak <[email protected]>
Signed-off-by: John Plevyak <[email protected]>
@jplevyak jplevyak requested a review from yxue March 26, 2020 19:03
Signed-off-by: John Plevyak <[email protected]>
@jplevyak jplevyak merged commit 3222ea6 into proxy-wasm:master Mar 26, 2020
StarryVae pushed a commit to StarryVae/proxy-wasm-cpp-host that referenced this pull request May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants