rsmith added a comment.
Thanks, this is very exciting, and the diagnostic changes in the test suite
look great!
================
Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:180
)cpp",
- // FIXME: it'd be nice if this resolved to the alias instead
- "struct Foo",
+ // It's so nice that this is resolved to the alias instead :-D
+ "Bar",
----------------
Very true. But probably not worth keeping the comment. :)
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/cert-static-object-exception.cpp:276
+
+auto NotOkay5 = []() { U u; return u.getBadLambda(); }();
+// CHECK-EXCEPTIONS: :[[@LINE-1]]:6: warning: initialization of 'NotOkay5'
with static storage duration may throw an exception that cannot be caught
[cert-err58-cpp]
----------------
What happened here? It looks like nothing this expression does can actually
throw. (This lambda's `operator()` isn't `noexcept`, but the functions it calls
all are.) Was this only avoiding a false positive by accident before?
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-owning-memory.cpp:94-96
+ auto owned_int7 = returns_owner1(); // Ok, since type deduction does not
eliminate the owner wrapper
+ const auto owned_int8 = returns_owner2(); // Ok, since type deduction does
not eliminate the owner wrapper
----------------
@JonasToth What was the intent here -- is there a specification for how
`gsl::owner` is supposed to interact with `auto` deduction? That is, is this a
bug fix or a regression?
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:250
Context.hasSameType(X.getAsType(), Y.getAsType()))
return X;
----------------
I wonder if we can come up with a good heuristic for determining which of `X`
and `Y` we want to return here. One thing that immediately springs to mind is:
if either type is canonical, we should return the other one.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1051-1052
+ DeduceTemplateArgumentsByTypeMatch(
+ S, TemplateParams, Params[ParamIdx].getUnqualifiedType(),
+ Args[ArgIdx].getUnqualifiedType(), Info, Deduced, TDF,
+ PartialOrdering,
----------------
Hm, do we need the `getUnqualifiedType()` calls here? I'd expect that we'd have
the `TDF_IgnoreQualifiers` flag set in this case, so we should be ignoring
qualifiers anyway. Perhaps `DeduceTemplateArgumentsByTypeMatch` should be
removing qualifiers itself if that flag is set?
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1350
+ bool PartialOrdering, bool DeducedFromArrayBound) {
+ QualType ParDesug;
+ auto updatePar = [&Param, &ParDesug, &S](QualType T) {
----------------
`Par` is an unusual name for a parameter type; `Parm` or `Param` would be more
common and I'd find either of those to be clearer. (Ideally use the same
spelling for `Param` and `ParDesug`.) Given that the description in the
standard uses `P` and `A` as the names of these types, those names would be
fine here too if you want something short.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1350-1362
+ QualType ParDesug;
+ auto updatePar = [&Param, &ParDesug, &S](QualType T) {
+ Param = T;
+ ParDesug = T.getDesugaredType(S.Context);
+ };
+ updatePar(Param);
+
----------------
rsmith wrote:
> `Par` is an unusual name for a parameter type; `Parm` or `Param` would be
> more common and I'd find either of those to be clearer. (Ideally use the same
> spelling for `Param` and `ParDesug`.) Given that the description in the
> standard uses `P` and `A` as the names of these types, those names would be
> fine here too if you want something short.
Instead of tracking two types here, I think it'd be clearer to follow the more
usual pattern in Clang: track only `Param` and `Arg`, use `Arg->getAs<T>()`
instead of `dyn_cast<T>(ArgDesug)` to identify if `Arg` is sugar for a `T`, and
use `Arg->getAs<T>()` instead of `cast<T>(ArgDesug)` to get a
minimally-desugared `T*` from `Arg`.
The only place where I think we'd want something different from that is in the
`switch (Param->getTypeClass())` below, where I think we should switch on the
type class of the canonical type (or equivalently on the type class of the
desugared type, but it's cheaper and more obviously correct to ask for the type
class of the canonical type).
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1561-1562
// Check the cv-qualifiers on the parameter and argument types.
CanQualType CanParam = S.Context.getCanonicalType(Param);
CanQualType CanArg = S.Context.getCanonicalType(Arg);
if (!(TDF & TDF_IgnoreQualifiers)) {
----------------
Can we remove these? It looks like we're almost not using them at all, and that
we shouldn't need to use them: any type matching should work equally well with
the desugared type, and we never want to include these in our deduced results
or our diagnostics.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1643
- return Param == Arg? Sema::TDK_Success : Sema::TDK_NonDeducedMismatch;
+ return ParDesug == ArgDesug ? Sema::TDK_Success
+ : Sema::TDK_NonDeducedMismatch;
----------------
This looks wrong to me: we should be comparing the types, not how they're
written. `Context.hasSameType(Param, Arg)` (or
`Context.hasSameUnqualifiedType(Param, Arg)` in the `TDF_IgnoreQualifiers`
case) would be appropriate here.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:1811-1812
+ S, TemplateParams,
+ FunctionProtoParam->getReturnType().getUnqualifiedType(),
+ FunctionProtoArg->getReturnType().getUnqualifiedType(), Info,
+ Deduced, 0,
----------------
Ignoring qualifiers here doesn't seem right to me. For example:
```
template<typename T> void f(T(T));
const int g(int);
void h() { f(g); }
```
... should result in a deduction failure, because we should get `T = const int`
from the return type but `T = int` from the parameter type.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2306
return Sema::TDK_Success;
- }
+ }
----------------
Presumably this indentation change is unintended?
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2378-2389
+ auto *E = Param.getAsExpr();
+ if (!E->isValueDependent()) {
+ if (const auto Int = E->getIntegerConstantExpr(S.Context)) {
+ if (Arg.getKind() == TemplateArgument::Integral) {
+ if (hasSameExtendedValue(*Int, Arg.getAsIntegral()))
+ return Sema::TDK_Success;
+ }
----------------
This suggests that we're passing in unresolved template arguments to this
function. That seems problematic to me: those unresolved template arguments
(as-written) will not be properly converted to the template's parameter type,
will miss default arguments, and might not have had arguments properly combined
into packs.
For *type* template arguments, I think it's fine to pass the argument as
written if we're sure we know what it is, because basically no conversions
apply for type template parameters and there may be meaningful sugar we want to
preserve, but otherwise I think we should be passing in the resolved template
argument.
================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:4831
QualType FuncParam =
- SubstituteDeducedTypeTransform(*this, TemplArg, /*UseTypeSugar*/false)
+ SubstituteDeducedTypeTransform(*this, TemplArg, /*UseTypeSugar*/ true)
.Apply(Type);
----------------
Hm, I see, adding the type sugar here presumably means that we get better
diagnostic quality: we can complain that we can't deduce (eg) `auto*` against
`int` instead of complaining that we can't deduce `type-parameter-0-0*` against
`int`, right?
Can we remove the `UseTypeSugar` flag entirely now, or is it used for something
else too?
================
Comment at: clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp:565
+ void bar1(const char * __restrict);
+ void t1() { foo1(&bar1); }
+
----------------
It'd be nice to check that this deduces the right type for `T`. I think this
case would also be useful to test:
```
template<class T> void foo0(fptr1<T>) {
using ExpectedT = const char*;
using ExpectedT = T;
}
void bar0(const char *const volatile __restrict);
void t0() { foo0(&bar0); }
```
================
Comment at: clang/test/SemaCXX/cxx1z-class-template-argument-deduction.cpp:569
+ void bar2(fptr1<const char * __restrict>);
+ void t2() { foo2(&bar2); }
+
----------------
Interesting, GCC rejects this due to the restrict qualifier mismatch. But I
think accepting is correct.
================
Comment at: clang/test/SemaTemplate/attributes.cpp:127-128
};
auto it = MemberTemplate<int>::Iter<const int>();
- int n = it; // expected-error {{no viable conversion from
'preferred_name::MemberTemplate<int>::const_iterator' to 'int'}}
+ int n = it; // expected-error {{no viable conversion from
'MemberTemplate<int>::Iter<const int>' to 'int'}}
----------------
Looks like we'll need to use a different mechanism to remove the type sugar in
this test. (Maybe one day we'll run out of ways to remove type sugar and we
won't need this test any more!)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110216/new/
https://reviews.llvm.org/D110216
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
