Skip to content

fix compile warning on copy ctor of Wasm #18

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.

Sign up for GitHub

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

Merged
jplevyak merged 4 commits into from
Mar 1, 2019
Merged

fix compile warning on copy ctor of Wasm #18

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d2179eb
fix compile warning on copy ctor of Wasm
lambdai Feb 12, 2019
87eb952
merge istio/wasm and resolve conflict
lambdai Feb 14, 2019
ac4acbe
address comment: use enable_s_f_t copy ctor
lambdai Feb 14, 2019
dd70c3e
Merge branch 'wasm' into build_warning
lambdai Feb 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/extensions/common/wasm/wasm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1018,7 +1018,7 @@ void Wasm::getFunctions() {
#undef _GET
}

Wasm::Wasm(const Wasm& wasm) {
Wasm::Wasm(const Wasm& wasm) : enable_shared_from_this<Wasm>() {

@PiotrSikora PiotrSikora Feb 13, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What compiler do you use and what warning are you getting?

The Wasm class already inherits this, and both constructors are public, so this shouldn't be needed, AFAIK.

@PiotrSikora PiotrSikora Feb 13, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah, sorry, I see that you've pasted the error in the opening message.

Does this change look reasonable to you, @jplevyak?

@jplevyak jplevyak Feb 13, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't think this is necessary. By default C++ will use the copy constructor from any public superclass. Perhaps this is related to the issue you fixed where enabl_shared_from_this was not public?

@lambdai lambdai Feb 13, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@jplevyak I agree with you that the behavior doesn't change with or without the commit.
It's compiler warning not an error. As this is the only low prior error at my side, I prefer to fix it.

[-Werror=extra]

@jplevyak jplevyak Feb 14, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What I am trying to figure out is how you are seeing this. Did you add -Werror=extra in your build? I tried to rebuild that file and I am not seeing it. Could you attach repro instructions.

wasm_vm_ = wasm.wasmVm()->clone();
general_context_ = createContext();
getFunctions();
Expand Down