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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
