Page MenuHomePhabricator

Add readability-duplicate-include check to clang-tidy
AbandonedPublic

Authored by LegalizeAdulthood on Feb 28 2015, 11:06 PM.

Details

Reviewers
alexfh
sbenza
Summary

This check looks for redundant header file inclusion.

Every time a #include is processed in the main file, it checks a vector of filenames to see if the included file has already been included. If so, it issues a warning and a replacement to remove the entire line containing the duplicated include directive.

When a macro is defined or undefined in the main file, the vector of filenames is cleared. This enables the following use case:

class LangOptionsBase {
public:
  // Define simple language options (with no accessors).
#define LANGOPT(Name, Bits, Default, Description) unsigned Name : Bits;
#define ENUM_LANGOPT(Name, Type, Bits, Default, Description)
#include "clang/Basic/LangOptions.def"

protected:
  // Define language options of enumeration type. These are private, and will
  // have accessors (below).
#define LANGOPT(Name, Bits, Default, Description)
#define ENUM_LANGOPT(Name, Type, Bits, Default, Description) \
  unsigned Name : Bits;
#include "clang/Basic/LangOptions.def"
};

Since macros are redefined between the inclusion of LangOptions.def, they are not flagged as redundant.

Diff Detail

Event Timeline

LegalizeAdulthood retitled this revision from to Add readability-redundant-include check to clang-tidy.
LegalizeAdulthood updated this object.
LegalizeAdulthood edited the test plan for this revision. (Show Details)
LegalizeAdulthood added a reviewer: alexfh.
LegalizeAdulthood added a subscriber: Unknown Object (MLST).

Add doxygen comments to check's .h file

alexfh edited edge metadata.Mar 2 2015, 7:07 AM

I'd better put the effort in making the llvm-include-order check extensible (or configurable) enough to support custom include order, and then implement this inside the include order check. There's a TODO, BTW ;)

Will this suit your needs?

I think there is benefit to checking for redundant includes separately from include order.

On many code bases where I've worked the include order is all over the place. The main barrier in my experience to having a consistent include order hasn't been one of tools, but getting agreement from the team. For the llvm/clang code bases the team has already decided on the order of includes, but for many teams that can start a big political battle.

However, noone is going to disagree with eliminating redundant includes.

alexfh added a comment.Mar 3 2015, 7:45 AM

I think there is benefit to checking for redundant includes separately from include order.

On many code bases where I've worked the include order is all over the place. The main barrier in my experience to having a consistent include order hasn't been one of tools, but getting agreement from the team. For the llvm/clang code bases the team has already decided on the order of includes, but for many teams that can start a big political battle.

However, noone is going to disagree with eliminating redundant includes.

Makes sense. See a couple of initial comments below.

I have a feeling that some common machinery could be pulled from the llvm-include-order check which could benefit this check as well. I'll try to come up with a specific idea later.

clang-tidy/readability/CMakeLists.txt
11

I'm not a native speaker, but I have a feeling that "duplicate" is a more suitable word here than "redundant". WDYT?

clang-tidy/readability/RedundantInclude.h
22 ↗(On Diff #20953)

This description is somewhat confusing. Maybe something along the lines of "Only consecutive #include directives without any other preprocessor directives between them are analyzed"?

clang-tidy/readability/CMakeLists.txt
11

Fixed.

clang-tidy/readability/RedundantInclude.h
22 ↗(On Diff #20953)

Fixed.

LegalizeAdulthood retitled this revision from Add readability-redundant-include check to clang-tidy to Add readability-duplicate-include check to clang-tidy.
LegalizeAdulthood edited edge metadata.

Update check description in header file.
Rename check class to DuplicateIncludeCheck
Rename check to readability-duplicate-include

alexfh added inline comments.Mar 9 2015, 10:30 AM
clang-tidy/readability/DuplicateIncludeCheck.cpp
62

What's the reason to limit the check to the main file only? I think, it should work on all headers as well. Also, sometimes it's fine to have duplicate includes even without macro definitions in between, e.g. when these #includes are in different namespaces.

I'd suggest using the same technique as in the IncludeOrderCheck: for each file collect all preprocessor directives sorted by SourceLocation. Then detect #include blocks (not necessarily the same way as its done in the IncludeOrderCheck. Maybe use the presense of any non-comment tokens between #includes as a boundary of blocks), and detect duplicate includes in each block.

clang-tidy/readability/DuplicateIncludeCheck.cpp
62

If I remove the isInMainFile, how do I ensure that I don't attempt to modify system headers?

alexfh added inline comments.Mar 15 2015, 5:08 PM
clang-tidy/readability/DuplicateIncludeCheck.cpp
62

Using SourceLocation::isInSystemHeader().

clang-tidy/readability/DuplicateIncludeCheck.cpp
62

Thanks, I'll try that. The next question that brings to mind is how do I distinguish between headers that I own and headers used from third party libraries?

For instance, suppose I run a check on a clang refactoring tool and it uses isInSystemHeader and starts flagging issues in the clang tooling library headers.

The compile_commands.json doesn't contain any information about headers in my project, only translation units in my build, so it doesn't know whether or not included headers belong to me or third-party libraries.

clang-tidy/readability/DuplicateIncludeCheck.cpp
62

For the benefit of others reading this, Alex pointed out to me the -header-filter and -system-headers options to clang-tidy. I think this means I don't need any narrowing if isExpansinInMainFile() in any of my matchers. I will do some experimenting to verify that this doesn't introduce regressions.

sbenza added inline comments.Mar 18 2015, 1:46 PM
clang-tidy/readability/DuplicateIncludeCheck.cpp
23

Function names start with lowercase.

28

This seems very wasteful, given that the SourceManager could tell you (if there was an API) the location for the next line in O(1) time.
Don't know if it is worth enough to add this API to SourceManager.

53

These should not end with _

clang-tidy/readability/DuplicateIncludeCheck.cpp
28

I thought it should have been easy to say "get me the source range that spans the line containing this #include directive", but I didn't find anything easier that worked properly for my test cases. I'm in the process of refactoring this check to share some common code with llvm-include-order and that might eliminate the need for this, but I haven't gotten that far yet.

sbenza added inline comments.Mar 19 2015, 10:34 AM
clang-tidy/readability/DuplicateIncludeCheck.cpp
28

I'm ok with the way it is.
It is only being used when making a warning so it should not cost anything on clean code.
It was just a general comment.
Maybe adding a comment here would be good in case someone comes looking for a speedup opportunity.

alexfh added a comment.Nov 5 2015, 1:28 PM

What's the state of this patch?

I need to update from review comments and upload a new diff.

alexfh requested changes to this revision.Mar 18 2016, 3:55 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Mar 18 2016, 3:55 AM
clang-tidy/readability/DuplicateIncludeCheck.cpp
62

If I remove the isInMainFile and replace it with a check to isInSystemHeader, then all my tests fail. I will have to investigate this further.

Include What You Use detect duplicated include directives. I think will be good idea to use it instead of Clang-tidy for much deeper analysis.

Review takes too long to make forward progress; abandoning.