[PATCH] D135314: [clangd] Avoid scanning up to end of file on each comment!

[Kadir Cetinkaya via Phabricator via cfe-commits]

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