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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


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


Repository:
  rC Clang

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

https://reviews.llvm.org/D55468