ilya-biryukov added a reviewer: clang-language-wg.
ilya-biryukov added a comment.
Thanks! I believe we should also recover in the similar manner we do for
missing `const`, see corresponding comment.
Extra NITs:
- there is a typo in description: *compariosn* should be comparison,
- we probably want to add this change to release notes.
Also adding @clang-language-wg in case someone else wants to chime in.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9434
+def err_volatile_comparison_operator : Error<
+ "defaulted comparison operator function must not be volatile">;
+def err_ref_qualifier_comparison_operator : Error<
----------------
NIT: for consistency with wording of notes above.
================
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">;
----------------
NIT: for consistency with the wording of `err_ref_qualifier_constructor`
================
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);
----------------
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?
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
