Use zip_longest for iterator range comparisons. NFC.

[Meinersbur]

Summary

Use zip_longest in two locations that compare iterator ranges. zip_longest allows the iteration using a range-based for-loop and to be symmetric over both ranges instead of prioritizing one over the other. In that latter case code have to handle the case that the first is longer than the second, the second is longer than the first, and both are of the same length, which must partially be checked after the loop.

With zip_longest, this becomes an element comparison within the loop like the comparison of the elements themselves. The symmetry makes it clearer that neither the first and second iterators are handled differently. The iterators are not event used directly anymore, just the ranges.

[dblaikie]

lib/Sema/SemaOverload.cpp

8987

I'd probably write this as "if (!std::get<0>(Pair))" - but I understand that opinions differ on such things easily enough.

lib/Serialization/ASTReaderDecl.cpp

2922

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)

[Meinersbur]

• Address @dblaikie's review

[Meinersbur]

lib/Sema/SemaOverload.cpp

8987

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.

lib/Serialization/ASTReaderDecl.cpp

2922

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

[dblaikie]

lib/Sema/SemaOverload.cpp

8987

*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?

lib/Serialization/ASTReaderDecl.cpp

2922

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)

[Meinersbur]

• Fix xor
• Store tuple elements in variables

[dblaikie]

Overall I'm not sure this construct/pattern improves readability, but not too fussed either way.

[Meinersbur]

lib/Serialization/ASTReaderDecl.cpp

2922

I knew exactly what you suggested -- I considered before going with the != version -- it seems It also only saw what I wanted to see. I still just copy&pasted from your comment to save some keystrokes. Maybe the != is less error-prone, as just demonstrated? Test cases did not fail.

[dblaikie]

lib/Serialization/ASTReaderDecl.cpp

2922

I'd still probably go with the !A || !B form - and either encourage you to add the missing test coverage, or maybe go find who owns/contributed/reviewed the code to request/suggest some testing.

lib/Sema/SemaOverload.cpp

if (!Cand1Attr || !Cand2Attr) {
     if (Cand1Attr == Cand2Attr)
       return Comparison::Equal;
     return Cand1Attr ? Comparison::Better : Comparison::Worse;
   }
 
   auto Cand1Attrs = Cand1->specific_attrs<EnableIfAttr>();
   auto Cand2Attrs = Cand2->specific_attrs<EnableIfAttr>();
 
-  auto Cand1I = Cand1Attrs.begin();
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
-  for (EnableIfAttr *Cand2A : Cand2Attrs) {
-    Cand1ID.clear();
-    Cand2ID.clear();
+  for (auto Pair : zip_longest(Cand1Attrs, Cand2Attrs)) {
+    Optional<EnableIfAttr *> Cand1A = std::get<0>(Pair);
+    Optional<EnableIfAttr *> Cand2A = std::get<1>(Pair);
 
     // It's impossible for Cand1 to be better than (or equal to) Cand2 if Cand1
-    // has fewer enable_if attributes than Cand2.
-    auto Cand1A = Cand1I++;
-    if (Cand1A == Cand1Attrs.end())
+    // has fewer enable_if attributes than Cand2, and vice versa.
+    if (!Cand1A)
       return Comparison::Worse;
+    if (!Cand2A)
+      return Comparison::Better;
+
+    Cand1ID.clear();
+    Cand2ID.clear();
 
-    Cand1A->getCond()->Profile(Cand1ID, S.getASTContext(), true);
-    Cand2A->getCond()->Profile(Cand2ID, S.getASTContext(), true);
+    (*Cand1A)->getCond()->Profile(Cand1ID, S.getASTContext(), true);
+    (*Cand2A)->getCond()->Profile(Cand2ID, S.getASTContext(), true);
     if (Cand1ID != Cand2ID)
       return Comparison::Worse;
   }
 
-  return Cand1I == Cand1Attrs.end() ? Comparison::Equal : Comparison::Better;
+  return Comparison::Equal;
 }
 
 static bool isBetterMultiversionCandidate(const OverloadCandidate &Cand1,
                                           const OverloadCandidate &Cand2) {
   if (!Cand1.Function || !Cand1.Function->isMultiVersion() || !Cand2.Function ||
       !Cand2.Function->isMultiVersion())
     return false;
 

lib/Serialization/ASTReaderDecl.cpp

 
 /// Determine whether the attributes we can overload on are identical for A and
 /// B. Will ignore any overloadable attrs represented in the type of A and B.
 static bool hasSameOverloadableAttrs(const FunctionDecl *A,
                                      const FunctionDecl *B) {
   // Note that pass_object_size attributes are represented in the function's
   // ExtParameterInfo, so we don't need to check them here.
 
-  // Return false if any of the enable_if expressions of A and B are different.
   llvm::FoldingSetNodeID Cand1ID, Cand2ID;
   auto AEnableIfAttrs = A->specific_attrs<EnableIfAttr>();
   auto BEnableIfAttrs = B->specific_attrs<EnableIfAttr>();
-  auto AEnableIf = AEnableIfAttrs.begin();
-  auto BEnableIf = BEnableIfAttrs.begin();
-  for (; AEnableIf != AEnableIfAttrs.end() && BEnableIf != BEnableIfAttrs.end();
-       ++BEnableIf, ++AEnableIf) {
+
+  for (auto Pair : zip_longest(AEnableIfAttrs, BEnableIfAttrs)) {
+    Optional<EnableIfAttr *> Cand1A = std::get<0>(Pair);
+    Optional<EnableIfAttr *> Cand2A = std::get<1>(Pair);
+
+    // Return false if the number of enable_if attributes is different.
+    if (!Cand1A || !Cand2A)
+      return false;
+
     Cand1ID.clear();
     Cand2ID.clear();
 
-    AEnableIf->getCond()->Profile(Cand1ID, A->getASTContext(), true);
-    BEnableIf->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+    (*Cand1A)->getCond()->Profile(Cand1ID, A->getASTContext(), true);
+    (*Cand2A)->getCond()->Profile(Cand2ID, B->getASTContext(), true);
+
+    // Return false if any of the enable_if expressions of A and B are
+    // different.
     if (Cand1ID != Cand2ID)
       return false;
   }
-
-  // Return false if the number of enable_if attributes was different.
-  return AEnableIf == AEnableIfAttrs.end() && BEnableIf == BEnableIfAttrs.end();
+  return true;
 }
 
 /// Determine whether the two declarations refer to the same entity.
 static bool isSameEntity(NamedDecl *X, NamedDecl *Y) {
   assert(X->getDeclName() == Y->getDeclName() && "Declaration name mismatch!");
 
   if (X == Y)
     return true;