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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
