stanionascu marked 5 inline comments as done.
stanionascu added inline comments.
================
Comment at: clangd/Protocol.cpp:61
if (KeyValue == "uri") {
- Result.uri = Value->getValue(Storage);
+ Result.uri = Uri::parse(Value->getValue(Storage));
} else if (KeyValue == "version") {
----------------
krasimir wrote:
> Hm, seems to me that here the uri should be kept as-is and the clients of
> `Result.uri` should convert it to file when appropriate. Otherwise it's
> confusing.
Implemented the "URI::parse" to store both representations.
================
Comment at: clangd/Protocol.h:32
+struct Uri {
+ static std::string parse(llvm::StringRef uri);
----------------
krasimir wrote:
> It's a little confusing: the `parse` method turns an `uri` to a `file` and
> the `unparse` method does the opposite. Maybe we should use more descriptive
> names, like `uriToFile` and `fileToUri`. This does not seem to be
> inconsistent with the rest of the protocol since the protocol only cares
> about uri-s, and file-s are an implementation detail of clangd I guess.
2nd try now, instead of actually storing a string based URI, store the object
which both representations (uri and file).
================
Comment at: clangd/clients/clangd-vscode/src/extension.ts:28
+ uriConverters: {
+ // FIXME: implement percent decoding in clangd
+ code2Protocol: (uri: vscode.Uri) : string => uri.toString(true),
----------------
krasimir wrote:
> By the way, what does this do? I'm not a typescript vs code guru.
Extended the FIXME, with the description, e.g. when vscode (on windows) sends
the uri to clangd, it's represented as "file:///c%3A/path/tofile" which leads
to:
- incorrect detection of the compilation database location.
- cannot find the actual unit in the database.
uri.toString(true) - skips the percent encoding and sends the uri parameter to
clangd as "file:///c:/path/tofile"
https://reviews.llvm.org/D31401
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
