sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Great! A few more nits
================
Comment at: clang-tools-extra/clangd/Headers.cpp:151
+ // will know that the next inclusion is behind the IWYU pragma.
+ if (!Text.consume_front(IWYUPragmaExport) &&
+ !Text.consume_front(IWYUPragmaKeep))
----------------
you never use Text again, so consume_front is unneccesary and confusing here,
we can just use starts_with
================
Comment at: clang-tools-extra/clangd/Headers.cpp:151
+ // will know that the next inclusion is behind the IWYU pragma.
+ if (!Text.consume_front(IWYUPragmaExport) &&
+ !Text.consume_front(IWYUPragmaKeep))
----------------
sammccall wrote:
> you never use Text again, so consume_front is unneccesary and confusing here,
> we can just use starts_with
add a FIXME that begin_export is not handled here (now that the other branch
supports it)
================
Comment at: clang-tools-extra/clangd/Headers.h:148
+ bool hasIWYUPragmas(HeaderID ID) const {
+ return HasIWYUPragmas.contains(ID);
----------------
hasIWYUExport?
(if the file contains non-export pragmas we need to return false)
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:274
+ // FIXME: Ignore the headers with IWYU export pragmas for now, remove this
+ // check when we have actual support for export pragmas.
+ if (AST.getIncludeStructure().hasIWYUPragmas(HID))
----------------
actual support for export pragmas => more precise tracking of exported headers?
(current wording doesn't really say what's missing)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125468/new/
https://reviews.llvm.org/D125468
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
