logan-5 marked 2 inline comments as done. logan-5 added a comment. In D82728#2137021 <https://reviews.llvm.org/D82728#2137021>, @dblaikie wrote:
> I think it might be nice to make the -Wno-inconsistent-missing-override > -Wsuggest-override situation a bit better (by having it still do the same > thing as -Wsuggest-override) but I don't feel /too/ strongly about it. So, ironing this out would mean the code would need this structure: if (Inconsistent && IsWarningEnabled(-Winconsistent-missing-override)) Emit(-Winconsistent-missing-override); else Emit(-Wsuggest-override); The issue is that I wasn't able to find a way to ask if a warning is enabled and make a control flow decision based on that. If there is an API for doing this, it's hidden well. :) So I fell back to just doing `if (Inconsistent)` instead, which has this quirk if the inconsistent version of the warning is disabled. ================ Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3064 if (MD->size_overridden_methods() > 0) { - unsigned DiagID = isa<CXXDestructorDecl>(MD) - ? diag::warn_destructor_marked_not_override_overriding - : diag::warn_function_marked_not_override_overriding; - Diag(MD->getLocation(), DiagID) << MD->getDeclName(); - const CXXMethodDecl *OMD = *MD->begin_overridden_methods(); - Diag(OMD->getLocation(), diag::note_overridden_virtual_function); + auto EmitDiag = [this, MD](unsigned DiagDtor, unsigned DiagFn) { + unsigned DiagID = isa<CXXDestructorDecl>(MD) ? DiagDtor : DiagFn; ---------------- dblaikie wrote: > Generally I'd recommend default ref capture `[&]` on any lambda that doesn't > escape its scope - normal scopes don't need to document which variables you > use inside them, and I think the same applies to lambdas (bit more debatable > when the lambda is named and called later, even within the same scope - so if > you feel strongly about keeping it the way it is, that's OK) I don't feel strongly at all; I'm fine with `[&]`. I'll make that change. ================ Comment at: clang/test/SemaCXX/warn-suggest-override:3-4 + +class A { + public: + ~A() {} ---------------- dblaikie wrote: > I'd probably simplify these tests by using struct, so everything's implicitly > public, rather than class and having to make things public. > > Also probably member function declarations rather than definitions would be > simpler? Can do. 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