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