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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.



================
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);
----------------
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.


================
Comment at: clang/include/clang/AST/Type.h:6482
+  //   cv-qualifiers or a ref-qualifier, or a reference type.
+  assert(getTypePtr() && "QualType must be valid in order to be referenceable");
+  const Type &Self = **this;
----------------
Nope, that will still assert. Here's a better approach.


================
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<
----------------
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.)


================
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,
----------------
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.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3928
         break;
+      case UnaryTransformType::AddConst:
+        Out << "2ac";
----------------
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?


================
Comment at: clang/lib/Sema/DeclSpec.cpp:597
   case DeclSpec::TST_decltype_auto: return "decltype(auto)";
+  // clang-format off
   case DeclSpec::TST_underlyingType: return "__underlying_type";
----------------
cjdb wrote:
> aaron.ballman wrote:
> > We don't typically add clang-format markings to the source files. I think this should be removed (it disables the formatting for the remainder of the file).
> My intention was always to delete these pre-merge. clang-format has some really strong opinions on this that breaks consistency with the rest of the file, and it was disrupting CI.
> 
> > (it disables the formatting for the remainder of the file).
> 
> Line 615 should prevent this :-)
Ah, that makes sense and is fine by me, thanks for letting me know.


================
Comment at: clang/lib/Sema/SemaType.cpp:9121
+                                    SourceLocation Loc) {
+  if (!BaseType->isPointerType())
+    return Context.getUnaryTransformType(BaseType, BaseType, UKind);
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203