aaron.ballman added a comment.
Basically looking good to me now, but the changes need a release note and I
does this mean we can change cxx_status.html to claim full support for P2002R1
now or is there more left to be done for that? (If there's more left to do,
adding those details to the cxx_status page would be appropriate.)
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9436
+def err_ref_qualifier_comparison_operator : Error<
+ "ref-qualifier '&&' is not allowed on defaulted comparison operators">;
----------------
ilya-biryukov wrote:
> NIT: for consistency with the wording of `err_ref_qualifier_constructor`
Still need to drop the `s` from `operators`
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8593-8601
+ const FunctionProtoType *FnType =
FD->getType()->castAs<FunctionProtoType>();
+ if (FnType->isVolatile()) {
+ Diag(FD->getLocation(), diag::err_volatile_comparison_operator);
+ return true;
+ }
+ if (FnType->getRefQualifier() == RQ_RValue) {
+ Diag(FD->getLocation(), diag::err_ref_qualifier_comparison_operator);
----------------
massberg wrote:
> massberg wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > ilya-biryukov wrote:
> > > > > NIT: this version is simpler and more aligned with the code below.
> > > > > Also, could you move the `volatile` handling after the check for
> > > > > `const`?
> > > > >
> > > > >
> > > > > Additionally, we seem to recover in case of `const` by adding it to
> > > > > the type and suggesting a fix-it to add it in the code.
> > > > > Could we do the same for `volatile` and `ref-qualifier`, i.e. suggest
> > > > > a fix-it to remove the `ref-qualifier` and `volatile` and change the
> > > > > corresponding type to make AST pretend they were never there?
> > > >
> > > Note, you can shorten it further with:
> > > ```
> > > return Diag(...);
> > > ```
> > > because that will return true for you.
> > Thanks! Which the change suggested by Ilya this has become obsolete.
> Thanks, this is much simpler!
>
> I have moved the code and added a recovery. However, adding a fix-it is much
> more complex as there is no simple way to get location to remove the
> keywords. Thus I decided to not offering a fix-it in this patch.
Yeah, it seems we don't track the ref qualifier location on any of the
`CXXMethodDecl`, `FunctionProtoType`, or `FunctionProtoTypeLoc` classes. A
future refactoring can consider adding locations for those to
`FunctionProtoTypeLoc`, but needs to be able to handle multiple qualifiers
(e.g., `void func() const &;` should track both the `const` and the `&`
separately).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148924/new/
https://reviews.llvm.org/D148924
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
