Quuxplusone added a comment.
In D82728#2133720 <https://reviews.llvm.org/D82728#2133720>, @dblaikie wrote:
> (the presence of at least one "override" being a signal the user intended to
> use override and missed some [...]
I'm in favor of `-Wsuggest-override`, and would definitely use it if it
existed. The problem I see with `-Wmissing-override`, as it's been implemented,
is that it uses the wrong granularity for "intent": it looks only across the
methods of a single class, rather than across all the classes of a single
header, or across a single translation unit, or across my entire codebase. In
real life, I //always// want to look across my entire codebase (excluding
system headers). If //any// class in my project uses `override`, I want Clang
to take that as a clear declaration of intent to use `override` throughout; I
don't want Clang treating class A differently from class B. But of course Clang
can't look at my whole codebase simultaneously. So the next best thing is to
give the user a simple way to "preload the intent flag": to say "As soon as you
start processing //any// class, please act as if intent has been declared for
that class." Adding `-Wsuggest-override` to my build line seems like a perfect
way to implement that "preload" facility.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3075
+ : diag::
+ warn_inconsistent_function_marked_not_override_overriding);
+ else
----------------
These linebreaks are super unfortunate. Could they be improved by doing it like
this?
```
auto EmitDiag = [this, MD](unsigned DiagDtor, unsigned DiagFn) {
unsigned DiagID = isa<CXXDestructorDecl>(MD) ? DiagDtor : DiagFn;
Diag(MD->getLocation(), DiagID) << MD->getDeclName();
const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
};
if (Inconsistent)
EmitDiag(diag::warn_inconsistent_destructor_marked_not_override_overriding,
diag::warn_inconsistent_function_marked_not_override_overriding);
else
EmitDiag(diag::warn_suggest_destructor_marked_not_override_overriding
diag::warn_suggest_function_marked_not_override_overriding);
```
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6764
+ if (HasOverridingMethodWithoutOverrideControl) {
+ bool InconsistentOverrideControl = HasMethodWithOverrideControl;
for (auto *M : Record->methods())
----------------
Can you s/InconsistentOverrideControl/HasInconsistentOverrideControl/ without
causing bad linebreaks?
================
Comment at: clang/test/SemaCXX/warn-suggest-destructor-override:6
+ ~A() {}
+ void virtual run() {}
+};
----------------
Surely this doesn't compile?!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82728/new/
https://reviews.llvm.org/D82728
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
