sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:193
+ } else {
+ elog("Code completion header path manipulation failed {0}",
+ HeaderFile.takeError());
----------------
adamcz wrote:
> sammccall wrote:
> > I think this is too noisy.
> >
> > We can hit this in "normal" cases of certain projects.
> > For better or worse, we don't currently filter out index results where we
> > know the responsible header and can't include it (because it's not under
> > any include path entry). In these cases toHeaderFile() returns an error.
> >
> > Since this can happen in the normal case for *each* completion candidate
> > (default 100), and completion requests are very frequent, this could
> > dominate log output in affected projects.
> >
> > It *might* be OK at vlog? I get the desire to not silence errors here. I
> > think the question is what are the error cases we're *trying* to call out
> > loudly. Maybe we can separate out the "shouldn't happen" vs the "fairly
> > expected" cases.
> > (Even then there's the prospect that this is either going to not fire or
> > fire dozens of times, which is a little sad)
> Originally I didn't even log here. We do this expansion at other places and
> they may log already, I don't think it's critical to log here. Kadir pointed
> out error needs to be handled or it will assert()-fail (that one place
> returns Expected rather than Optional, I missed that), which is the
> motivation for this change.
>
> I made it vlog, but if you think it's too spammy I can remove it and just
> ignore the error (by this time explicitly, so it doesn't crash). I don't
> think we'll see these errors often - this only happens when we failed to
> parse the URI or recognize the scheme, which basically can only happen with
> remote index and some kind of version skew or corrupt data, so I'm fine with
> vlog() I think.
Oh wow, I totally missed that the Expected was unhandled before!
Yeah let's go with vlog.
(If you like, downgrading the log() to vlog() for the callsite in
CodeComplete.cpp seems like a good idea)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93220/new/
https://reviews.llvm.org/D93220
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
