Skip to content

Add missing includes / don't include standard headers in namespaces #364

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

Closed
mkauf opened this issue Aug 24, 2023 · 2 comments · Fixed by #365
Closed

Add missing includes / don't include standard headers in namespaces #364

mkauf opened this issue Aug 24, 2023 · 2 comments · Fixed by #365

Comments

@mkauf
Copy link

mkauf commented Aug 24, 2023

There are some missing includes. On some systems, these includes may not be necessary (because they are contained in other system headers).

I have found these while compiling Envoy proxy on Fedora 38.

vm_id_handle.h:

  • #include <string>
  • #include <string_view>

wasm_vm.h:

  • #include <string_view>
@mkauf
Copy link
Author

mkauf commented Aug 24, 2023

I also got other strange compile errors when compiling Envoy proxy:

In file included from external/proxy_wasm_cpp_host/src/null/null.cc:17:
In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/null_vm.h:22:
In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/null_vm_plugin.h:18:
In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/wasm_vm.h:27:
In file included from external/proxy_wasm_cpp_host/include/proxy-wasm/word.h:22:
external/proxy_wasm_cpp_sdk/proxy_wasm_common.h:59:8: error: no type named 'string' in namespace 'proxy_wasm::std'; did you mean '::std::string'?
inline std::string toString(WasmResult r) {
       ^~~~~~~~~~~
       ::std::string
/usr/bin/../lib/gcc/x86_64-redhat-linux/13/../../../../include/c++/13/bits/stringfwd.h:77:33: note: '::std::string' declared here
  typedef basic_string<char>    string;   

I think that happens because the header proxy_wasm_common.h contains these lines:

#include <cstdint>
#include <string>

And in files like wasm.h, the file proxy_wasm_common.h is included inside a C++ namespace:

namespace proxy_wasm {

#include "proxy_wasm_common.h"

...
}

-> so the header <string> is included inside the namespace proxy_wasm. That's bad practice and probably that's also the cause of the build problem.

@mkauf mkauf changed the title Add missing includes Add missing includes / don't include standard headers in namespaces Aug 24, 2023
@sitano
Copy link
Contributor

sitano commented Aug 29, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants