[PATCH] D56370: [clangd] Add support for type hierarchy (super types only for now)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added a comment.

In D56370#1424177 <https://reviews.llvm.org/D56370#1424177>, @nridge wrote:

> Fix a (somewhat amusing) typo where I wrote '0' instead of 'O' in a fromJSON() implementation
>
> (Does the fact that this didn't cause any test failures suggest that the fromJSON() functions aren't exercised by any tests?)


Unfortunately we usually test LSP layer with lit tests, and since we don't have any lit tests for this use-case it wasn't caught. Could you add a lit test for overall use case? You can see examples inside clang-tools-extra/test/clangd/ folder.

In D56370#1424180 <https://reviews.llvm.org/D56370#1424180>, @nridge wrote:

> Unfortunately, there is a further problem: the Theia client-side implementation of type hierarchy has recently merged, and their code has changed so that they do require `typeHierarchy/resolve` to be supported. They ask for 1 level in the initial request, ignore any extra levels the server might send, and ask for extra levels using `typeHierarchy/resolve` instead.
>
> What should we do here -- seek to change Theia, or implement `typeHierachy/resolve` for supertypes after all?


Looking at https://github.com/theia-ide/theia/pull/3802#issuecomment-455992523 inside theia's implementation of this feature, I believe these are all subject to change.
So let's leave it as it is, even if it would mean you will be able to get only one level of parents knowledge in theia for now. I believe it should be addressed in theia's implementation and proposal itself(which is acutally addressed by sam, https://github.com/Microsoft/vscode-languageserver-node/pull/426/files#r255416980)



================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365
                               Callback<tooling::Replacements> CB) {
-  auto Action = [Sel](decltype(CB) CB, std::string File,
-                            std::string TweakID,
-                            Expected<InputsAndAST> InpAST) {
+  auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+                      Expected<InputsAndAST> InpAST) {
----------------
nridge wrote:
> kadircet wrote:
> > nridge wrote:
> > > kadircet wrote:
> > > > Can you revert this change?
> > > I tried, but clang-format automatically makes the change again.
> > > 
> > > Is there some particular clang-format configuration I should apply, so it produces the same results? I don't currently have any configuration, i.e. I think it's just reading the .clang-format file in the monorepo root.
> > That's what we use as well(and your formatted version is correct), but unfortunately sometimes people send out changes with wrong formatting and they are hard to notice during review. Generally we try not to touch those lines in irrelevant patches to keep blame clean.
> > 
> > If you are running clang-format directly you can instead try clang-format-diff to format only changed lines. But not that crucial.
> I'm not running clang-format directly, VSCode's clang-format extension is doing so automatically upon every file-save. I have not found an option to configure it to format only changed lines.
> 
> Would it help if I moved the formatting corrections to their own patch (and made this patch depend on that)?
it is not that important, nvm.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56370/new/

https://reviews.llvm.org/D56370