This is an archive of the discontinued LLVM Phabricator instance.

[include-cleaner] Capture private headers in PragmaIncludes.
ClosedPublic

Authored by VitaNuo on Nov 24 2022, 8:56 AM.

Details

Summary

Save file IDs of IWYU private headers and report them as private.

Diff Detail

Event Timeline

VitaNuo created this revision.Nov 24 2022, 8:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2022, 8:56 AM
VitaNuo requested review of this revision.Nov 24 2022, 8:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 24 2022, 8:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
VitaNuo updated this revision to Diff 477874.Nov 25 2022, 1:04 AM

Improve the comment.

VitaNuo updated this revision to Diff 477876.Nov 25 2022, 1:08 AM

Fix formatting.

hokein added inline comments.Nov 25 2022, 2:08 AM
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
76

I think this function should return true if the given file is marked with the IWYU private mapping pragma, because conceptually these file are also private.

95

It should collect files with mapping IWYU private pragmas.

and I think the last sentence can be removed (the intention of using UniqueID is clear from other comments).

clang-tools-extra/include-cleaner/lib/Record.cpp
269

I'd suggest moving this to the above line 236 where we handle the private mapping.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
350

no extra ; token needed for the #include.

371

add EXPECT_TRUE(PI.isPrivate(PrivateFE.get())).

kadircet added inline comments.Nov 25 2022, 2:35 AM
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
97

i'd actually merge this with the previous map, and store empty verbatim spellings. semantics of getPublic is to return empty path when there are no mappings anyway.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
361

nit: no need for the declaration of Private here (nor in private.h). it's actually introducing a double definition error into TU now.

VitaNuo updated this revision to Diff 477940.Nov 25 2022, 6:14 AM

Address review comments.

VitaNuo marked 4 inline comments as done.Nov 25 2022, 6:18 AM

The formatting has introduced a bit of unrelated diff. It's not much, so I hope that's fine.

clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
95

I've followed Kadir's suggestion below now and merged the private pragmas with the IWYUPublic map.

97

Sounds reasonable. I wasn't sure which of the options to pick in the first place.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
361

Out of curiosity: what's TU?

VitaNuo updated this revision to Diff 477944.Nov 25 2022, 6:30 AM
VitaNuo marked an inline comment as done.

Fix build.

VitaNuo updated this revision to Diff 477946.Nov 25 2022, 6:32 AM

Fix comment.

kadircet added inline comments.Nov 25 2022, 6:34 AM
clang-tools-extra/include-cleaner/lib/Record.cpp
236

nit: you can drop the braces here. LLVM convention is to not have braces when a condition/loop body has only a single statement.

239

nit: you can also rewrite this as:

StringRef PublicHeader;
if (Pragma->consume_front(", include ")) {
  PublicHeader = save(Pragma->startswith("<") || Pragma->startswith("\"")
                      ? (*Pragma)
                      : ("\"" + *Pragma + "\"").str());
}
Out->IWYUPublic.insert({FE->getLastRef().getUniqueID(), PublicHeader});
clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
361

it means translation unit, it's the source file provided to the compiler after all preprocessor directives are processed.

VitaNuo updated this revision to Diff 477954.Nov 25 2022, 7:04 AM

Small refactoring.

VitaNuo marked an inline comment as done.Nov 25 2022, 7:07 AM
VitaNuo added inline comments.
clang-tools-extra/include-cleaner/lib/Record.cpp
236

Ah ok, thank you. It's a bit funny, my whole life until now I've been rigorously taught to never do this ;-)

239

Yeah this is good, thanks.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
361

Thanks.

VitaNuo updated this revision to Diff 477956.Nov 25 2022, 7:09 AM
VitaNuo marked 2 inline comments as done.

Fix build.

VitaNuo updated this revision to Diff 477958.Nov 25 2022, 7:11 AM

Fix history.

VitaNuo updated this revision to Diff 477960.Nov 25 2022, 7:21 AM

Apply the local changes.

hokein accepted this revision.Nov 28 2022, 12:34 AM

Thanks, looks good from my side, a few nits on the test.

clang-tools-extra/include-cleaner/unittests/RecordTest.cpp
364

nit: IIUC, we're using a blank line to separate the tests per FileEntry, this EXPECT_EQ statement should belong to the above group (which is PrivateFE), right?

367–371

nit: add EXPECT_FALSE(PI.isPrivate(PublicFE.get()));

This revision is now accepted and ready to land.Nov 28 2022, 12:34 AM
VitaNuo updated this revision to Diff 478145.Nov 28 2022, 12:44 AM
VitaNuo marked an inline comment as done.

Address the remaining comments.

VitaNuo marked 2 inline comments as done.Nov 28 2022, 12:44 AM
This revision was landed with ongoing or failed builds.Nov 28 2022, 1:02 AM
This revision was automatically updated to reflect the committed changes.