erichkeane added inline comments.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:2967
+ "parameters!");
+ for (FunctionProtoType::param_type_iterator
+ O = OldType->param_type_begin(),
----------------
royjacobson wrote:
> erichkeane wrote:
> > Thanks for the clarification on 'Reversed'. The comment makes it more
> > clear.
> >
> > This whole 'for' header is... really tough to mentally parse, even BEFORE
> > this, and now it is even worse with 'Reversed' involved. I would prefer
> > that the iterators be initialized ahead of time. Additionally, can we use
> > `reverse_iterator` for the 'NewType' instead of branching on `Reversed`
> > here?
> >
> > Any other suggestions you have to simplify this loop would also be
> > appreciated. I might ALSO consider using 'zip' here?
> I made it index based which IMO is easier to understand now.
>
> I thought reverse_iterator would be annoying because I would need to make the
> code a template since it's a different type.
>
> Also - ping about the comment in isBetterOverloadCandidate :)
Ah, right, you'd have 1 reverse iterator on one side of the 'zip'. Thats
unfortunate. I guess this reads well-enough though, iti s definitely an
improvement.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:9829
+ bool CanCompareConstraints = false;
+ if (Cand1.Function && Cand2.Function && Cand1.Function->hasPrototype() &&
+ Cand2.Function->hasPrototype()) {
----------------
royjacobson wrote:
> erichkeane wrote:
> > Since the point of this is to just calculate the CanCompareConstraints, I
> > think it should be a separate function called below where it is used.
> Do you mean as in a separate `Sema` function? Or a local lambda?
Just a static function is fine, basically its a whole lot of work happening in
this function for a single result, so I'm suggesting splitting off the
calculation for `CanCompareConstraints` into its own function, so you get:
`bool CanCompareConstrants = new_function(...);`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123182/new/
https://reviews.llvm.org/D123182
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
