[PATCH] D93220: [clangd] Add error handling (elog) in code completion.

[Sam McCall via Phabricator via cfe-commits]

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