This is an archive of the discontinued LLVM Phabricator instance.

Add -Wunused-using, a warning that finds unused using declarations.
AcceptedPublic

Authored by CarlosAlbertoEnciso on Mar 23 2018, 6:35 AM.

Details

Summary

Add -Wunused-using, a warning that finds unused using declarations.
Also added an alias -Wunused-usings.

This patch uses a similar approach as the work done for the typedefs
as discussed in:

https://reviews.llvm.org/rC217298

It uses the 'referenced' available bit for the declarations.

For the below test:

namespace nsp {
  void foo();
}

using foo;

A debug information entries are generated for the namespace
and for the using declaration.

In order to reduce the debug information for those specific cases,
the work is divided in 2 parts:

  • Emit a warning for the unused using
  • Do not generate debug information for the unused using

The current patch deals with the first part and it covers global
and local detection of unused using declarations.

Thanks for your view on this issue and on the general approach.

Note: This patch is dependent on

https://reviews.llvm.org/D46190

which is under review.

Diff Detail

Event Timeline

My opinion matters less than @rsmith or @dblaikie on the review, but it seems to me that Typedef and Using are SO similar that the implementations should just be combined. You'd likely have to change a couple of types along the way to be more generic, but the implementations are essentially a copy/paste of eachother.

My opinion matters less than @rsmith or @dblaikie on the review, but it seems to me that Typedef and Using are SO similar that the implementations should just be combined. You'd likely have to change a couple of types along the way to be more generic, but the implementations are essentially a copy/paste of eachother.

That is a very valid point and it simplifies quite a lot the patch.

If the other reviewers do not have any objection, I will combine both implementations and update the uploaded patch.

Thanks,
Carlos

While implementing the warning is great (wonder if there's any codebase
that isn't -Wunused-using clean, that we could use to compare Clang and
GCC's behavior broadly - make sure it's catching the same cases (or
justify/investigate differences)) - and using it to motivate the debug info
is an improvement to the debug info - it won't quite address all the wasted
debug info, unfortunately :/

Consider this:

namespace a {

struct b;

};
namespace x {

using a::b;
inline void f(b*) {
}

}

Now the using declaration is used, but if 'f' is never called in this
translation unit, it's a bit weird to produce debug info for the using decl
and could still substantially bloat debug info. (indeed most of the bloat
that the using decl/directive debug info is producing is probably from
directives that are used, but not in a way that's relevant to a certain
translation unit)

I've not looked at the change yet, but if it's particularly
expensive/complicated to wire up the debug info side, it might not be worth
it given it's probably not a significant savings & somewhat of a dead-end
compared to what would be needed for a more complete fix. But I guess it's
probably not expensive/complicated, so probably some fine low hanging fruit
to pick until a more complete fix/improvement is implemented.

This duplicates Clang-tidy misc-unused-using-decls. If Clang will provide same or better functionality, it should be removed.

Please also mention new warning in Release Notes and documentation.

Will this warning be part of -Wall or -Wextra?

aprantl removed a subscriber: aprantl.Mar 23 2018, 11:28 AM

This duplicates Clang-tidy misc-unused-using-decls. If Clang will provide same or better functionality, it should be removed.

Thanks for bringing this up.

Please also mention new warning in Release Notes and documentation.

I will do it.

Will this warning be part of -Wall or -Wextra?

The warning is part of -Wall

Thanks for your feedback.

First of all, thanks very much for your feedback.

While implementing the warning is great (wonder if there's any codebase
that isn't -Wunused-using clean, that we could use to compare Clang and
GCC's behavior broadly - make sure it's catching the same cases (or
justify/investigate differences)) - and using it to motivate the debug info
is an improvement to the debug info - it won't quite address all the wasted
debug info, unfortunately :/

You are making a very good point. The -Wunused-using is just addressing
one of the factors affecting the debug info size.

Consider this:

namespace a {

struct b;

};
namespace x {

using a::b;
inline void f(b*) {
}

}

Now the using declaration is used, but if 'f' is never called in this
translation unit, it's a bit weird to produce debug info for the using decl
and could still substantially bloat debug info. (indeed most of the bloat
that the using decl/directive debug info is producing is probably from
directives that are used, but not in a way that's relevant to a certain
translation unit)

The modified patch catches that 'referenced' but 'unused' using declaration.

I've not looked at the change yet, but if it's particularly
expensive/complicated to wire up the debug info side, it might not be worth
it given it's probably not a significant savings & somewhat of a dead-end
compared to what would be needed for a more complete fix. But I guess it's
probably not expensive/complicated, so probably some fine low hanging fruit
to pick until a more complete fix/improvement is implemented.

The warning generation check is done at the point where a lexical scope
is closed and it does not 'see' further changes on the same using after that
point.

The second part (do not generate debug info) uses a similar approach, with
the only difference that the checks and debug info generation is done at
the end of the compilation unit, when more information is available.

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?

lib/Sema/MultiplexExternalSemaSource.cpp
278–279

Could you use a range-based-for here?

lib/Serialization/ASTReader.cpp
8104–8106

