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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet 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());
     }
----------------
VitaNuo wrote:
> 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?
> If I understand correctly, you should be able to extract IncludedFile from IncludedHeader->physical() and then you don't need the extra parameter.

Not really, if included file is recognized as a system include we won't have a physical file entry.

> 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?

It still does (and I am adding tests for those). All of them were still recorded based on their line number into this structure, now instead we're recording them through their fileentry (even if their `include_cleaner::Header` representation is non-physical, we're talking about an include here, so it must have a logical file it resolves to, when it exists). So this still implies some behavior changes of course:
- We no longer respect IWYU pragmas assoaciated with non-existent/virtual files, but all of the users already suppress unused diagnostics for unresolved files and moreover the new `shouldKeep` API forces the caller to pass in a file-entry. Hence going forward we give explicit info to the callers that they should deal with unresolved headers on their own way.
- If there are multiple include directives resolving to the same physical file, we'll bind their fate. We already assume in other places that a file will be included at most once, so I don't think that's also a worth concern.

WDYT?


================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:262
+      Out->ShouldKeep.insert(IncludedFile->getUniqueID());
+    }
 
----------------
VitaNuo wrote:
> nit: remove braces.
because the condition is spanning multiple lines, i'd rather keep it


================
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());
 }
----------------
VitaNuo wrote:
> Consider inlining this function.
same concerns about inlining as in the previous patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156123