Page MenuHomePhabricator

[clangd] Provide a way to disable external index
ClosedPublic

Authored by kadircet on Apr 8 2021, 6:55 AM.

Details

Summary

Users can reset any external index set by previous fragments by
putting a None for the index location, e.g:

Index:
  External:
    Server: None

Note that this will reset both File and Server, and will be a no-op when user
puts Server: None but they have a clangd without remote-index support.

Diff Detail

Event Timeline

kadircet created this revision.Apr 8 2021, 6:55 AM
kadircet requested review of this revision.Apr 8 2021, 6:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2021, 6:55 AM

Note that this will reset both File and Server, and will be a no-op when user
puts Server: None but they have a clangd without remote-index support.

This feels like a nasty consequence, but I am not sure if it is important enough to justify some other syntax like:

Index:
  External: None

but happy to go this way if you think that's more user friendly.

Using server: None for this seems a bit weird and irregular (why not file: none?)

What about no key inside External?
Like just empty External: or maybe it has to be External: {} (I don't know YAML well...)

kadircet updated this revision to Diff 336770.Apr 12 2021, 1:11 AM
  • As discussed offline, support External: None instead.
sammccall accepted this revision.Apr 12 2021, 4:08 AM
sammccall added inline comments.
clang-tools-extra/clangd/Config.h
86

Hmm, we could now make this non-optional, slight yak-shave, up to you.

This revision is now accepted and ready to land.Apr 12 2021, 4:08 AM
kadircet added inline comments.Apr 12 2021, 7:43 AM
clang-tools-extra/clangd/Config.h
86

i'll do that in a follow-up, since we want this to be cherry-picked i'd rather keep the change to a minimum.

This revision was automatically updated to reflect the committed changes.

This change breaks llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp:570:11: error: enumeration value 'None' not handled in switch [-Werror,-Wswitch]. I would fix forward, but the solution is not obvious to me.

Reverting shortly.

I am very new here so I will very likely get this wrong. From what I can tell around the warning, the case missing in ClangdMain.cpp std::unique_ptr<SymbolIndex> loadExternalIndex(const Config::ExternalIndexSpec &External, AsyncTaskRunner *Tasks) shouldn't be getting hit with a None in the External.Kind field. Rather it should return an error.

To prevent the error from being thrown, at the ProjectAwareIndex.cpp SymbolIndex *ProjectAwareIndex::getIndex() const call, it should return a nullptr when the External.Field is None.

There would also need to be a test case to ensure that works as intended.

thanks for the insights @crr0004 ! i've actually relanded this in rGecc6965b2342397a215b00e8e476d8d37d080322 with the fix to clangdmain. since we always reset the externalindexspec when the kind is None in ConfigCompile.cpp:370, loadExternalIndex should never be called with a spec of None kind. Hence we are just falling to llvm_unreachable.