[PATCH] D44826: Add -Wunused-using, a warning that finds unused using declarations.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

You mention a missed case in the description - where the target of a using decl is used and that causes the unused-using not to warn about the using decl? That seems like a big limitation. Does that mean the 'used' bit is being tracked in the wrong place, on the target of the using decl instead of on the using decl itself?



================
Comment at: lib/Sema/MultiplexExternalSemaSource.cpp:278-279
+    llvm::SmallSetVector<const UsingDecl*, 4> &Decls) {
+  for(size_t i = 0; i < Sources.size(); ++i)
+    Sources[i]->ReadUnusedUsingCandidates(Decls);
+}
----------------
Could you use a range-based-for here?


================
Comment at: lib/Serialization/ASTReader.cpp:8101-8103
+    UsingDecl *D = dyn_cast_or_null<UsingDecl>(
+        GetDecl(UnusedUsingCandidates[I]));
+    if (D)
----------------
roll the declaration into the condition, perhaps:

  if (auto *D = dyn_cast_or_null...)
    Decls.insert(D);


================
Comment at: test/Modules/warn-unused-using.cpp:5
+
+// For modules, the warning should only fire the first time, when the module is
+// built.
----------------
This would only warn for the unused local using, but wouldn't fire for an namespace-scoped unused using? Perhaps there should be a test for that?


================
Comment at: test/SemaCXX/warn-unused-using.cpp:6-8
+  typedef int INT;
+  typedef char CHAR;
+  typedef float FLOAT;
----------------
Do these need to be different types? Maybe just name them t1/t2/t3/t4, etc - or name them after their use in test cases to indicate what makes each type interesting/different, if that's suitable.


================
Comment at: test/SemaCXX/warn-unused-using.cpp:11-12
+
+using nsp::Print; // expected-warning {{unused using 'Print'}}
+using nsp::var; // expected-warning {{unused using 'var'}}
+
----------------
What's the difference between these two cases?


================
Comment at: test/SemaCXX/warn-unused-using.cpp:15-20
+  using nsp::Print; // expected-warning {{unused using 'Print'}}
+  {
+    using nsp::var; // expected-warning {{unused using 'var'}}
+    {
+      using nsp::INT; // expected-warning {{unused using 'INT'}}
+      using nsp::FLOAT; // expected-warning {{unused using 'FLOAT'}}
----------------
And these 4? (I guess one in a nested scope is significant in some way? But not sure about the second scope, and the two (rather than one) using decls in there)


================
Comment at: test/SemaCXX/warn-unused-using.cpp:28
+  FLOAT f;
+  f = 2.0;
+}
----------------
Probably don't need this use? (can turn off all the other warnings, rather than crafting warning-free code when testing only one warning)


================
Comment at: test/SemaCXX/warn-unused-using.cpp:35
+struct B : A {
+  using A::Foo;  // expected-warning {{unused using 'Foo'}}
+  virtual void Foo(double x) const;
----------------
This tests the fact that 'Foo' is overridden in B anyway, so this using decl doesn't bring anything in?

I'm assuming in the case where the using decl does bring at least one name in, this warning doesn't fire? (maybe test that?)


================
Comment at: test/SemaCXX/warn-unused-using.cpp:39-42
+void one() {
+  B b;
+  b.Foo(1.0);
+}
----------------
Does B need to be used at all, here? Oh, to demonstrate that 'Foo' is using one overload, but not using the using decl?


Repository:
  rC Clang

https://reviews.llvm.org/D44826