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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits