[PATCH] D110216: [clang] retain type sugar in auto / template argument deduction
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Sep 29 12:46:06 PDT 2021
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
More information about the cfe-commits
mailing list