Page MenuHomePhabricator

Adds a json::Expr type to represent intermediate JSON expressions.
ClosedPublic

Authored by sammccall on Oct 30 2017, 2:30 PM.

Details

Summary

WIP: needs the rest of the lit tests updated.

This form can be created with a nice clang-format-friendly literal syntax,
and gets escaping right. It knows how to call unparse() on our Protocol types.
All the places where we pass around JSON internally now use this type.

Object properties are sorted (stored as std::map) and so serialization is
canonicalized, with optional prettyprinting (triggered by a -pretty flag).
This makes the lit tests much nicer to read and somewhat nicer to debug.

Compared to the current approach, it has some efficiencies like avoiding copies
of string literals used as object keys, but is probably slower overall.
I think the code/test quality benefits are worth it.

This patch doesn't attempt to do anything about JSON *parsing*.
It takes direction from the proposal in this doc[1], but is limited in scope
and visibility, for now.
I am of half a mind just to use Expr as the target of a parser, and maybe do a
little string deduplication, but not bother with clever memory allocation.
That would be simple, and fast enough for clangd...
[1] https://docs.google.com/document/d/1OEF9IauWwNuSigZzvvbjc1cVS1uGHRyGTXaoy3DjqM4/edit

+cc d0k so he can tell me not to use std::map.

Diff Detail

Repository
rL LLVM

Event Timeline

sammccall created this revision.Oct 30 2017, 2:30 PM
rwols added a subscriber: rwols.Oct 30 2017, 3:19 PM
sammccall updated this revision to Diff 121321.Nov 2 2017, 8:38 AM

Twine->StringRef for unowned strings, and remove Twine support.

(I had the owneship model all wrong, oops!)

sammccall updated this revision to Diff 121322.Nov 2 2017, 8:44 AM

Add comment about the horrible mutable/const&& hack.

ioeric edited edge metadata.Nov 2 2017, 9:00 AM

The json:: code looks great. Would it make sense to add unit tests for it?

I'm not very familiar with the clangd code, but the clangd changes also look fine to me... someone else could take another look.

clangd/JSONExpr.cpp
199 ↗(On Diff #120893)

nit: /*Radix=*/10

clangd/JSONExpr.h
39 ↗(On Diff #120893)

s/arr/ary/ ?

same below.

sammccall updated this revision to Diff 121334.Nov 2 2017, 10:09 AM

Add unittests for Expr, and remove the default constructor so that {} is a list.

sammccall edited the summary of this revision. (Show Details)Nov 2 2017, 10:10 AM
bkramer accepted this revision.Nov 2 2017, 10:18 AM

I think we can go ahead with this. Using std::map is also fine if your elements are heavy and you need sorting :)

This revision is now accepted and ready to land.Nov 2 2017, 10:18 AM
ioeric accepted this revision.Nov 2 2017, 10:19 AM

lgtm

rwols added inline comments.Nov 4 2017, 8:02 AM
clangd/JSONExpr.h
34 ↗(On Diff #121334)

Why can't unparse be a free function?

117 ↗(On Diff #121334)

Maybe use std::addressof?

sammccall updated this revision to Diff 121721.Nov 6 2017, 4:58 AM

Update all lit tests. Unfortunately the completion ones can't use prettyprinting because I can't find a way to get CHECK-DAG-like behavior for multiline blocks.

clangd/JSONExpr.h
34 ↗(On Diff #121334)

This seems like a good idea (though not without downsides - I like that T::unparse doesn't allow accidental conversions, for example).

However, the current scheme (all the existing code in Protocol) is T::unparse, and I'd rather not make this patch even bigger, so I've added a FIXME comment. This is a mechanically easy followup change if we want it.

117 ↗(On Diff #121334)

You mean like this?

new (template std::addressof(as<T>)()) T(std::forward<U>(V)...);

This certainly seems harder to read, and IIUC it guards against T having an overloaded operator&, which it never does (bool, double, StringRef, std::string, Object, Array). So I'm not sure we need the future-proofing here.

sammccall updated this revision to Diff 121735.Nov 6 2017, 7:36 AM

Merge with upstream changes and clang-format

This revision was automatically updated to reflect the committed changes.

Please CC cfe-commits when submitting clangd patches.

Please CC cfe-commits when submitting clangd patches.

Sigh, I'm really sorry - I do try not to forget this.
Is there a way we can have this happen automatically? It looks like an admin could add a Herald rule, I guess there's a reason this isn't set up.

Please feel free to leave comments, will address them.

Please CC cfe-commits when submitting clangd patches.

Sigh, I'm really sorry - I do try not to forget this.
Is there a way we can have this happen automatically? It looks like an admin could add a Herald rule, I guess there's a reason this isn't set up.

I don't think so. I think there have been some discussions about this, but I'm not sure if they led to something yet.

Please feel free to leave comments, will address them.

malaperle edited edge metadata.Nov 7 2017, 7:00 AM

Hi Sam! After this patch, I see odd behavior when using Clangd in both VS Code and Eclipse. It looks like basically nothing happens after the initialize response:
<-- {"jsonrpc":"2.0","id":0,"method":"initialize","params":{.....}}
--> {"id":0,"jsonrpc":"2.0","result":{...}}

Have you seen this behavior? I haven't had the time to debug yet. If it is a real bug and not just on my machine, it's a bit alarming that the tests didn't catch it!

malaperle added inline comments.Nov 7 2017, 7:39 AM
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
39

I think "capabilities" was removed by mistake?