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