cor3ntin added inline comments.
================
Comment at: clang/include/clang/Sema/Sema.h:3775
bool UseMemberUsingDeclRules, bool ConsiderCudaAttrs = true,
- bool ConsiderRequiresClauses = true);
+ bool UseOverrideRules = false);
----------------
aaron.ballman wrote:
> I think we should add some comments here explaining what `UseOverrideRules`
> means. The old parameter was kind of mysterious as well, but this one feels
> even more mysterious to me.
Maybe we should document the whole function, but for that I'd need to better
understand `UseMemberUsingDeclRules`
The other solution might be to have an `IsOverride` function such that both
`IsOverride` and `IsOverload` function would dispatch to the same internal
function.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:911-915
if (X->getNumParams() != Y->getNumParams())
return false;
for (unsigned I = 0; I < X->getNumParams(); ++I)
if (!Ctx.hasSameUnqualifiedType(X->getParamDecl(I)->getType(),
Y->getParamDecl(I)->getType()))
----------------
aaron.ballman wrote:
> Do we need changes here?
Yes, although I need to figure out a test
================
Comment at: clang/lib/Sema/SemaOverload.cpp:996-999
// match than the non-reversed version.
return FD->getNumParams() != 2 ||
!S.Context.hasSameUnqualifiedType(FD->getParamDecl(0)->getType(),
FD->getParamDecl(1)->getType()) ||
----------------
aaron.ballman wrote:
> Do we need changes here?
Yes, although I need to figure out a test
================
Comment at: clang/lib/Sema/SemaOverload.cpp:2846-2850
// Check argument types.
for (unsigned ArgIdx = 0, NumArgs = FromFunctionType->getNumParams();
ArgIdx != NumArgs; ++ArgIdx) {
QualType FromArgType = FromFunctionType->getParamType(ArgIdx);
QualType ToArgType = ToFunctionType->getParamType(ArgIdx);
----------------
aaron.ballman wrote:
> Do we need changes here? (There may be others as well; this kind of goes back
> to "when do we need to care about explicit object arguments?".)
I've elected not to modify any of the Objective C code paths. I have no idea
how Objective c++ inherit new features nor how deducing this would impact it.
================
Comment at: clang/lib/Sema/SemaOverload.cpp:3170-3172
+ for (auto &&[Idx, Type] : llvm::enumerate(Old)) {
// Reverse iterate over the parameters of `OldType` if `Reversed` is true.
+ size_t J = Reversed ? (llvm::size(New) - Idx - 1) : Idx;
----------------
aaron.ballman wrote:
> Rather than doing this dance, will `llvm::enumerate(Reversed ?
> llvm::reverse(Old) : Old)` work? (I've never tried composing our range-based
> reverse with any other range-based API.)
What would be the type of `Reversed ? llvm::reverse(Old) : Old` ? there is no
common type between the two branches afaict
================
Comment at: clang/lib/Sema/SemaOverload.cpp:6250
+ ObjType = ObjType->getPointeeType();
+ Obj = UnaryOperator::Create(S.getASTContext(), Obj, UO_Deref, ObjType,
+ VK_LValue, OK_Ordinary, SourceLocation(),
----------------
aaron.ballman wrote:
> FWIW, we don't typically treat parameters as though they were local variables
> unless it increases readability of the code. I don't know if this quite
> qualifies or not. I don't insist on a change, but may be something to keep in
> mind as we work through the review and update code.
Here the first line of the function would have to be
`Expr* Local = Obj;` and then Obj would not be used again. I'm keeping it, lest
you insist :)
================
Comment at: clang/lib/Sema/SemaOverload.cpp:15963-15966
// FIXME: This can't currently fail, but in principle it could.
- return CreateBuiltinUnaryOp(UnOp->getOperatorLoc(), UO_AddrOf, SubExpr)
+ return CreateBuiltinUnaryOp(UnOp->getOperatorLoc(), UO_AddrOf,
+ SubExpr.get())
.get();
----------------
aaron.ballman wrote:
>
Oh wow, I didn't spot the gnarly double conversion here. Thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140828/new/
https://reviews.llvm.org/D140828
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
