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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo 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;
 
----------------
Why wouldn't you actually inline the implementation for both these functions? The implementations seem trivial enough. 


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:37
 #include <utility>
+#include <vector>
 
----------------
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.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:272
 
+    FileID CommentFID = SM.getFileID(Range.getBegin());
+    auto *FE = SM.getFileEntryForID(CommentFID);
----------------
You might want to inline this call, it seems to have a single usage right 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;
----------------
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.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:421
+  return AlwaysKeep.contains(FE->getUniqueID());
+}
+
----------------
As mentioned above, consider inlining.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156122