This is an archive of the discontinued LLVM Phabricator instance.

[find-all-symbols] Add IWYU private pragma support.
ClosedPublic

Authored by hokein on May 2 2016, 10:48 AM.

Diff Detail

Event Timeline

hokein updated this revision to Diff 55846.May 2 2016, 10:48 AM
hokein updated this revision to Diff 55847.
hokein retitled this revision from to [find-all-symbols] Add IWYU private pragma support..
hokein updated this object.

Fix a nit.

hokein set the repository for this revision to rL LLVM.
hokein added subscribers: ioeric, bkramer, cfe-commits.
klimek added inline comments.May 3 2016, 9:34 AM
include-fixer/find-all-symbols/FindAllSymbols.cpp
175–184

It seems weird to add this and just forward the interface.

include-fixer/find-all-symbols/FindAllSymbols.h
53

I think the fact that those are generated via IWYU comments are an implementation detail, and this code doesn't care. Perhaps call it HeaderMapping or HeaderRemapping. Also, it's unclear what the semantics are, so I think this needs a comment.

include-fixer/find-all-symbols/PragmaCommentHandler.h
24

I'd pull out an interface like HeaderMapCollector or ForwardingHeaderCollector, or even just pass in a std::function that collects the header. Or, just make this PragmaCommentHandler have a method to return a list of forwarding headers?
Generally, I think this couples the two classes much more than necessary.

kimgr added a subscriber: kimgr.May 4 2016, 12:06 AM

Re semantics, you may want to link to IWYU's docs at
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUPragmas.md
.

  • Kim

Den 3 maj 2016 6:34 em skrev "Manuel Klimek via cfe-commits" <
cfe-commits@lists.llvm.org>:

klimek added inline comments.

Comment at: include-fixer/find-all-symbols/FindAllSymbols.cpp:173-182
@@ -165,1 +172,12 @@

+void FindAllSymbols::addPragmaHeader(FileID ID, llvm::StringRef FilePath)
{
+ PragmaHeaderMap[ID] = FilePath;
+}
+
+llvm::Optional<std::string> FindAllSymbols::getPragmaHeader(FileID ID)
const {
+ auto It = PragmaHeaderMap.find(ID);
+ if (It == PragmaHeaderMap.end())
+ return llvm::None;
+ return It->second;
+}

+

It seems weird to add this and just forward the interface.

Comment at: include-fixer/find-all-symbols/FindAllSymbols.h:49
@@ -45,1 +48,3 @@

+ void addPragmaHeader(FileID ID, llvm::StringRef FilePath);

+

I think the fact that those are generated via IWYU comments are an
implementation detail, and this code doesn't care. Perhaps call it
HeaderMapping or HeaderRemapping. Also, it's unclear what the semantics
are, so I think this needs a comment.

Comment at: include-fixer/find-all-symbols/PragmaCommentHandler.h:23
@@ +22,3 @@
+public:
+ PragmaCommentHandler(FindAllSymbols *Matcher) : Matcher(Matcher) {}

+

I'd pull out an interface like HeaderMapCollector or
ForwardingHeaderCollector, or even just pass in a std::function that
collects the header. Or, just make this PragmaCommentHandler have a method
to return a list of forwarding headers?
Generally, I think this couples the two classes much more than necessary.

Repository:

rL LLVM

http://reviews.llvm.org/D19816


cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

hokein updated this revision to Diff 56101.May 4 2016, 12:47 AM
hokein marked 2 inline comments as done.

Address review comments.

hokein updated this revision to Diff 56120.May 4 2016, 3:45 AM

Improvements based on offline discussion.

hokein updated this revision to Diff 56121.May 4 2016, 3:49 AM

Fix a nit.

klimek added inline comments.May 4 2016, 4:58 AM
include-fixer/find-all-symbols/FindAllSymbols.h
48

I'd pass in the HeaderMap here.

include-fixer/find-all-symbols/PragmaCommentHandler.h
22

"PragmaCommentHandler handle all pragma comment"
This comment doesn't really tell me anything new.

Perhaps:
PragmaCommentHandler parses comments on include files to determine when we should include a different header from the header that directly defines a symbol.

unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
418

Any reason for the extra block here?

hokein updated this revision to Diff 56130.May 4 2016, 5:18 AM
hokein marked an inline comment as done.

Update.

unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
418

Because I want to keep consistence with other test cases here.

klimek added inline comments.May 4 2016, 5:31 AM
unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
418

Why are the other test cases using an extra block?

hokein added inline comments.May 4 2016, 7:45 AM
unittests/include-fixer/find-all-symbols/FindAllSymbolsTests.cpp
418

I will update the code after D19913 gets submitted.

bkramer added inline comments.May 6 2016, 3:07 AM
include-fixer/find-all-symbols/FindAllSymbols.h
16

No space between #includes in LLVM.

include-fixer/find-all-symbols/PragmaCommentHandler.cpp
28

Isn't this regex just a complex way to write Text.find(IWYUPragma)?

hokein updated this revision to Diff 56579.May 9 2016, 7:39 AM
hokein marked an inline comment as done.

Address comments.

hokein marked an inline comment as done.May 9 2016, 7:42 AM
hokein added inline comments.
include-fixer/find-all-symbols/PragmaCommentHandler.cpp
28

Oh, didn't notice that. Yeah, we can use the find to handle the case.for now.

hokein updated this revision to Diff 56872.May 11 2016, 3:56 AM
hokein marked an inline comment as done.

Rebase to master.

hokein marked an inline comment as done.May 11 2016, 3:57 AM
hokein updated this revision to Diff 57472.May 17 2016, 6:48 AM

Rebase.

klimek accepted this revision.May 17 2016, 7:25 AM
klimek edited edge metadata.

LG with nit to fix.

include-fixer/find-all-symbols/FindAllSymbols.h
47–50

Add a comment describing what the Collector is for.

This revision is now accepted and ready to land.May 17 2016, 7:25 AM
hokein updated this revision to Diff 57490.May 17 2016, 9:40 AM
hokein edited edge metadata.

Add comments.

hokein marked an inline comment as done.May 17 2016, 9:42 AM
hokein updated this revision to Diff 57493.May 17 2016, 9:49 AM

Rebase.

This revision was automatically updated to reflect the committed changes.