[PATCH] D55468: Use zip_longest for iterator range comparisons. NFC.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: lib/Sema/SemaOverload.cpp:8979
+    // has fewer enable_if attributes than Cand2, and vice versa.
+    if (std::get<0>(Pair) == None)
       return Comparison::Worse;
----------------
Meinersbur wrote:
> dblaikie wrote:
> > I'd probably write this as "if (!std::get<0>(Pair))" - but I understand that opinions differ on such things easily enough.
> Here I tried do be explicit that it's an `llvm::Optional` and not testing for null pointers (or whatever the payload of the llvm::Optional is, which might itself have an overload bool conversion operator). It seemed worthwhile especially because `llvm::Optional` itself does not appear itself around these lines.
*nod* Up to you - though, given std::get<0> and std::get<1> appear twice in this loop, perhaps it makes sense to pull them out into named variables and then you can name the Optional too?


================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2922
+    // Return false if the number of enable_if attributes is different.
+    if (std::get<0>(Pair).hasValue() != std::get<1>(Pair).hasValue())
+      return false;
----------------
Meinersbur wrote:
> dblaikie wrote:
> > This might be more legible as "if (std::get<0>(Pair) || std::get<1>(Pair))", I think? (optionally using "hasValue", if preferred - but comparing the hasValues to each other, rather than testing if either is false, seems a bit opaque to me at least)
> The idea was 'if the items are unequal, the list is unequal', where when either one is undefined, but not the the other, is considered unequal. The test for the elements themselves have the same structure (Cand1ID != Cand2ID).
Sorry, looks like I made a mistake in the suggested change - should be a ! before each of the gets (I wonder if the change as you have it now/as I originally suggested, is causing any test failures - one really hopes it does... because as written it looks like it'd cause this loop to always return false on the first iteration):

  if (!std::get<0>(Pair) || !std::get<1>(Pair))

& thanks for the explanation about the approach you were using - I see where you're coming from. I'd personally still lean this ^ way, I think.

(maybe if we get super ridiculous one day, and have a monadic (not sure if that's the right word) sort of conditional accessor for Optional (where you pass in a lambda over T, returning U, and the result is Optional<U>) we could write this in terms of that & then the != would fit right in... though would be a bit verbose/quirky to be sure)


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468