This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Drop optional on ExternalIndexSpec
ClosedPublic

Authored by kadircet on Apr 12 2021, 8:21 AM.

Diff Detail

Event Timeline

kadircet created this revision.Apr 12 2021, 8:21 AM
kadircet requested review of this revision.Apr 12 2021, 8:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2021, 8:21 AM
sammccall accepted this revision.Jun 2 2021, 11:18 AM

Sorry about delay.

clang-tools-extra/clangd/tool/ClangdMain.cpp
555

I think you hit llvm_unreachable here - is this an invariant enforced by the caller, or did you mean to return nullptr?

This revision is now accepted and ready to land.Jun 2 2021, 11:18 AM
kadircet updated this revision to Diff 349378.Jun 2 2021, 2:22 PM
kadircet marked an inline comment as done.

Add comment to IndexFactory about pre-condition of spec never being none.

clang-tools-extra/clangd/tool/ClangdMain.cpp
555

it is enforced by the caller (i.e. ProjectAwareIndex::getIndex) hence fallback to unreachable was deliberate.

It was more explicit before as Generator takes a reference, also semantically it feels wrong for generator to be called with a None spec.

Even though we are lucky and can indicate the error by returning null in this case, I'd rather keep it as an assertion failure, similar to before. Gonna update the documentation on the IndexFactory to mention that spec is never None though.

This revision was landed with ongoing or failed builds.Jun 2 2021, 2:27 PM
This revision was automatically updated to reflect the committed changes.