[PATCH] D156122: [include-cleaner] Introduce support for always_keep pragma

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:63
+  bool shouldKeep(unsigned HashLineNumber) const;
+  bool shouldKeep(const FileEntry *FE) const;
 
----------------
VitaNuo wrote:
> Why wouldn't you actually inline the implementation for both these functions? The implementations seem trivial enough. 
most of the binaries are build with LTO nowadays, so inlining code into headers doesn't really gain much from performance perspective. moreover, these are not really hot functions, they're invoked once per `#include` directive inside the main file, at the end of analysis.

OTOH, when they're inlined and there needs to be a change to these function definitions, it requires all the translation units depending on this header to be recompiled. Hence I think having them in the cpp file is a not benefit for development cycles.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:37
 #include <utility>
+#include <vector>
 
----------------
VitaNuo wrote:
> I don't think you've added a dependency on all these headers in this patch. Nice if you've actually fixed the missing include violations all over the file, but please double-check that there are no debugging etc. artefacts left.
yes these are all fixes generated by include-cleaner, I don't have any unused include findings.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:272
 
+    FileID CommentFID = SM.getFileID(Range.getBegin());
+    auto *FE = SM.getFileEntryForID(CommentFID);
----------------
VitaNuo wrote:
> You might want to inline this call, it seems to have a single usage right below.
do you mean initializer for `CommentFID`? if so `CommentFID` is actually used in more places below


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:288
+    if (Pragma->consume_front("always_keep")) {
+      Out->AlwaysKeep.insert(FE->getUniqueID());
+      return false;
----------------
VitaNuo wrote:
> I believe you need to check `FE` for `nullptr`, similar to private pragma handling above. 
> Also, the code above does `FE->getLastRef().getUniqueID()`, is there any difference to `FE->getUniqueID()`? I wouldn't expect so, but then you could maybe unify this one with the above, this way or the other.
thanks missed that. unified all the usages for `getUniqueID`, they all should be the same.

you're absolutely right about checking for `FE`, in practice it's almost never null as it's the file in which we've found the IWYU pragma. But SourceManager actually allows working with virtual files, in which case there might not be a phyiscal file entry.
code in rest of this logic was also relying on `FE` being non-null without checking for it. Moved code around a little bit and used filename inside the SourceManager instead to make sure export/keep logic can work with virtual files too.

for now not adding support to always_keep and private pragmas for virtual files, as it requires deeper changes in the way we store uniqueids (we can no longer depend on them) and they're not common in practice (usually those virtual files are provided as predefines from the compiler and they likely won't contain IWYU pragmas).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156122/new/

https://reviews.llvm.org/D156122