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

Reply via email to