logan-5 added a comment. Glad this is generating some discussion. For my $0.02, I would also (obviously) love to be able to enable this warning on all the codebases I work on, and this patch was born out of a discussion on the C++ Slack with another user who had found this warning very useful in GCC and was wondering why Clang didn't have it yet.
In D82728#2134072 <https://reviews.llvm.org/D82728#2134072>, @dblaikie wrote: > The issue is that such a warning then needs to be off by default, because we > can't assume the user's intent. And Clang's historically been fairly averse > to off-by-default warnings due to low user-penetration (not zero, like I > said, I imagine LLVM itself would use such a warning, were it implemented) & > so concerns about the cost/benefit tradeoff of the added complexity (source > code and runtime) of the feature. I agree `-Wsuggest-override` should be off by default, yet I suspect its user-penetration will be much higher than other off-by-default warnings, due to numerous instances of people on the Internet asking for <https://stackoverflow.com/a/41109550/5379590> this feature <https://stackoverflow.com/a/29154136/5379590>, as well as the precedent for it set by GCC. Moreover, since this implementation of this warning lies along the exact same code paths as the already existing `-Winconsistent-missing-override`, the added complexity from this patch is absolutely minimal. ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3075 + : diag:: + warn_inconsistent_function_marked_not_override_overriding); + else ---------------- Quuxplusone wrote: > 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); > ``` Agreed. Good idea on the fix--needed one more line break (the first one still hit column 81), but it looks much better now. ================ Comment at: clang/test/SemaCXX/warn-suggest-destructor-override:6 + ~A() {} + void virtual run() {} +}; ---------------- Quuxplusone wrote: > Surely this doesn't compile?! Because of `void virtual`? It does, surprisingly, as it does in the test for warn-inconsistent-missing-destructor-override, where I pilfered this from. Nevertheless, changed to `virtual void` for sanity's sake. 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
