[dawn][node] Fix warnings in dawn/node, refactor warning setup
dawn/node was compiled without a lot of warnings. This CL adds the
warning flags that the rest of Dawn uses, and fixes the resulting
warnings. (In CMake, these warning flags were only applied by
dawn_internal_config, so they're split out into dawn_warnings_config so
they can be used separately. There is some redundancy with
common_compile_options, but that's applied to a lot more targets.)
Set the Node API include directories as "system" include paths so
warnings in them are ignored. This is natively supported by CMake but
requires a small hack in GN.
Bug: none
Change-Id: I00cf6c6715a328bece8710089269cc7be434dde9
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/244658
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
diff --git a/CMakeLists.txt b/CMakeLists.txt
index e532a25..baca383 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -387,7 +387,8 @@
find_package(Python3 REQUIRED)
message(STATUS "Dawn: using python at ${Python3_EXECUTABLE}")
-# Compile definitions for the internal config
+# Options and compile definitions for the internal config
+add_dependencies(dawn_internal_config dawn_warnings_config)
if (DAWN_ALWAYS_ASSERT OR IS_DEBUG_BUILD)
target_compile_definitions(dawn_internal_config INTERFACE "DAWN_ENABLE_ASSERTS")
endif()
diff --git a/src/cmake/DawnCompilerWarningFlags.cmake b/src/cmake/DawnCompilerWarningFlags.cmake
index df32102..f3dccae 100644
--- a/src/cmake/DawnCompilerWarningFlags.cmake
+++ b/src/cmake/DawnCompilerWarningFlags.cmake
@@ -25,8 +25,25 @@
# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-# This module requires CMake 3.19 features (the `CheckCompilerFlag`
-# module). Just skip it for older CMake versions and let the warnings appear.
+# Common compiler flag config. This can be depended on directly, or via
+# dawn_internal_config.
+add_library(dawn_warnings_config INTERFACE)
+
+# Flags we can add without bothering with CheckCompilerFlag.
+
+if (DAWN_WERROR)
+ if (COMPILER_IS_LIKE_GNU)
+ target_compile_options(dawn_warnings_config
+ INTERFACE
+ "-Werror"
+ )
+ endif ()
+endif ()
+
+# Flags that might not be supported in every version of a compiler.
+
+# This requires CMake 3.19 features (for the `CheckCompilerFlag` module).
+# Just skip it for older CMake versions and let the warnings appear.
if (CMAKE_VERSION VERSION_LESS "3.19")
return ()
endif ()
@@ -37,9 +54,10 @@
foreach (lang IN LISTS ARGN)
check_compiler_flag("${lang}" "${flag}" "dawn_have_compiler_flag-${lang}-${flag}")
if (dawn_have_compiler_flag-${lang}-${flag})
- target_compile_options(dawn_internal_config
+ target_compile_options(dawn_warnings_config
INTERFACE
- "$<BUILD_INTERFACE:$<$<COMPILE_LANGUAGE:${lang}>:${flag}>>")
+ "$<BUILD_INTERFACE:$<$<COMPILE_LANGUAGE:${lang}>:${flag}>>"
+ )
endif ()
endforeach ()
endfunction ()
diff --git a/src/dawn/node/BUILD.gn b/src/dawn/node/BUILD.gn
index 07b71b4..cca1fd9 100644
--- a/src/dawn/node/BUILD.gn
+++ b/src/dawn/node/BUILD.gn
@@ -56,25 +56,38 @@
"$dawn_root/third_party/gn/dxc/build/run_built_binary.py"
config("common_config") {
- warning_flags = []
- if (is_clang) {
- warning_flags += [
- "-Wno-error", # TODO(amaiorano): For now, allow warnings
- "-Wno-extra-semi",
- "-Wno-shadow",
- "-Wno-unused-variable",
- "-Wno-implicit-fallthrough",
- "-Wno-unused-but-set-variable",
+ # Add include paths for Node API headers, and handle the fact that they produce many warnings.
+ if (is_win) {
+ # -isystem is not supported (MSVC or clang-cl.exe).
+ include_dirs = [
+ "${dawn_node_addon_api_dir}",
+ "${dawn_node_api_headers_dir}/include",
]
+ if (is_clang) {
+ # Clang (clang-cl.exe). Disable the warnings produced by Node headers.
+ cflags = [
+ "-Wno-extra-semi",
+ "-Wno-extra-semi-stmt",
+ "-Wno-inconsistent-missing-destructor-override",
+ "-Wno-shadow",
+ "-Wno-shadow-field",
+ "-Wno-suggest-destructor-override",
+ ]
+ } else {
+ # MSVC. Just disable most warnings.
+ cflags = [ "/W0" ]
+ }
} else {
- # MSVC
- warning_flags += [
- "/W0", # Suppress most warnings
+ # -isystem should be supported (Clang or GCC).
+ # GN does not have a feature to support -isystem (like CMake's
+ # target_include_directories SYSTEM option), so we set it directly via cflags.
+ # Note this may have slightly weird behavior (e.g. https://crbug.com/41400937).
+ cflags = [
+ "-isystem" + rebase_path(dawn_node_addon_api_dir, root_build_dir),
+ "-isystem" +
+ rebase_path("${dawn_node_api_headers_dir}/include", root_build_dir),
]
}
-
- cflags_c = warning_flags
- cflags_cc = warning_flags
}
template("idlgen") {
@@ -140,12 +153,9 @@
"$interop_gen_dir/WebGPU.h",
"interop/Core.cpp",
"interop/Core.h",
+ "interop/NodeAPI.h",
]
- include_dirs = [
- "${dawn_node_addon_api_dir}",
- "${dawn_node_api_headers_dir}/include",
- "${dawn_node_gen_dir}",
- ]
+ include_dirs = [ "${dawn_node_gen_dir}" ]
deps = [
":idlgen_webgpu_cpp",
":idlgen_webgpu_h",
@@ -224,11 +234,7 @@
"binding/TogglesLoader.cpp",
"binding/TogglesLoader.h",
]
- include_dirs = [
- "${dawn_node_addon_api_dir}",
- "${dawn_node_api_headers_dir}/include",
- "${dawn_node_gen_dir}",
- ]
+ include_dirs = [ "${dawn_node_gen_dir}" ]
deps = [
":dawn_node_interop",
"$dawn_root/src/dawn/native:native",
@@ -279,6 +285,7 @@
output_name = "dawn"
output_extension = "node"
output_prefix_override = true # dawn.node, not libdawn.node
+ public_configs = [ ":common_config" ]
sources = [
"ManualConstructors.cpp",
"ManualConstructors.h",
@@ -307,9 +314,5 @@
}
sources += [ "NapiSymbols.cpp" ]
}
- include_dirs = [
- "${dawn_node_addon_api_dir}",
- "${dawn_node_api_headers_dir}/include",
- "${dawn_node_gen_dir}",
- ]
+ include_dirs = [ "${dawn_node_gen_dir}" ]
}
diff --git a/src/dawn/node/CMakeLists.txt b/src/dawn/node/CMakeLists.txt
index c0367f7..4ebff35 100644
--- a/src/dawn/node/CMakeLists.txt
+++ b/src/dawn/node/CMakeLists.txt
@@ -104,13 +104,25 @@
LIBRARY_OUTPUT_DIRECTORY "${PROJECT_BINARY_DIR}"
CXX_STANDARD 20
)
-target_link_libraries(dawn_node dawn_node_binding dawn_node_interop dawn_native dawncpp dawn_proc
- libtint)
-target_include_directories(dawn_node PRIVATE
- "${PROJECT_SOURCE_DIR}"
- "${NODE_ADDON_API_DIR}"
- "${NODE_API_HEADERS_DIR}/include"
- "${DAWN_NODE_GEN_DIR}"
+target_link_libraries(dawn_node
+ PRIVATE
+ dawncpp
+ dawn_native
+ dawn_node_binding
+ dawn_node_interop
+ dawn_proc
+ dawn_warnings_config
+ libtint
+)
+target_include_directories(dawn_node
+ PRIVATE
+ "${PROJECT_SOURCE_DIR}"
+ "${DAWN_NODE_GEN_DIR}"
+)
+target_include_directories(dawn_node SYSTEM
+ PRIVATE
+ "${NODE_ADDON_API_DIR}"
+ "${NODE_API_HEADERS_DIR}/include"
)
# To reduce the build dependencies for compiling the dawn.node targets, we do
@@ -158,7 +170,7 @@
)
add_custom_target(napi-symbols DEPENDS "${NAPI_SYMBOLS_LIB}")
add_dependencies(dawn_node napi-symbols)
- target_link_libraries(dawn_node "${NAPI_SYMBOLS_LIB}")
+ target_link_libraries(dawn_node PRIVATE "${NAPI_SYMBOLS_LIB}")
else()
# Generate the NapiSymbols.h file from the Napi symbol list
set(NAPI_SYMBOLS_H "${DAWN_NODE_GEN_DIR}/NapiSymbols.h")
diff --git a/src/dawn/node/binding/CMakeLists.txt b/src/dawn/node/binding/CMakeLists.txt
index 24860cf..00bd04c 100644
--- a/src/dawn/node/binding/CMakeLists.txt
+++ b/src/dawn/node/binding/CMakeLists.txt
@@ -95,13 +95,17 @@
target_include_directories(dawn_node_binding
PRIVATE
"${PROJECT_SOURCE_DIR}"
+ "${DAWN_NODE_GEN_DIR}"
+)
+target_include_directories(dawn_node_binding SYSTEM
+ PRIVATE
"${NODE_ADDON_API_DIR}"
"${NODE_API_HEADERS_DIR}/include"
- "${DAWN_NODE_GEN_DIR}"
)
target_link_libraries(dawn_node_binding
PRIVATE
dawncpp
dawn_node_interop
+ dawn_warnings_config
)
diff --git a/src/dawn/node/binding/Converter.cpp b/src/dawn/node/binding/Converter.cpp
index 02e2990..b09a644 100644
--- a/src/dawn/node/binding/Converter.cpp
+++ b/src/dawn/node/binding/Converter.cpp
@@ -67,9 +67,11 @@
default:
case 3:
out.depthOrArrayLayers = (*vec)[2];
- case 2: // fallthrough
+ [[fallthrough]];
+ case 2:
out.height = (*vec)[1];
- case 1: // fallthrough
+ [[fallthrough]];
+ case 1:
out.width = (*vec)[0];
return true;
case 0:
@@ -101,11 +103,14 @@
default:
case 4:
out.a = (*vec)[3];
- case 3: // fallthrough
+ [[fallthrough]];
+ case 3:
out.b = (*vec)[2];
- case 2: // fallthrough
+ [[fallthrough]];
+ case 2:
out.g = (*vec)[1];
- case 1: // fallthrough
+ [[fallthrough]];
+ case 1:
out.r = (*vec)[0];
return true;
case 0:
@@ -121,10 +126,13 @@
default:
case 3:
out.z = in[2];
- case 2: // fallthrough
+ [[fallthrough]];
+ case 2:
out.y = in[1];
- case 1: // fallthrough
+ [[fallthrough]];
+ case 1:
out.x = in[0];
+ [[fallthrough]];
case 0:
break;
}
@@ -1355,7 +1363,8 @@
out.buffer = *buffer;
return true;
}
- if (auto* res = std::get_if<interop::Interface<interop::GPUExternalTexture>>(&in.resource)) {
+ if ([[maybe_unused]] auto* res =
+ std::get_if<interop::Interface<interop::GPUExternalTexture>>(&in.resource)) {
// TODO(crbug.com/dawn/1129): External textures
UNIMPLEMENTED(env, {});
}
diff --git a/src/dawn/node/binding/EventTarget.h b/src/dawn/node/binding/EventTarget.h
index 6d3ad89..c6bba70 100644
--- a/src/dawn/node/binding/EventTarget.h
+++ b/src/dawn/node/binding/EventTarget.h
@@ -30,7 +30,7 @@
class EventTarget : public virtual interop::EventTarget {
public:
explicit EventTarget(Napi::Env env);
- ~EventTarget();
+ ~EventTarget() override;
void addEventListener(
Napi::Env,
diff --git a/src/dawn/node/binding/GPU.cpp b/src/dawn/node/binding/GPU.cpp
index 86f38a1..fcc1861 100644
--- a/src/dawn/node/binding/GPU.cpp
+++ b/src/dawn/node/binding/GPU.cpp
@@ -294,16 +294,16 @@
struct Features : public interop::WGSLLanguageFeatures {
explicit Features(InteropWGSLFeatureSet features) : features_(features) {}
- ~Features() = default;
+ ~Features() override = default;
- bool has(Napi::Env env, std::string name) {
+ bool has(Napi::Env env, std::string name) override {
interop::WGSLLanguageFeatureName feature;
if (!interop::Converter<interop::WGSLLanguageFeatureName>::FromString(name, feature)) {
return false;
}
return features_.count(feature) != 0u;
}
- std::vector<std::string> keys(Napi::Env env) {
+ std::vector<std::string> keys(Napi::Env env) override {
std::vector<std::string> out;
out.reserve(features_.size());
for (auto feature : features_) {
@@ -312,9 +312,9 @@
}
return out;
}
- size_t getSize(Napi::Env env) { return features_.size(); }
- Napi::Value iterator(const Napi::CallbackInfo& info) {
- return CreateIterator(info, this->features_);
+ size_t getSize(Napi::Env env) override { return features_.size(); }
+ Napi::Value iterator(const Napi::CallbackInfo& info) override {
+ return CreateIterator<InteropWGSLFeatureSet>(info, this->features_);
}
InteropWGSLFeatureSet features_;
diff --git a/src/dawn/node/binding/GPUBuffer.cpp b/src/dawn/node/binding/GPUBuffer.cpp
index 5bffb0b..4426e3f 100644
--- a/src/dawn/node/binding/GPUBuffer.cpp
+++ b/src/dawn/node/binding/GPUBuffer.cpp
@@ -88,9 +88,9 @@
ctx->promise.Resolve();
mapped_ = true;
break;
- case wgpu::MapAsyncStatus::CallbackCancelled: // fallthrough
- assert(false);
+ case wgpu::MapAsyncStatus::CallbackCancelled:
case wgpu::MapAsyncStatus::Aborted:
+ assert(status != wgpu::MapAsyncStatus::CallbackCancelled);
async_->Reject(ctx->env, ctx->promise, Errors::AbortError(ctx->env));
break;
case wgpu::MapAsyncStatus::Error:
diff --git a/src/dawn/node/binding/GPUDevice.h b/src/dawn/node/binding/GPUDevice.h
index 80a7d69..0e26554 100644
--- a/src/dawn/node/binding/GPUDevice.h
+++ b/src/dawn/node/binding/GPUDevice.h
@@ -60,7 +60,7 @@
wgpu::Device device,
interop::Promise<interop::Interface<interop::GPUDeviceLostInfo>> lost_promise,
std::shared_ptr<AsyncRunner> async);
- ~GPUDevice();
+ ~GPUDevice() override;
void ForceLoss(wgpu::DeviceLostReason reason, const char* message);
diff --git a/src/dawn/node/binding/GPUShaderModule.cpp b/src/dawn/node/binding/GPUShaderModule.cpp
index 73f0831..315c5cc 100644
--- a/src/dawn/node/binding/GPUShaderModule.cpp
+++ b/src/dawn/node/binding/GPUShaderModule.cpp
@@ -56,7 +56,7 @@
explicit GPUCompilationMessage(const wgpu::CompilationMessage& m)
: lineNum(m.lineNum),
message(m.message) {
- bool foundUtf16 = false;
+ [[maybe_unused]] bool foundUtf16 = false;
for (const auto* chain = m.nextInChain; chain != nullptr; chain = chain->nextInChain) {
if (chain->sType == wgpu::SType::DawnCompilationMessageUtf16) {
assert(!foundUtf16);
diff --git a/src/dawn/node/binding/IteratorHelper.h b/src/dawn/node/binding/IteratorHelper.h
index 94c7a70..b0b7d0a 100644
--- a/src/dawn/node/binding/IteratorHelper.h
+++ b/src/dawn/node/binding/IteratorHelper.h
@@ -58,7 +58,7 @@
Napi::Env env = info.Env();
Napi::Object iterObj = Napi::Object::New(env);
- auto* helper = new IteratorHelper{collection, info.This().As<Napi::Object>()};
+ auto* helper = new IteratorHelper<T>{collection, info.This().As<Napi::Object>()};
iterObj.AddFinalizer(std::remove_reference_t<decltype(*helper)>::Cleanup, helper);
iterObj.Set("next", Napi::Function::New(
diff --git a/src/dawn/node/interop/CMakeLists.txt b/src/dawn/node/interop/CMakeLists.txt
index 8a77145..01865c7 100644
--- a/src/dawn/node/interop/CMakeLists.txt
+++ b/src/dawn/node/interop/CMakeLists.txt
@@ -67,12 +67,16 @@
target_include_directories(dawn_node_interop
PRIVATE
"${PROJECT_SOURCE_DIR}"
+ "${DAWN_NODE_GEN_DIR}"
+)
+target_include_directories(dawn_node_interop SYSTEM
+ PRIVATE
"${NODE_ADDON_API_DIR}"
"${NODE_API_HEADERS_DIR}/include"
- "${DAWN_NODE_GEN_DIR}"
)
target_link_libraries(dawn_node_interop
PRIVATE
dawncpp
+ dawn_warnings_config
)
diff --git a/src/dawn/node/interop/Core.h b/src/dawn/node/interop/Core.h
index cf3a8ed..f9dd526 100644
--- a/src/dawn/node/interop/Core.h
+++ b/src/dawn/node/interop/Core.h
@@ -670,16 +670,16 @@
std::unordered_map<K, V> map(keys.Length());
for (uint32_t i = 0; i < static_cast<uint32_t>(keys.Length()); i++) {
K key{};
- V value{};
+ V val{};
auto key_res = Converter<K>::FromJS(env, keys[i], key);
if (!key_res) {
return key_res.Append("for object key");
}
- auto value_res = Converter<V>::FromJS(env, obj.Get(keys[i]), value);
+ auto value_res = Converter<V>::FromJS(env, obj.Get(keys[i]), val);
if (!value_res) {
return value_res.Append("for object value of key: ", key);
}
- map[key] = value;
+ map[key] = val;
}
out = std::move(map);
return Success;
diff --git a/src/dawn/node/interop/NodeAPI.h b/src/dawn/node/interop/NodeAPI.h
index dfc618b..4bd3ae1 100644
--- a/src/dawn/node/interop/NodeAPI.h
+++ b/src/dawn/node/interop/NodeAPI.h
@@ -31,6 +31,6 @@
// Dawn is built with exceptions disabled.
#define NAPI_DISABLE_CPP_EXCEPTIONS
-#include "napi.h" // NOLINT(build/include_directory) - external include
+#include <napi.h>
#endif // SRC_DAWN_NODE_INTEROP_NODEAPI_H_
diff --git a/src/dawn/node/interop/WebGPU.cpp.tmpl b/src/dawn/node/interop/WebGPU.cpp.tmpl
index e11217d..d65fab2 100644
--- a/src/dawn/node/interop/WebGPU.cpp.tmpl
+++ b/src/dawn/node/interop/WebGPU.cpp.tmpl
@@ -354,7 +354,7 @@
Result Converter<{{$.Name}}>::FromJS(Napi::Env env, Napi::Value value, {{$.Name}}& out) {
auto object = value.ToObject();
Result res;
-{{- template "DictionaryMembersFromJS" $}};
+{{- template "DictionaryMembersFromJS" $}}
return Success;
}
@@ -394,6 +394,8 @@
if (!res) {
return res.Append("while converting member '{{$m.Name}}'");
}
+{{- else}}
+ (void)object;
{{- end}}
{{- end}}
diff --git a/src/dawn/node/interop/WebGPU.h.tmpl b/src/dawn/node/interop/WebGPU.h.tmpl
index cf420f8..e6dfbb5 100644
--- a/src/dawn/node/interop/WebGPU.h.tmpl
+++ b/src/dawn/node/interop/WebGPU.h.tmpl
@@ -148,7 +148,7 @@
return Bind(env, std::make_unique<T>(std::forward<ARGS>(args)...));
}
- virtual ~{{$.Name}}();
+ virtual ~{{$.Name}}() {{- if $.Inherits }} override{{end}};
{{$.Name}}();
{{- if $s := SetlikeOf $}}
{{- template "InterfaceSetlike" $s}}
diff --git a/src/dawn/node/utils/Debug.h b/src/dawn/node/utils/Debug.h
index 13ed0c3..fb45b1c 100644
--- a/src/dawn/node/utils/Debug.h
+++ b/src/dawn/node/utils/Debug.h
@@ -85,14 +85,14 @@
std::ostream& Write(std::ostream& out, const std::unordered_map<K, V>& value) {
out << "{";
bool first = true;
- for (auto& [key, value] : value) {
+ for (auto& [key, val] : value) {
if (!first) {
out << ", ";
}
first = false;
Write(out, key);
out << ": ";
- Write(out, value);
+ Write(out, val);
}
return out << "}";
}
@@ -138,8 +138,8 @@
// string representation of all the variadic arguments.
#define LOG(...) \
::wgpu::utils::Write(std::cout << __FILE__ << ":" << __LINE__ << " " << __FUNCTION__ << ": ", \
- ##__VA_ARGS__) \
- << "\n";
+ __VA_ARGS__) \
+ << "\n"
// UNIMPLEMENTED(env) raises a JS exception. Used to stub code that has not yet been implemented.
#define UNIMPLEMENTED(env, ...) \
