[PATCH] D19816: [find-all-symbols] Add IWYU private pragma support.

[Kim Gräsman via cfe-commits]

kimgr added a subscriber: kimgr.
kimgr added a comment.

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 at 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 at lists.llvm.org

>  http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits



Repository:
  rL LLVM

http://reviews.llvm.org/D19816