[PATCH] D148924: [clang] Show error if defaulted comparions operator function is volatile or has ref-qualifier &&.
Jens Massberg via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 27 02:18:31 PDT 2023
massberg marked 5 inline comments as done.
massberg added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:8593
+ const FunctionProtoType *FnType = FD->getType()->castAs<FunctionProtoType>();
+ if (FnType->isVolatile()) {
----------------
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.
================
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:
> 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148924/new/
https://reviews.llvm.org/D148924
More information about the cfe-commits
mailing list