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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 3 inline comments as done.
sammccall added inline comments.


================
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:
> 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)
The comment is in the header file, extended it to be more explicit about avoiding 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.
----------------
sammccall wrote:
> kadircet wrote:
> > 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)
> The comment is in the header file, extended it to be more explicit about avoiding StringRef
> it makes logic in callers less complicated

I think this encourages a risky pattern for very minimal gain.
We've seen comment handlers are performance-sensitive (each strlen of the source file isn't **that** expensive!).
If we bail out explicitly early in the overwhelmingly common no-pragma case, then the remaining code is not critical.
If we continue on and "naturally" fail to match any pragma case, I don't see performance problems today, however we need to be extra-careful whenever we change the code. I'd much prefer to have to read/write an obvious `if (empty) return` than to think about performance.

If we're going to prefer an early bailout, StringRef isn't really simpler than Optional<StringRef>, and it pushes callers to handle this the right way.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135314/new/

https://reviews.llvm.org/D135314