[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

In D116203#3255018 <https://reviews.llvm.org/D116203#3255018>, @aaron.ballman wrote:

> In D116203#3220165 <https://reviews.llvm.org/D116203#3220165>, @aaron.ballman wrote:
>
>> The summary for the patch explains what's being added, but there's really no information as to why it's being added. Why do we need these builtins?
>
> Btw, I still think the patch summary should be updated with this information.

Sorry, I completely missed this. Done.

In D116203#3277515 <https://reviews.llvm.org/D116203#3277515>, @zoecarver wrote:

> This patch looks awesome, Chris.
>
> Does it make sense to have builtins for `add_const`, etc.? Isn't `T const` already kind of a builtin for this?

Potentially, but I need a core language lawyer to weigh in here. The library wording for `std::add_const<T>::type` is:

> If `T` is a reference, function, or top-level `const`-qualified type, then type names the same type as `T`, otherwise `T const`.

Although my understanding is that the `T const` in this case is redundant (and thus not applied per dcl.type.cv <https://eel.is/c++draft/dcl.type.cv#1>), the library wording goes out of its way to say "do nothing".



================
Comment at: clang/include/clang/AST/Type.h:6488
+  const auto *F = Self.getAs<FunctionProtoType>();
+  return F == nullptr ||
+         (F->getMethodQuals().empty() && F->getRefQualifier() == RQ_None);
----------------
aaron.ballman wrote:
> cjdb wrote:
> > aaron.ballman wrote:
> > > A function without a prototype is referenceable? (This is more of a "should this predicate do anything in C?" kind of question, I suppose.)
> > Does C++ have a notion of non-prototyped functions? I don't think it does? As such, I'm struggling to model this in a way that makes sense :(
> > 
> > Maybe that's evidence for NAD, maybe it isn't :shrug:
> C++ doesn't have the notion of a function without a prototype (thankfully).
> 
> Should this function assert that it's not called in C mode at all? I don't think asking the question makes sense in C.
I made a change in that this asserts when not in C++ mode.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8595
+def err_make_signed_integral_only : Error<
+  "'%select{make_unsigned|make_signed}0' is only compatible with non-bool integers and enum types, but was given %1">;
 def ext_typecheck_cond_incompatible_pointers : ExtWarn<
----------------
aaron.ballman wrote:
> cjdb wrote:
> > aaron.ballman wrote:
> > > This can be reformatted, I believe, but did you look around to see if an existing diagnostic would suffice?
> > Do you have any tips on how to narrow my search? I don't really want to //read// 11K lines of diagnostics.
> I usually search for "typecheck" for type checking diagnostics. One that looks close is:
> ```
> def err_pragma_loop_invalid_argument_type : Error<
>   "invalid argument of type %0; expected an integer type">;
> ```
> that could likely be changed to something along the lines of:
> ```
> def err_invalid_argument_type : Error<
>   "invalid argument of type %0; expected an integer type %select{|other than 'bool'}1">;
> ```
> (I think enum types are always integer types and so they may not need to be called out in the diagnostic.)
I'm not particularly fond of this one because it doesn't call out the builtin's user-facing name. I suppose we could do this, but I'm not sure where to draw the line:

```
def err_invalid_argument_type : Error<
  "invalid argument %select{|for 'make_%select{signed|unsigned}2'}1, with type %0; expected an integer type %select{|other than 'bool'}3">;
```

This means that users who misuse `std::make_[un]signed` will get a nice diagnostic instead of one targeting `__make_[un]signed`, which helps them work out where they can fix their code.


================
Comment at: clang/lib/AST/ASTContext.cpp:5604
 /// savings are minimal and these are rare.
+// Update 2021-12-16: it might be worth revisiting this
 QualType ASTContext::getUnaryTransformType(QualType BaseType,
----------------
aaron.ballman wrote:
> cjdb wrote:
> > WDYT @aaron.ballman?
> Are these expected to be less rare now due to your patch? FWIW, I'm fine revisiting if we can measure some value, but I think that's good as a separate patch.
I've got no idea, which is why I'm flagging it. Each of these built-in traits makes a call, so it's no longer just `__underlying_type`. Sounds like I can resolve for now.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3928
         break;
+      case UnaryTransformType::AddConst:
+        Out << "2ac";
----------------
aaron.ballman wrote:
> Are these the suggested manglings from the Itanium mangling document, or something you invented yourself?
> 
> Should there be corresponding Microsoft manglings or are those already handled magically?
> 
> Also, test coverage for the manglings?
I copied the mangling from D67052 and then inferred for what's missing over there. I'll consult the Itanium mangling doc to ensure that they're correct. How can I check the corresponding Microsoft manglings are handled?


================
Comment at: clang/lib/Sema/SemaType.cpp:9113
+      BaseType.isReferenceable() || BaseType->isVoidType()
+          ? BuildPointerType(BaseType.getNonReferenceType(), Loc, EntityName)
+          : BaseType;
----------------
erichkeane wrote:
> Do we at any point want this builtin to be address-space aware?  
I'm not against this functionality, but there isn't (currently) motivation for it. WDYT about `__add_pointer(T, address_space)`? Do we want this to be a separate builtin (e.g. `__add_pointer_with_address_space(T, address_space)`)?


================
Comment at: clang/lib/Sema/SemaType.cpp:9227
 
-        DiagnoseUseOfDecl(ED, Loc);
+  QualType Underlying = Context.getIntTypeForBitwidth(
+      Context.getIntWidth(BaseType), IsMakeSigned);
----------------
erichkeane wrote:
> Can you add a couple of tests to make sure this works with _BitInt?  Note that this + the libc++ fixes get this done: https://github.com/llvm/llvm-project/issues/50427
Done for `_BitInt`. Is there a corresponding unsigned `_BitInt`? Neither `_BitUint`, `BitUInt`, nor `_UBitInt` work :(


================
Comment at: clang/lib/Sema/SemaType.cpp:9121
+                                    SourceLocation Loc) {
+  if (!BaseType->isPointerType())
+    return Context.getUnaryTransformType(BaseType, BaseType, UKind);
----------------
aaron.ballman wrote:
> cjdb wrote:
> > aaron.ballman wrote:
> > > Should we care about ObjC pointers (which are a bit special)?
> > What's the compat story between ObjC and C++?
> Objective-C++ is a thing: https://en.wikipedia.org/wiki/Objective-C#Objective-C++
> 
> The salient point is that the type system models ObjC pointers as being distinct from pointers. e.g., https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/Type.h#L6702
PTAL


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203