This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Extract options struct for ClangdLSPServer. NFC
ClosedPublic

Authored by sammccall on Sep 29 2020, 1:38 AM.

Details

Summary

In preparation for making moving TweakFilter from ClangdServer::Options to
a ClangdLSPServer option, and letting it vary per-request.
(In order to implement CodeActionParams.only)

Also a general overdue cleanup.

Diff Detail

Event Timeline

sammccall created this revision.Sep 29 2020, 1:38 AM
sammccall requested review of this revision.Sep 29 2020, 1:38 AM
sammccall added inline comments.Sep 29 2020, 1:39 AM
clang-tools-extra/clangd/ClangdLSPServer.h
39–54

One question here is whether the inheritance is too tricky.

The nice thing about it is that Opts.CrossFileRename = true is pretty natural at the configuration site, where Opts.ServerOpts.CrossFileRename isn't.

sammccall updated this revision to Diff 294907.Sep 29 2020, 1:42 AM

Drop redundant 'opts' from member names of per-feature opts structs

kbobyrev accepted this revision.Sep 30 2020, 12:51 AM

LGTM, thanks!

clang-tools-extra/clangd/ClangdLSPServer.h
56

nit: Maybe move this to the tests? I'm not sure if this would be useful in the public interface.

This revision is now accepted and ready to land.Sep 30 2020, 12:51 AM
sammccall marked an inline comment as done.Sep 30 2020, 1:10 AM
This revision was landed with ongoing or failed builds.Sep 30 2020, 1:10 AM
This revision was automatically updated to reflect the committed changes.
ro added a subscriber: ro.Sep 30 2020, 2:12 AM

This patch broke builds with gcc, e.g. Builder clang-solaris11-amd64 Build #6724:

[23/122] Building CXX object tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o
FAILED: tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o 
/usr/gcc/9/bin/c++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/tools/extra/clangd/tool -I/opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/clang-tools-extra/clangd/tool -Itools/clang/tools/extra/clangd/../clang-tidy -I/opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/clang/include -Itools/clang/include -Iinclude -I/opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/llvm/include -I/opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/llvm/include/llvm/Support/Solaris -I/opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/clang-tools-extra/clangd/tool/.. -Itools/clang/tools/extra/clangd/tool/.. -Itools/clang/tools/extra/clangd -fPIC -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3     -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o -MF tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o.d -o tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o -c /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/clang-tools-extra/clangd/tool/ClangdMain.cpp
In file included from /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/clang-tools-extra/clangd/tool/ClangdMain.cpp:9:
/opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/clang-tools-extra/clangd/tool/../ClangdLSPServer.h:47:36: error: declaration of ‘llvm::Optional<clang::clangd::OffsetEncoding> clang::clangd::ClangdLSPServer::Options::OffsetEncoding’ changes meaning of ‘OffsetEncoding’ [-fpermissive]
   47 |     llvm::Optional<OffsetEncoding> OffsetEncoding;
      |                                    ^~~~~~~~~~~~~~
In file included from /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/clang-tools-extra/clangd/tool/../Headers.h:12,
                 from /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/clang-tools-extra/clangd/tool/../CodeComplete.h:19,
                 from /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/clang-tools-extra/clangd/tool/../ClangdServer.h:13,
                 from /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/clang-tools-extra/clangd/tool/../ClangdLSPServer.h:12,
                 from /opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/clang-tools-extra/clangd/tool/ClangdMain.cpp:9:
/opt/llvm-buildbot/home/solaris11-amd64/clang-solaris11-amd64/llvm/clang-tools-extra/clangd/tool/../Protocol.h:370:12: note: ‘OffsetEncoding’ declared here as ‘enum class clang::clangd::OffsetEncoding’
  370 | enum class OffsetEncoding {
      |            ^~~~~~~~~~~~~~