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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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?



================
Comment at: clang/include/clang/AST/Type.h:743
 
+  bool isReferenceable() const;
+
----------------
Doc comments would be handy here because this is a pretty recent term of art.


================
Comment at: clang/include/clang/AST/Type.h:6480
+  //   type that is either an object type, a function type that does not have
+  //   cv-qualifiers or a ref-qualifier, or a reference type
+  const Type &Self = **this;
----------------



================
Comment at: clang/include/clang/AST/Type.h:6481
+  //   cv-qualifiers or a ref-qualifier, or a reference type
+  const Type &Self = **this;
+  if (Self.isObjectType() || Self.isReferenceType())
----------------
This is unsafe -- the `Type *` can be null if the `QualType` is invalid.


================
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);
----------------
A function without a prototype is referenceable? (This is more of a "should this predicate do anything in C?" kind of question, I suppose.)


================
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<
----------------
This can be reformatted, I believe, but did you look around to see if an existing diagnostic would suffice?


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


================
Comment at: clang/lib/Sema/SemaType.cpp:1265
+TSTToUnaryTransformType(DeclSpec::TST SwitchTST) {
+  switch (SwitchTST) { // clang-format off
+  case TST_add_const:            return UnaryTransformType::AddConst;
----------------
Same comments here about the clang-format markup.


================
Comment at: clang/lib/Sema/SemaType.cpp:1694
     if (Result.isNull()) {
-      Result = Context.IntTy;
+      if (DS.getTypeSpecType() == DeclSpec::TST_underlyingType)
+        Result = Context.IntTy;
----------------
The point to this was for error recovery; can we recover enough type information from the non-underlying type cases to recover similarly?


================
Comment at: clang/lib/Sema/SemaType.cpp:6023
     void VisitUnaryTransformTypeLoc(UnaryTransformTypeLoc TL) {
-      // FIXME: This holds only because we only have one unary transform.
-      assert(DS.getTypeSpecType() == DeclSpec::TST_underlyingType);
+      // Make sure it is a unary transform type
+      assert(DS.getTypeSpecType() >= DeclSpec::TST_underlyingType &&
----------------



================
Comment at: clang/lib/Sema/SemaType.cpp:9121
+                                    SourceLocation Loc) {
+  if (!BaseType->isPointerType())
+    return Context.getUnaryTransformType(BaseType, BaseType, UKind);
----------------
Should we care about ObjC pointers (which are a bit special)?


================
Comment at: clang/lib/Sema/SemaType.cpp:9136
+  Qualifiers Quals = Underlying.getQualifiers();
+  Quals.removeCVRQualifiers();
+  return Context.getUnaryTransformType(
----------------
What about things like type attributes (are those lost during decay)?


================
Comment at: clang/test/SemaCXX/type-traits.cpp:3447
+    { int a[T(__is_same(add_cv_t<M>, const volatile M))]; }
+    { int a[T(__is_same(add_lvalue_reference_t<M>, M))]; }
+    { int a[T(__is_same(add_pointer_t<M>, M))]; }
----------------
Shouldn't this be the same as `M&`?

Actually, something funky is going on that I've not looked into very far. When I try the test out and use `M&` instead of `M` here: https://godbolt.org/z/rx5bcsfqe, so we should be sure we're instantiating the tests we expect to instantiate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203