dblaikie 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;
----------------
Meinersbur wrote:
> 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.
*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?
================
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:
> > 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)
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
