Meinersbur marked 3 inline comments as done. Meinersbur 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; ---------------- 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. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55468/new/ https://reviews.llvm.org/D55468 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
