Page MenuHomePhabricator
Differential D144828

[clang-tidy] Add misc-header-include-cycle check
Needs ReviewPublic
Actions

Authored by PiotrZSL on Sun, Feb 26, 8:31 AM.

Details

Reviewers
njames93
aaron.ballman
carlosgalvezp
LegalizeAdulthood
Summary

Check detects cyclic #include dependencies between user-defined headers.

Diff Detail

Event Timeline

PiotrZSL created this revision.Sun, Feb 26, 8:31 AM
Herald added a reviewer: njames93. · View Herald TranscriptSun, Feb 26, 8:31 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: carlosgalvezp, xazax.hun. · View Herald Transcript
PiotrZSL updated this revision to Diff 500591.Sun, Feb 26, 8:32 AM

Typo fix.

Harbormaster completed remote builds in B216082: Diff 500591.Sun, Feb 26, 8:55 AM
Eugene.Zelenko added reviewers: aaron.ballman, carlosgalvezp.Mon, Feb 27, 7:11 AM
Eugene.Zelenko added a project: Restricted Project.
PiotrZSL updated this revision to Diff 501040.Mon, Feb 27, 11:37 PM

Fixes

PiotrZSL added a comment.Mon, Feb 27, 11:38 PM

Findings on llvm:

clang-tools-extra/unittests/clang-doc/ClangDocTest.h:12:10: warning: direct self-inclusion of header file 'ClangDocTest.h' [misc-header-include-cycle]
libc/src/__support/FPUtil/x86_64/LongDoubleBits.h:21:10: warning: circular header file dependency detected while including 'FPBits.h', please check the include path: 'FPBits.h' -> 'LongDoubleBits.h' -> 'FPBits.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBAddress.h:13:10: warning: circular header file dependency detected while including 'SBModule.h', please check the include path: 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBBlock.h' -> 'SBTarget.h' -> 'SBAddress.h' -> 'SBModule.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBBlock.h:14:10: warning: circular header file dependency detected while including 'SBTarget.h', please check the include path: 'SBTarget.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBBlock.h' -> 'SBTarget.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBFunction.h:12:10: warning: circular header file dependency detected while including 'SBAddress.h', please check the include path: 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBFunction.h' -> 'SBAddress.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBLineEntry.h:12:10: warning: circular header file dependency detected while including 'SBAddress.h', please check the include path: 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBLineEntry.h' -> 'SBAddress.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBModule.h:15:10: warning: circular header file dependency detected while including 'SBSymbolContext.h', please check the include path: 'SBSymbolContext.h' -> 'SBBlock.h' -> 'SBTarget.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbol.h:12:10: warning: circular header file dependency detected while including 'SBAddress.h', please check the include path: 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBSymbol.h' -> 'SBAddress.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbol.h:15:10: warning: circular header file dependency detected while including 'SBTarget.h', please check the include path: 'SBTarget.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBSymbol.h' -> 'SBTarget.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbolContext.h:12:10: warning: circular header file dependency detected while including 'SBBlock.h', please check the include path: 'SBBlock.h' -> 'SBTarget.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBBlock.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbolContext.h:15:10: warning: circular header file dependency detected while including 'SBFunction.h', please check the include path: 'SBFunction.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBFunction.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbolContext.h:16:10: warning: circular header file dependency detected while including 'SBLineEntry.h', please check the include path: 'SBLineEntry.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBLineEntry.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbolContext.h:17:10: warning: circular header file dependency detected while including 'SBModule.h', please check the include path: 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBModule.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbolContext.h:18:10: warning: circular header file dependency detected while including 'SBSymbol.h', please check the include path: 'SBSymbol.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBSymbol.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbolContextList.h:13:10: warning: circular header file dependency detected while including 'SBSymbolContext.h', please check the include path: 'SBSymbolContext.h' -> 'SBBlock.h' -> 'SBTarget.h' -> 'SBSymbolContextList.h' -> 'SBSymbolContext.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBTarget.h:12:10: warning: circular header file dependency detected while including 'SBAddress.h', please check the include path: 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBBlock.h' -> 'SBTarget.h' -> 'SBAddress.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBTarget.h:20:10: warning: circular header file dependency detected while including 'SBSymbolContextList.h', please check the include path: 'SBSymbolContextList.h' -> 'SBSymbolContext.h' -> 'SBBlock.h' -> 'SBTarget.h' -> 'SBSymbolContextList.h' [misc-header-include-cycle]
lldb/include/lldb/lldb-private-types.h:14:10: warning: circular header file dependency detected while including 'lldb-private.h', please check the include path: 'lldb-private.h' -> 'lldb-private-types.h' -> 'lldb-private.h' [misc-header-include-cycle]
lldb/include/lldb/lldb-private.h:16:10: warning: circular header file dependency detected while including 'lldb-private-types.h', please check the include path: 'lldb-private-types.h' -> 'lldb-private.h' -> 'lldb-private-types.h' [misc-header-include-cycle]
lldb/source/Plugins/Process/scripted/ScriptedProcess.h:17:10: warning: circular header file dependency detected while including 'ScriptedThread.h', please check the include path: 'ScriptedThread.h' -> 'ScriptedProcess.h' -> 'ScriptedThread.h' [misc-header-include-cycle]
lldb/source/Plugins/Process/scripted/ScriptedThread.h:14:10: warning: circular header file dependency detected while including 'ScriptedProcess.h', please check the include path: 'ScriptedProcess.h' -> 'ScriptedThread.h' -> 'ScriptedProcess.h' [misc-header-include-cycle]
third-party/unittest/googletest/include/gtest/gtest_pred_impl.h:43:10: warning: circular header file dependency detected while including 'gtest.h', please check the include path: 'gtest.h' -> 'gtest_pred_impl.h' -> 'gtest.h' [misc-header-include-cycle]
Harbormaster completed remote builds in B216403: Diff 501040.Mon, Feb 27, 11:59 PM
PiotrZSL published this revision for review.Tue, Feb 28, 12:04 AM

