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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/Type.h:6535
+  const auto *F = Self.getAs<FunctionProtoType>();
+  return F == nullptr ||
+         (F->getMethodQuals().empty() && F->getRefQualifier() == RQ_None);
----------------
You can turn this into an assert that `F` is not null -- C++ doesn't have the notion of functions without a prototype.


================
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:
> > 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.
The assert is fine by me, but I think you're going to need to add a `(void)IsCPlusPlus;` to the function (or a `[[maybe_unused]]` to the parameter) otherwise non-asserts builds may start to diagnose the unused parameter.


================
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:
> > 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.
Eh, may as well go with a dedicated diagnostic at that point, that seems like it's making the diagnostic far less general.


================
Comment at: clang/lib/AST/ItaniumMangle.cpp:3928
         break;
+      case UnaryTransformType::AddConst:
+        Out << "2ac";
----------------
rsmith wrote:
> cjdb wrote:
> > 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?
> This is mangling the trait as a vendor-specific type *qualifier*, so `__add_lvalue_reference(T)` will demangle as `alref T` instead. The underlying_type trait probably does this because it predates the Itanium ABI having a mangling form for a vendor-specific type *specifier* with arguments. Also, if we want this to demangle properly, we should follow the ABI specification's recommendations and use the actual source name of the builtin. So I think the ideal manglings (other than being kinda long, which probably doesn't matter given that these are unlikely to show up in actual mangled names) would be things like:
> 
> `u13__add_pointerI` ... inner type mangling ... `E`
> 
> It would probably be OK to change the mangling for `__enum_underlying_type` to this form too. I doubt that's made it into real mangled names in the wild, but if you want to add `-fclang-abi-compat` support for that, it wouldn't hurt.
Double-check -- should we be suffixing `E` on the end of these?


================
Comment at: clang/lib/Parse/ParseDecl.cpp:4286
+  case tok::kw___remove_reference:     return DeclSpec::TST_remove_reference;
+  case tok::kw___remove_restrict:     return DeclSpec::TST_remove_restrict;
+  case tok::kw___remove_volatile:      return DeclSpec::TST_remove_volatile;
----------------
Might as well make everything beautiful here. :-)


================
Comment at: clang/lib/Parse/ParseDecl.cpp:4289-4290
+  default:
+    llvm_unreachable(__FILE__
+                     "passed in an unhandled type transformation built-in");
+  } // clang-format on
----------------
assert vs unreachable? I'm on the fence. I worry about the fact that `TypeTransformTokToDeclSpec()` is pretty divorced from the list of new case statements above which determine when to call `ParseTypeTransformTypeSpecifier()`. e.g., someone may call `ParseTypeTransformTypeSpecifier()` from somewhere else and be in for a nasty surprise, so this feels more like an assert than an unreachable to me. (It's possible to get here, but if you do, you screwed something up.)


================
Comment at: clang/lib/Sema/SemaType.cpp:9155
+      BaseType.isReferenceable(
+          LangStandard::getLangStandardForKind(LangOpts.LangStd)
+              .isCPlusPlus()) ||
----------------
Why not `LangOpts.CPlusPlus`?


================
Comment at: clang/lib/Sema/SemaType.cpp:9193
+      QualType(BaseType).isReferenceable(
+          LangStandard::getLangStandardForKind(LangOpts.LangStd).isCPlusPlus())
+          ? BuildReferenceType(BaseType,
----------------
`LangOpts.CPlusPlus`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116203