kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
wow, that's an interesting finding. thanks!
================
Comment at: clang-tools-extra/clangd/Headers.cpp:25
-const char IWYUPragmaKeep[] = "// IWYU pragma: keep";
-const char IWYUPragmaExport[] = "// IWYU pragma: export";
-const char IWYUPragmaBeginExports[] = "// IWYU pragma: begin_exports";
+llvm::Optional<StringRef> parseIWYUPragma(const char *Text) {
+ // This gets called for every comment seen in the preamble, so it's quite
hot.
----------------
i think this also deserves a comment to make sure people won't refactor it in
the future to take in a stringref.
================
Comment at: clang-tools-extra/clangd/Headers.cpp:25
-const char IWYUPragmaKeep[] = "// IWYU pragma: keep";
-const char IWYUPragmaExport[] = "// IWYU pragma: export";
-const char IWYUPragmaBeginExports[] = "// IWYU pragma: begin_exports";
+llvm::Optional<StringRef> parseIWYUPragma(const char *Text) {
+ // This gets called for every comment seen in the preamble, so it's quite
hot.
----------------
kadircet wrote:
> i think this also deserves a comment to make sure people won't refactor it in
> the future to take in a stringref.
i'd actually return an empty stringref, instead of `None`. it makes logic in
callers less complicated and I don't think we convey any different signal
between None vs empty right now (at least a signal that can be used by the
callers)
================
Comment at: clang-tools-extra/clangd/Headers.cpp:27
+ // This gets called for every comment seen in the preamble, so it's quite
hot.
+ constexpr char IWYUPragma[] = "// IWYU pragma: ";
+ if (strncmp(Text, IWYUPragma, strlen(IWYUPragma)))
----------------
nit: `static constexpr StringLiteral` instead (we can just use `size()`
afterwards instead of calling `strlen` a bunch.
================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:456
+ << "Only // comments supported!";
+ EXPECT_EQ(parseIWYUPragma("// IWYU pragma: keep"), llvm::None)
+ << "Sensitive to whitespace";
----------------
it might be useful to get rid of the space between `:` and `keep` as well.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135314/new/
https://reviews.llvm.org/D135314
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
