[PATCH] D52879: Derive builtin return type from its definition

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D52879#1314694 <https://reviews.llvm.org/D52879#1314694>, @mantognini wrote:

> In D52879#1311177 <https://reviews.llvm.org/D52879#1311177>, @riccibruno wrote:
>
> > And moreover I believe this change is subtly incorrect for the following reason:
> >  The type that is passed into the constructor of the call expression is the type
> >  of the call expression, and not the return type.
> >
> > The difference between the return type and the type of the call expression is that
> >  1.) References are dropped, and
> >  2.) For C++: The cv-qualifiers of non class pr-values are dropped,
> >  3.) For C: The cv-qualifiers are dropped.
> >
> > ( as can be seen in `FunctionType::getCallResultType` )
>
>
> Could you clarify one thing for me: If I understood what you were saying, the type passed to CallExpr should not be ref-cv-qualified, is that right?
>
> In that case, couldn't we "simply" remove the ref + CV-qualifiers from ReturnTy? (`getNonReferenceType().withoutLocalFastQualifiers()`)
>
> Note: I cannot revert this //and// keep the test because the new ones will fail.


It's not that simple unfortunately. The type passed to `CallExpr` is the type of the call expression,
which is different from the return type. However you can just use `FunctionType::getCallResultType`.



================
Comment at: lib/Sema/SemaExpr.cpp:5550
   ExprResult Result;
+  QualType ReturnTy;
   if (BuiltinID &&
----------------
Please rename this to something more appropriate,
like `ResultTy` or similar.


================
Comment at: lib/Sema/SemaExpr.cpp:5553
       Fn->getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)) {
-    Result = ImpCastExprToType(Fn, Context.getPointerType(FDecl->getType()),
-                               CK_BuiltinFnToFnPtr).get();
+    // Extract the return type from the (builtin) function pointer type.
+    auto FnPtrTy = Context.getPointerType(FDecl->getType());
----------------
Please leave a FIXME indicating that someone should
go through each of the builtins and remove the
various `setType` when appropriate.


================
Comment at: lib/Sema/SemaExpr.cpp:5554
+    // Extract the return type from the (builtin) function pointer type.
+    auto FnPtrTy = Context.getPointerType(FDecl->getType());
+    Result = ImpCastExprToType(Fn, FnPtrTy, CK_BuiltinFnToFnPtr).get();
----------------
Don't use `auto` here.


================
Comment at: lib/Sema/SemaExpr.cpp:5556
+    Result = ImpCastExprToType(Fn, FnPtrTy, CK_BuiltinFnToFnPtr).get();
+    auto FnTy = FnPtrTy->getPointeeType()->castAs<FunctionType>();
+    ReturnTy = FnTy->getReturnType();
----------------
`const auto *` or just spell the type.


Repository:
  rC Clang

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

https://reviews.llvm.org/D52879