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