[PATCH] D156123: [include-cleaner] Unify always_keep with rest of the keep logic

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:248
+      if (Top.SeenAtFile == SM.getMainFileID() && IncludedFile)
+        Out->ShouldKeep.insert(IncludedFile->getUniqueID());
     }
----------------
If I understand correctly, you should be able to extract `IncludedFile` from `IncludedHeader->physical()` and then you don't need the extra parameter.

Also, technically this and the below code used to work for standard and verbatim headers before this change. Now it probably won't, since there will be no corresponding `IncludedFile`. Is this actually something to worry about?


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:262
+      Out->ShouldKeep.insert(IncludedFile->getUniqueID());
+    }
 
----------------
nit: remove braces.


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:419
 bool PragmaIncludes::shouldKeep(const FileEntry *FE) const {
-  return AlwaysKeep.contains(FE->getUniqueID());
+  return ShouldKeep.contains(FE->getUniqueID());
 }
----------------
Consider inlining this function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156123