Ready for review

Herald added a subscriber: cfe-commits. · View Herald TranscriptTue, Feb 28, 12:04 AM
njames93 added a comment.Tue, Feb 28, 8:17 AM

This is a great check that I've been meaning to work on for ages but never had time.
It mostly looks good but there are a few issues
I feel there is another issue that this can be very spammy with diagnostics(though clang-tidy is likely suppressing the duplicated ones).
But maybe when we emit a warning for a specified include, we could put it in a set to not warn on that include again

clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
107–111

How does this handle this case where an include includes a file that self includes itself.

// Foo.hpp
#pragma once
#include "Foo.hpp"

// Bar.hpp
#pragma once
#include Foo.hpp

// Main.cpp
#include "Bar.hpp"

Where will the warning be emitted, it should appear in Foo.hpp but the logic here says it won't and would just be reported as a circular dependency below.
Can you make sure the warning is emitted in Foo.hpp and add test cases that test for this, The current tests just check include a file that self references itself with no intermediate files.

113–123

Rather than just printing out the include path, can you emit each include location as a note.
OtherInclude.h:5:1: note: 'ThisInclude.h' included here
You can pass DiagnosticLevel::Note as the 3rd parameter to Check::diag to emit a note. Just have to make sure you emit the note after you emit the warning.

Notes have better support for front end diagnostics parsing.

135

1024 entries is pushing the boat out for a small vector.

clang-tools-extra/docs/clang-tidy/checks/misc/header-include-cycle.rst
64

This tag line is just unnecessary

PiotrZSL added inline comments.Tue, Feb 28, 9:53 AM
clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
107–111

in theory it should raise an issue for self-include here..
and issue should be raised in Foo.hpp, can add check for this...

113–123

yes I wanted to do that, but at the point of FileChanged, where I fill up stack, i don't have an proper info where such file wre included.
It's possible, would need just to somehow connect info from InclusionDirective, assuming that when we got include, we go in such file, then probably keeping last processed include should be sufficient...

135

Thats just 4KB of memory pre-allocated to avoid re-allocations. Small is "muten" here.

clang-tools-extra/docs/clang-tidy/checks/misc/header-include-cycle.rst
64

maybe it is unnecessary, but I just wanted to put some advertisement slogan for a check ...
Doesn't need to be bold... But it's is catchy...

PiotrZSL planned changes to this revision.Tue, Feb 28, 12:57 PM
PiotrZSL marked 4 inline comments as done.Sun, Mar 5, 12:41 PM

"But maybe when we emit a warning for a specified include, we could put it in a set to not warn on that include again"
I don't think so, in such case we hide include paths, and most of the time header guards should help here in reduction of cyclic dependences.

PiotrZSL updated this revision to Diff 502464.Sun, Mar 5, 12:44 PM

Added support for --include
Added support for self-include of source file
Added more tests
Reworked include stack.

PiotrZSL added a comment.Sun, Mar 5, 12:45 PM

TODO: Add option: ExcludeHeadersRegexp

PiotrZSL planned changes to this revision.Sun, Mar 5, 1:15 PM

Option need to be added

Harbormaster completed remote builds in B217446: Diff 502464.Sun, Mar 5, 1:25 PM
PiotrZSL updated this revision to Diff 503122.Tue, Mar 7, 12:04 PM

Ready for review, added new check option.

Harbormaster completed remote builds in B217927: Diff 503122.Tue, Mar 7, 12:24 PM
PiotrZSL updated this revision to Diff 503144.Tue, Mar 7, 1:18 PM

Rebase due to failed clangd tests on windows

Harbormaster completed remote builds in B217941: Diff 503144.Tue, Mar 7, 1:41 PM
PiotrZSL updated this revision to Diff 503251.Wed, Mar 8, 12:18 AM

Rebase due to broken baseline

Harbormaster completed remote builds in B218014: Diff 503251.Wed, Mar 8, 12:45 AM
PiotrZSL added a reviewer: LegalizeAdulthood.Tue, Mar 14, 12:35 PM

Revision Contents
Changeset List

Diff 503251

View Options

clang-tools-extra/clang-tidy/misc/CMakeLists.txt

Loading...
View Options

clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.h

Loading...
View Options

clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp

Loading...
View Options

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

Loading...
View Options

clang-tools-extra/docs/ReleaseNotes.rst

Loading...
View Options

clang-tools-extra/docs/clang-tidy/checks/list.rst

Loading...
View Options

clang-tools-extra/docs/clang-tidy/checks/misc/header-include-cycle.rst

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.first-d.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.first.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.fourth-d.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.fourth.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.second-d.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.second.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.self-d.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.self-e.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.self-i.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.self-n.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.self-o.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.self.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.third-d.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/header-include-cycle.third.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/system/header-include-cycle.first-s.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/system/header-include-cycle.fourth-s.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/system/header-include-cycle.second-s.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/system/header-include-cycle.self-s.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/system/header-include-cycle.third-s.hpp

Loading...
View Options

clang-tools-extra/test/clang-tidy/checkers/misc/header-include-cycle.cpp

Loading...