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
