[PATCH] D156122: [include-cleaner] Introduce support for always_keep pragma
Viktoriia Bakalova via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 31 05:16:36 PDT 2023
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
More information about the cfe-commits
mailing list