commit c98afebda8a80490f21c97fca137e353143d330e
author Corentin Wallez <cwallez@chromium.org> Tue Apr 09 18:14:40 2024 +0000
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue Apr 09 18:14:40 2024 +0000
tree 833dac862227c66a1d03b31a1a436463172d287b
parent f14eb94bf9dd58432673bfe186d51711f07d8705

RefBase: make operator bool explicit.

Otherwise doing refA != refB could end up converting both sides to bool
and then perform the comparison of the two bools. Definitely not what
was intended.

Also adds a test and removes suppressions.

Fixed: dawn:2489
Change-Id: I2566a8101f694feb40d49ce296ad6db37f0c612c
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/183121
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>

src/dawn/common/RefBase.h

diff --git a/src/dawn/common/RefBase.h b/src/dawn/common/RefBase.h
index 9785407..bdf6ec8 100644
--- a/src/dawn/common/RefBase.h
+++ b/src/dawn/common/RefBase.h
@@ -117,13 +117,15 @@
         return *this;
     }
 
-    operator bool() const { return !!mValue; }
+    explicit operator bool() const { return !!mValue; }
 
     // Comparison operators.
     bool operator==(const T& other) const { return mValue == other; }
-
     bool operator!=(const T& other) const { return mValue != other; }
 
+    bool operator==(const RefBase<T, Traits>& other) const { return mValue == other.mValue; }
+    bool operator!=(const RefBase<T, Traits>& other) const { return mValue != other.mValue; }
+
     const T operator->() const { return mValue; }
     T operator->() { return mValue; }
 

src/dawn/tests/unittests/RefBaseTests.cpp

diff --git a/src/dawn/tests/unittests/RefBaseTests.cpp b/src/dawn/tests/unittests/RefBaseTests.cpp
index ee70d2f..f898275 100644
--- a/src/dawn/tests/unittests/RefBaseTests.cpp
+++ b/src/dawn/tests/unittests/RefBaseTests.cpp
@@ -316,5 +316,30 @@
                                              Event{Action::kMarker, 30}));
 }
 
+// Regression test for an issue where RefBase<T*> comparison would end up using operator bool
+// depending on the order in which the compiler did implicit conversions.
+struct FakePtrRefTraits {
+    static constexpr int* kNullValue{nullptr};
+    static void Reference(int*) {}
+    static void Release(int*) {}
+};
+TEST(RefBase, MissingExplicitOnOperatorBool) {
+    using MyRef = RefBase<int*, FakePtrRefTraits>;
+    int a = 0;
+    int b = 1;
+    MyRef refA(&a);
+    MyRef refB(&b);
+
+    EXPECT_TRUE(refA == refA);
+    EXPECT_FALSE(refA != refA);
+    EXPECT_TRUE((refA) == (refA));
+    EXPECT_FALSE((refA) != (refA));
+
+    EXPECT_FALSE(refA == refB);
+    EXPECT_TRUE(refA != refB);
+    EXPECT_FALSE((refA) == (refB));
+    EXPECT_TRUE((refA) != (refB));
+}
+
 }  // anonymous namespace
 }  // namespace dawn

src/dawn/tests/unittests/validation/ObjectCachingTests.cpp

diff --git a/src/dawn/tests/unittests/validation/ObjectCachingTests.cpp b/src/dawn/tests/unittests/validation/ObjectCachingTests.cpp
index 80280b7..8a6071a 100644
--- a/src/dawn/tests/unittests/validation/ObjectCachingTests.cpp
+++ b/src/dawn/tests/unittests/validation/ObjectCachingTests.cpp
@@ -125,10 +125,6 @@
 // Test that BindGroupLayouts with a static sampler entry are correctly
 // deduplicated.
 TEST_F(ObjectCachingTest, BindGroupLayoutStaticSamplerDeduplication) {
-    // TODO(crbug.com/dawn/2489): The inequality check between bind group
-    // layouts with distinct static samplers fails on the MSVC bots.
-    DAWN_SKIP_TEST_IF(DAWN_PLATFORM_IS(WINDOWS));
-
     wgpu::BindGroupLayoutEntry binding = {};
     binding.binding = 0;
     wgpu::StaticSamplerBindingLayout staticSamplerBinding = {};