joerg added a comment.
We already have a couple of case that expect the encoding to be compatible. I'm
not very attached to the additional special cases from YAML, but having either
a common escape function OR a JSON escape in LLVM/Support is quite important.
https://reviews.llvm.org/D31992
malaperle-ericsson added a comment.
In https://reviews.llvm.org/D31992#728036, @joerg wrote:
> Just to avoid any confusion: this should be the generic YAML escape routine
> in llvm/lib/Support, i.e. IMO we don't want to have separate YAML and JSON
> escape routines.
> Effective, change YAMLPar
joerg added a comment.
Just to avoid any confusion: this should be the generic YAML escape routine in
llvm/lib/Support, i.e. IMO we don't want to have separate YAML and JSON escape
routines.
Effective, change YAMLParser.cpp line 697 to use \u and drop the whole UTF-8
handling part.
https://re
malaperle-ericsson added a comment.
Once the use of "two characters representation" is clarified, I will update the
patch again.
https://reviews.llvm.org/D31992
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/m
joerg added a comment.
The short version is perfectly fine as long as works for both JSON and YAML.
Less output is always good :)
https://reviews.llvm.org/D31992
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/
malaperle-ericsson updated this revision to Diff 95414.
malaperle-ericsson added a comment.
Handle other control characters and add test
https://reviews.llvm.org/D31992
Files:
clangd/ASTManager.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
test/clangd/encoding.
malaperle-ericsson added a comment.
In https://reviews.llvm.org/D31992#726447, @joerg wrote:
> let's escape ... all the known ASCII control characters,
Do you mean encode all of them with \u or keep the two characters
representation for those that exist? I think \n is nicer than \u000A and i
joerg added a comment.
In https://reviews.llvm.org/D31992#725963, @klimek wrote:
> If I understand correctly, that's " and \ for JSON and ", \ and all
> non-printable characters (which unfortunately requires understanding of
> unicode to solve this fully correctly) in YAML.
I'd modify that sl
klimek added a comment.
In https://reviews.llvm.org/D31992#725912, @malaperle-ericsson wrote:
> In https://reviews.llvm.org/D31992#725866, @krasimir wrote:
>
> > Seems that we're starting to hit some YAML/JSON mismatches, or is it that
> > your YAML string support is lacking?
>
>
> I don't think
klimek added inline comments.
Comment at: clangd/Protocol.cpp:26-50
+ for (llvm::StringRef::iterator i = Input.begin(), e = Input.end(); i != e;
++i) {
+if (*i == '\\')
+ EscapedInput += "";
+else if (*i == '"')
+ EscapedInput += "\\\"";
+// bell
+
malaperle-ericsson added a comment.
In https://reviews.llvm.org/D31992#725913, @joerg wrote:
> I'm strongly against this patch. Can you give an actual test case for the
> problematic behavior?
Sure I can add a test. If you meant more real work scenario, you can juste type
"é" in VS Code and i
joerg added a comment.
I'm strongly against this patch. Can you give an actual test case for the
problematic behavior?
Repository:
rL LLVM
https://reviews.llvm.org/D31992
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.
malaperle-ericsson added a comment.
In https://reviews.llvm.org/D31992#725866, @krasimir wrote:
> Seems that we're starting to hit some YAML/JSON mismatches, or is it that
> your YAML string support is lacking?
I don't think so. It seems like JSON and YAML are not completely aligned on
escape
sorry, is it that *our* YAML string support is lacking?
On Thu, Apr 13, 2017 at 2:38 PM, Krasimir Georgiev via Phabricator <
revi...@reviews.llvm.org> wrote:
> krasimir added a comment.
>
> Seems that we're starting to hit some YAML/JSON mismatches, or is it that
> your YAML string support is lac
krasimir added a comment.
Seems that we're starting to hit some YAML/JSON mismatches, or is it that your
YAML string support is lacking?
Repository:
rL LLVM
https://reviews.llvm.org/D31992
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
15 matches
Mail list logo
