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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur 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;
----------------
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.


================
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;
----------------
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).


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468