This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Extract BackgroundIndex::Options struct. NFC
ClosedPublic

Authored by sammccall on Jul 4 2020, 2:54 AM.

Details

Summary

I've dropped the background context parameter, since we in practice just pass the
current context there, and we now have a different way to specify context too.
While here, clean up a couple of comments.

Diff Detail

Event Timeline

sammccall created this revision.Jul 4 2020, 2:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 4 2020, 2:54 AM
kadircet accepted this revision.Jul 4 2020, 3:32 AM
kadircet marked an inline comment as done.

thanks for doing this!

clang-tools-extra/clangd/ClangdServer.cpp
185

I think we should move storage factory into options too ?

clang-tools-extra/clangd/index/Background.h
127

🎉

This revision is now accepted and ready to land.Jul 4 2020, 3:32 AM
sammccall marked an inline comment as done.Jul 4 2020, 7:56 AM
sammccall added inline comments.
clang-tools-extra/clangd/ClangdServer.cpp
185

The storage factory isn't optional, nor does it have a good default. So it seems mechanically awkward to put it in the struct, and I'm not sure there's much benefit...

How would you see this being initialized?

kadircet marked an inline comment as done.Jul 6 2020, 3:41 AM
kadircet added inline comments.
clang-tools-extra/clangd/ClangdServer.cpp
185

I thought we were exposing NullStorage so I was planning for it to be the default for ignorant call sites. But seeing it is not exposed, and this only being used by ClangdServer (and tests), I suppose it is not that important.

This revision was landed with ongoing or failed builds.Aug 13 2020, 9:13 AM
This revision was automatically updated to reflect the committed changes.