[PATCH] D100668: [ADT] Add STLForwardCompat.h and llvm::disjunction

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Apr 19 17:25:36 PDT 2021


dblaikie added a comment.

(good enough to commit - but happy to also keep discussing the finer nuances of spec conformance here if you like)



================
Comment at: llvm/include/llvm/ADT/STLForwardCompat.h:36-37
+template <typename B1, typename... Bn>
+struct conjunction<B1, Bn...>
+    : std::conditional<bool(B1::value), conjunction<Bn...>, B1>::type {};
+
----------------
scott.linder wrote:
> dblaikie wrote:
> > Hmm, seems this isn't actually conforming implementation (the standard one says that it short-circuits - doesn't try to evaluate later expressions if an early one is true/false - and that the type returned by std::conditional is the type of that last condition that proved the rule, I think?). But probably close enough since it's already committed.
> My understanding of the definition of `std::conditional` led me to believe this definition does satisfy the "short-circuiting" property, and I tried to include at least a (potentially incomplete?) trivial test of that using an `incomplete_type` following an element that proves the rule of the predicate (i.e. `false` for `conjunction`, `true` for `disjunction`)
Oh, right you are!

Is it worth testing the exact type of disjunction/conjunction, rather than only their boolean value? To test that aspect of the spec conformance? Hmm - wonder what the spec says about whether the type parameters must be able to be derived from?


================
Comment at: llvm/unittests/ADT/STLForwardCompatTest.cpp:22-30
+  EXPECT_TRUE((llvm::conjunction<>::value));
+  EXPECT_FALSE((llvm::conjunction<std::false_type>::value));
+  EXPECT_TRUE((llvm::conjunction<std::true_type>::value));
+  EXPECT_FALSE((llvm::conjunction<std::false_type, incomplete_type>::value));
+  EXPECT_FALSE((llvm::conjunction<std::false_type, std::true_type>::value));
+  EXPECT_FALSE((llvm::conjunction<std::true_type, std::false_type>::value));
+  EXPECT_TRUE((llvm::conjunction<std::true_type, std::true_type>::value));
----------------
scott.linder wrote:
> dblaikie wrote:
> > Was there no existing testing of conjunction? (ie: something that should be deleted as the testing is added/moved here?)
> Not sure, I assume it just wasn't noticed in review? There are tests of it indirectly via `Any`, see https://reviews.llvm.org/D48807
Yep, my bad. Thanks for adding in testing now


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100668/new/

https://reviews.llvm.org/D100668



More information about the llvm-commits mailing list