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

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added inline comments.


================
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())
----------------
aaron.ballman wrote:
> This is unsafe -- the `Type *` can be null if the `QualType` is invalid.
I think this is fixed?


================
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:
> 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:


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


================
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,
----------------
WDYT @aaron.ballman?


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


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


================
Comment at: clang/lib/Sema/SemaType.cpp:9121
+                                    SourceLocation Loc) {
+  if (!BaseType->isPointerType())
+    return Context.getUnaryTransformType(BaseType, BaseType, UKind);
----------------
aaron.ballman wrote:
> Should we care about ObjC pointers (which are a bit special)?
What's the compat story between ObjC and C++?


================
Comment at: clang/lib/Sema/SemaType.cpp:9136
+  Qualifiers Quals = Underlying.getQualifiers();
+  Quals.removeCVRQualifiers();
+  return Context.getUnaryTransformType(
----------------
aaron.ballman wrote:
> What about things like type attributes (are those lost during decay)?
According to https://eel.is/c++draft/tab:meta.trans.other, no.


================
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))]; }
----------------
aaron.ballman wrote:
> 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.
Appreciate that you're thorough in your reviews! (Seriously: it's easy to start glazing over tests like this).

Per https://eel.is/c++draft/meta.trans.ref, `add_lvalue_reference_t<T>` should be `T` whenever it's not a referenceable type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203