Use result type to handle diagnostics
Updates the binding validation methods to return a `Result` and pass the
diagnostics through the result instead of as a parameter.
As requested in dawn-review.googlesource.com/c/dawn/+/156761
Change-Id: I769dc4d269a478c7b875f72677942605f999d7bb
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/160741
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Reviewed-by: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/tint/lang/msl/writer/common/BUILD.bazel b/src/tint/lang/msl/writer/common/BUILD.bazel
index c8f2c0d..51256e7 100644
--- a/src/tint/lang/msl/writer/common/BUILD.bazel
+++ b/src/tint/lang/msl/writer/common/BUILD.bazel
@@ -61,6 +61,7 @@
"//src/tint/utils/math",
"//src/tint/utils/memory",
"//src/tint/utils/reflection",
+ "//src/tint/utils/result",
"//src/tint/utils/rtti",
"//src/tint/utils/strconv",
"//src/tint/utils/symbol",
diff --git a/src/tint/lang/msl/writer/common/BUILD.cmake b/src/tint/lang/msl/writer/common/BUILD.cmake
index 177f390..728427a 100644
--- a/src/tint/lang/msl/writer/common/BUILD.cmake
+++ b/src/tint/lang/msl/writer/common/BUILD.cmake
@@ -62,6 +62,7 @@
tint_utils_math
tint_utils_memory
tint_utils_reflection
+ tint_utils_result
tint_utils_rtti
tint_utils_strconv
tint_utils_symbol
diff --git a/src/tint/lang/msl/writer/common/BUILD.gn b/src/tint/lang/msl/writer/common/BUILD.gn
index 0ea4cc8..befabfe 100644
--- a/src/tint/lang/msl/writer/common/BUILD.gn
+++ b/src/tint/lang/msl/writer/common/BUILD.gn
@@ -64,6 +64,7 @@
"${tint_src_dir}/utils/math",
"${tint_src_dir}/utils/memory",
"${tint_src_dir}/utils/reflection",
+ "${tint_src_dir}/utils/result",
"${tint_src_dir}/utils/rtti",
"${tint_src_dir}/utils/strconv",
"${tint_src_dir}/utils/symbol",
diff --git a/src/tint/lang/msl/writer/common/option_helpers.cc b/src/tint/lang/msl/writer/common/option_helpers.cc
index cd44e5e..cda70c6 100644
--- a/src/tint/lang/msl/writer/common/option_helpers.cc
+++ b/src/tint/lang/msl/writer/common/option_helpers.cc
@@ -27,6 +27,8 @@
#include "src/tint/lang/msl/writer/common/option_helpers.h"
+#include <utility>
+
#include "src/tint/utils/containers/hashset.h"
namespace tint::msl::writer {
@@ -34,7 +36,9 @@
/// binding::BindingInfo to tint::BindingPoint map
using InfoToPointMap = tint::Hashmap<binding::BindingInfo, tint::BindingPoint, 8>;
-bool ValidateBindingOptions(const Options& options, diag::List& diagnostics) {
+Result<SuccessType> ValidateBindingOptions(const Options& options) {
+ diag::List diagnostics;
+
tint::Hashmap<tint::BindingPoint, binding::BindingInfo, 8> seen_wgsl_bindings{};
InfoToPointMap seen_msl_buffer_bindings{};
@@ -94,27 +98,27 @@
// Storage and uniform are both [[buffer()]]
if (!valid(seen_msl_buffer_bindings, options.bindings.uniform)) {
diagnostics.add_note(diag::System::Writer, "when processing uniform", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
if (!valid(seen_msl_buffer_bindings, options.bindings.storage)) {
diagnostics.add_note(diag::System::Writer, "when processing storage", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
// Sampler is [[sampler()]]
if (!valid(seen_msl_sampler_bindings, options.bindings.sampler)) {
diagnostics.add_note(diag::System::Writer, "when processing sampler", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
// Texture and storage texture are [[texture()]]
if (!valid(seen_msl_texture_bindings, options.bindings.texture)) {
diagnostics.add_note(diag::System::Writer, "when processing texture", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
if (!valid(seen_msl_texture_bindings, options.bindings.storage_texture)) {
diagnostics.add_note(diag::System::Writer, "when processing storage_texture", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
for (const auto& it : options.bindings.external_texture) {
@@ -126,26 +130,26 @@
// Validate with the actual source regardless of what the remapper will do
if (wgsl_seen(src_binding, plane0)) {
diagnostics.add_note(diag::System::Writer, "when processing external_texture", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
// Plane0 & Plane1 are [[texture()]]
if (msl_seen(seen_msl_texture_bindings, plane0, src_binding)) {
diagnostics.add_note(diag::System::Writer, "when processing external_texture", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
if (msl_seen(seen_msl_texture_bindings, plane1, src_binding)) {
diagnostics.add_note(diag::System::Writer, "when processing external_texture", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
// Metadata is [[buffer()]]
if (msl_seen(seen_msl_buffer_bindings, metadata, src_binding)) {
diagnostics.add_note(diag::System::Writer, "when processing external_texture", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
}
- return true;
+ return Success;
}
// The remapped binding data and external texture data need to coordinate in order to put things in
diff --git a/src/tint/lang/msl/writer/common/option_helpers.h b/src/tint/lang/msl/writer/common/option_helpers.h
index 1abf984..6f21fbd 100644
--- a/src/tint/lang/msl/writer/common/option_helpers.h
+++ b/src/tint/lang/msl/writer/common/option_helpers.h
@@ -34,6 +34,7 @@
#include "src/tint/api/options/external_texture.h"
#include "src/tint/lang/msl/writer/common/options.h"
#include "src/tint/utils/diagnostic/diagnostic.h"
+#include "src/tint/utils/result/result.h"
namespace tint::msl::writer {
@@ -41,9 +42,8 @@
using RemapperData = std::unordered_map<BindingPoint, BindingPoint>;
/// @param options the options
-/// @param diagnostics the diagnostics
-/// @returns true if the binding points are valid
-bool ValidateBindingOptions(const Options& options, diag::List& diagnostics);
+/// @returns success or failure
+Result<SuccessType> ValidateBindingOptions(const Options& options);
/// Populates data from the writer options for the remapper and external texture.
/// @param options the writer options
diff --git a/src/tint/lang/msl/writer/writer.cc b/src/tint/lang/msl/writer/writer.cc
index cb5281d..fc86c0c 100644
--- a/src/tint/lang/msl/writer/writer.cc
+++ b/src/tint/lang/msl/writer/writer.cc
@@ -48,9 +48,9 @@
}
{
- diag::List validation_diagnostics;
- if (!ValidateBindingOptions(options, validation_diagnostics)) {
- return Failure{validation_diagnostics};
+ auto res = ValidateBindingOptions(options);
+ if (!res) {
+ return res.Failure();
}
}
diff --git a/src/tint/lang/spirv/writer/common/BUILD.bazel b/src/tint/lang/spirv/writer/common/BUILD.bazel
index 7c07fad..87e23c6 100644
--- a/src/tint/lang/spirv/writer/common/BUILD.bazel
+++ b/src/tint/lang/spirv/writer/common/BUILD.bazel
@@ -65,6 +65,7 @@
"//src/tint/utils/math",
"//src/tint/utils/memory",
"//src/tint/utils/reflection",
+ "//src/tint/utils/result",
"//src/tint/utils/rtti",
"//src/tint/utils/text",
"//src/tint/utils/traits",
diff --git a/src/tint/lang/spirv/writer/common/BUILD.cmake b/src/tint/lang/spirv/writer/common/BUILD.cmake
index 969c5e7..4d4db56 100644
--- a/src/tint/lang/spirv/writer/common/BUILD.cmake
+++ b/src/tint/lang/spirv/writer/common/BUILD.cmake
@@ -66,6 +66,7 @@
tint_utils_math
tint_utils_memory
tint_utils_reflection
+ tint_utils_result
tint_utils_rtti
tint_utils_text
tint_utils_traits
diff --git a/src/tint/lang/spirv/writer/common/BUILD.gn b/src/tint/lang/spirv/writer/common/BUILD.gn
index 0c9830d..c0aa3c5 100644
--- a/src/tint/lang/spirv/writer/common/BUILD.gn
+++ b/src/tint/lang/spirv/writer/common/BUILD.gn
@@ -68,6 +68,7 @@
"${tint_src_dir}/utils/math",
"${tint_src_dir}/utils/memory",
"${tint_src_dir}/utils/reflection",
+ "${tint_src_dir}/utils/result",
"${tint_src_dir}/utils/rtti",
"${tint_src_dir}/utils/text",
"${tint_src_dir}/utils/traits",
diff --git a/src/tint/lang/spirv/writer/common/option_helper.cc b/src/tint/lang/spirv/writer/common/option_helper.cc
index dc3670b..aec904a 100644
--- a/src/tint/lang/spirv/writer/common/option_helper.cc
+++ b/src/tint/lang/spirv/writer/common/option_helper.cc
@@ -27,11 +27,15 @@
#include "src/tint/lang/spirv/writer/common/option_helpers.h"
+#include <utility>
+
#include "src/tint/utils/containers/hashset.h"
namespace tint::spirv::writer {
-bool ValidateBindingOptions(const Options& options, diag::List& diagnostics) {
+Result<SuccessType> ValidateBindingOptions(const Options& options) {
+ diag::List diagnostics;
+
tint::Hashmap<tint::BindingPoint, binding::BindingInfo, 8> seen_wgsl_bindings{};
tint::Hashmap<binding::BindingInfo, tint::BindingPoint, 8> seen_spirv_bindings{};
@@ -88,23 +92,23 @@
if (!valid(options.bindings.uniform)) {
diagnostics.add_note(diag::System::Writer, "when processing uniform", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
if (!valid(options.bindings.storage)) {
diagnostics.add_note(diag::System::Writer, "when processing storage", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
if (!valid(options.bindings.texture)) {
diagnostics.add_note(diag::System::Writer, "when processing texture", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
if (!valid(options.bindings.storage_texture)) {
diagnostics.add_note(diag::System::Writer, "when processing storage_texture", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
if (!valid(options.bindings.sampler)) {
diagnostics.add_note(diag::System::Writer, "when processing sampler", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
for (const auto& it : options.bindings.external_texture) {
@@ -116,24 +120,24 @@
// Validate with the actual source regardless of what the remapper will do
if (wgsl_seen(src_binding, plane0)) {
diagnostics.add_note(diag::System::Writer, "when processing external_texture", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
if (spirv_seen(plane0, src_binding)) {
diagnostics.add_note(diag::System::Writer, "when processing external_texture", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
if (spirv_seen(plane1, src_binding)) {
diagnostics.add_note(diag::System::Writer, "when processing external_texture", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
if (spirv_seen(metadata, src_binding)) {
diagnostics.add_note(diag::System::Writer, "when processing external_texture", {});
- return false;
+ return Failure{std::move(diagnostics)};
}
}
- return true;
+ return Success;
}
// The remapped binding data and external texture data need to coordinate in order to put things in
diff --git a/src/tint/lang/spirv/writer/common/option_helpers.h b/src/tint/lang/spirv/writer/common/option_helpers.h
index 0be6d35..3f51355 100644
--- a/src/tint/lang/spirv/writer/common/option_helpers.h
+++ b/src/tint/lang/spirv/writer/common/option_helpers.h
@@ -34,14 +34,15 @@
#include "src/tint/api/options/external_texture.h"
#include "src/tint/lang/spirv/writer/common/options.h"
#include "src/tint/utils/diagnostic/diagnostic.h"
+#include "src/tint/utils/result/result.h"
namespace tint::spirv::writer {
using RemapperData = std::unordered_map<BindingPoint, BindingPoint>;
/// @param options the options
-/// @returns true if the binding points are valid
-bool ValidateBindingOptions(const Options& options, diag::List& diagnostics);
+/// @return success or failure
+Result<SuccessType> ValidateBindingOptions(const Options& options);
/// Populates data from the writer options for the remapper and external texture.
/// @param options the writer options
diff --git a/src/tint/lang/spirv/writer/writer.cc b/src/tint/lang/spirv/writer/writer.cc
index e6f3098..a0d7035 100644
--- a/src/tint/lang/spirv/writer/writer.cc
+++ b/src/tint/lang/spirv/writer/writer.cc
@@ -58,9 +58,9 @@
!options.disable_workgroup_init && options.use_zero_initialize_workgroup_memory_extension;
{
- diag::List validation_diagnostics;
- if (!ValidateBindingOptions(options, validation_diagnostics)) {
- return Failure{validation_diagnostics};
+ auto res = ValidateBindingOptions(options);
+ if (!res) {
+ return res.Failure();
}
}
