[PATCH] D98160: [clang] Use decltype((E)) for compound requirement type constraint

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:5446
       // Build a new, canonical decltype(expr) type.
       Canon = new (*this, TypeAlignment) DependentDecltypeType(*this, e);
       DependentDecltypeTypes.InsertNode(Canon, InsertPos);
----------------
rsmith wrote:
> If we don't track the extra parens here too, we can end up giving the same canonical type to dependent `decltype` types with and without implicit parens, even though they can instantiate to different types. (But I think the simplest way to handle this would be to include the parens in the expression; see other comment.)
Good catch, but yeah I'll see about the other solution.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8643
     QualType MatchedType =
-        BuildDecltypeType(E, E->getBeginLoc()).getCanonicalType();
+        BuildDecltypeType(E, E->getBeginLoc(), true, true).getCanonicalType();
     llvm::SmallVector<TemplateArgument, 1> Args;
----------------
rsmith wrote:
> Instead of adding complexity to the type system to deal with this special case, can you directly create a `ParenExpr` here?
Hmm I thought about that. Since I am a bit unfamiliar with the code there, I was not sure it was going to end up being more or less complicated than the proposed solution. But I'll give it a shot if that looks simpler to you.


================
Comment at: clang/test/CXX/expr/expr.prim/expr.prim.req/compound-requirement.cpp:97
 template<typename T>
-concept Large = sizeof(T) >= 4; // expected-note{{because 'sizeof(short) >= 4' (2 >= 4) evaluated to false}}
+concept LargeRef = sizeof(typename reference<T>::type) >= 4;
+// expected-note at -1{{because 'sizeof(typename reference<short &>::type) >= 4' (2 >= 4) evaluated to false}}
----------------
Quuxplusone wrote:
> I think this is too large of a change. How about just keeping the old test code but changing it to
> ```
> template<typename T>
> concept Large = sizeof(std::decay_t<T>) >= 4; // expected-note{{because... etc}}
> ```
Yeah I thought about that, but on the other hand changing the test to make it a bit more strict did not look like a problem to me.
Hmm, we cover this case in the test below anyway, might as well reduce the amount of changes I guess.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98160/new/

https://reviews.llvm.org/D98160