roll the declaration into the condition, perhaps:

if (auto *D = dyn_cast_or_null...)
  Decls.insert(D);
test/Modules/warn-unused-using.cpp
6

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?

test/SemaCXX/warn-unused-using.cpp
6–8 ↗(On Diff #139581)

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.

11–12 ↗(On Diff #139581)

What's the difference between these two cases?

15–20 ↗(On Diff #139581)

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)

28 ↗(On Diff #139581)

Probably don't need this use? (can turn off all the other warnings, rather than crafting warning-free code when testing only one warning)

35 ↗(On Diff #139581)

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?)

39–42 ↗(On Diff #139581)

Does B need to be used at all, here? Oh, to demonstrate that 'Foo' is using one overload, but not using the using decl?

@dblaikie,

Thanks for your feedback and questions. I will address them.

Currently I am looking with more detail the tracking of the 'used' and 'referenced' bits.

CarlosAlbertoEnciso edited the summary of this revision. (Show Details)

This patch addresses the reviewers comments:

  1. Merge the -Wunused-local-typedefs and -Wunused-usings implementations
  2. Update the Release Notes
  3. Review the test cases
CarlosAlbertoEnciso marked 2 inline comments as done.May 25 2018, 4:01 AM
CarlosAlbertoEnciso added inline comments.
lib/Serialization/ASTReader.cpp
8104–8106

Done.

test/Modules/warn-unused-using.cpp
6

I have added a namespace-scoped unused using.

Please improve test coverage a bit.
-Wunused-using is part of -Wunused.
And -Wunused is part of -Wall.
That needs to be tested.

Also, one small test to show that it is not on by default,
and -Wno-unused-using actually disables it is needed.

docs/ReleaseNotes.rst
117
``-Wall``
117
``-Wunused-using``
include/clang/Basic/DiagnosticGroups.td
828–829

Why? gcc compatibility?

include/clang/Basic/DiagnosticSemaKinds.td
282–284

http://en.cppreference.com/w/cpp/keyword/using says that there are multiple flavors.
It may be more friendlier to have more than one single message for all of them.

test/SemaCXX/referenced_alias_declaration_1.cpp
2

The svn attribute changes are likely unintentional.

test/SemaCXX/referenced_alias_declaration_2.cpp
2

The svn attribute changes are likely unintentional.

test/SemaCXX/referenced_using_all.cpp
2

The svn attribute changes are likely unintentional.

test/SemaCXX/referenced_using_declaration_1.cpp
2

The svn attribute changes are likely unintentional.

test/SemaCXX/referenced_using_declaration_2.cpp
2

The svn attribute changes are likely unintentional.

test/SemaCXX/referenced_using_directive.cpp
2

The svn attribute changes are likely unintentional.

CarlosAlbertoEnciso marked 2 inline comments as done.May 26 2018, 6:43 AM

@lebedev.ri,

Thanks very much for your review. I will address those issues and update the patch.

include/clang/Basic/DiagnosticGroups.td
828–829

No particular reason. I just follow the 'unused-local-typedefs' model.
If there is not objection from others reviewers, I will drop the gcc compatibility.

This patch addresses the reviewers comments:

  • Additional test cases to cover: -Wunused, -Wall and -Wno-unused-using
  • Formatting in ReleaseNotes
  • Removed the '-Wunused-usings' alias (GCC compatibility).
  • Added individual warning messages for using-declaration, using-directive and namespace-alias.
  • Fixed SVN attribute changes.
CarlosAlbertoEnciso marked 11 inline comments as done.May 31 2018, 4:40 AM
thakis accepted this revision.May 31 2018, 7:41 AM
thakis added a subscriber: thakis.

Looks great.

docs/ReleaseNotes.rst
126

Maybe include an example with a using directive and a namespace alias as well.

139

nit: I'd omit this paragraph -- this is true for all warnings and not special for this warning.

include/clang/Basic/DiagnosticGroups.td
828–829

Does gcc have a -Wunused-usings? As far as I can tell it doesn't, so I agree not having the alias makes sense. -Wunused-local-typedefs is here because gcc has that flag.

This revision is now accepted and ready to land.May 31 2018, 7:41 AM
CarlosAlbertoEnciso marked 3 inline comments as done.

Address feedback from @thakis in relation to the Release Notes.

docs/ReleaseNotes.rst
139

Removed that specific paragraph.

include/clang/Basic/DiagnosticGroups.td
828–829

As far as I can see, GCC does not have the `-Wunused-usings` alias.

Thanks to all reviewers for your comments and suggestions.

lebedev.ri added inline comments.Jun 1 2018, 4:37 AM
include/clang/Basic/DiagnosticSemaKinds.td
282–290

JFYI you can condense it down to just

def warn_unused_using_declaration : Warning<
  "unused %select{using declaration|using directive|namespace alias}0 %1">,
  InGroup<UnusedUsing>, DefaultIgnore;

if that simplifies the code that actually emits that warning.

include/clang/Basic/DiagnosticSemaKinds.td
282–290

Thanks for the suggestion.

I will have a look at it and see if that simplifies the code that emits the warning.