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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
