[PATCH] D31401: [clangd] Extract FsPath from file:// uri
stanionascu marked 2 inline comments as done. stanionascu added a comment. Thank you for the review! If there are no other comments/suggestions, would be great if someone would merge it as I'm lacking the permission to do so. https://reviews.llvm.org/D31401 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31401: [clangd] Extract FsPath from file:// uri
krasimir accepted this revision. krasimir added a comment. This revision is now accepted and ready to land. Looks good! https://reviews.llvm.org/D31401 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31401: [clangd] Extract FsPath from file:// uri
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
[PATCH] D31401: [clangd] Extract FsPath from file:// uri
stanionascu updated this revision to Diff 94271.
stanionascu added a comment.
Refactored URI handling to make it more explicit, also updated the parts which
expect a File parameter and not an URI.
https://reviews.llvm.org/D31401
Files:
clangd/ASTManager.cpp
clangd/ASTManager.h
clangd/DocumentStore.h
clangd/Protocol.cpp
clangd/Protocol.h
clangd/ProtocolHandlers.cpp
clangd/clients/clangd-vscode/src/extension.ts
Index: clangd/clients/clangd-vscode/src/extension.ts
===
--- clangd/clients/clangd-vscode/src/extension.ts
+++ clangd/clients/clangd-vscode/src/extension.ts
@@ -23,15 +23,23 @@
const clientOptions: vscodelc.LanguageClientOptions = {
// Register the server for C/C++ files
-documentSelector: ['c', 'cc', 'cpp', 'h', 'hh', 'hpp']
+documentSelector: ['c', 'cc', 'cpp', 'h', 'hh', 'hpp'],
+uriConverters: {
+// FIXME: by default the URI sent over the protocol will be percent encoded (see rfc3986#section-2.1)
+//the "workaround" below disables temporarily the encoding until decoding
+//is implemented properly in clangd
+code2Protocol: (uri: vscode.Uri) : string => uri.toString(true),
+protocol2Code: (uri: string) : vscode.Uri => undefined
+}
};
const clangdClient = new vscodelc.LanguageClient('Clang Language Server', serverOptions, clientOptions);
function applyTextEdits(uri: string, edits: vscodelc.TextEdit[]) {
let textEditor = vscode.window.activeTextEditor;
-if (textEditor && textEditor.document.uri.toString() === uri) {
+// FIXME: vscode expects that uri will be percent encoded
+if (textEditor && textEditor.document.uri.toString(true) === uri) {
textEditor.edit(mutator => {
for (const edit of edits) {
mutator.replace(vscodelc.Protocol2Code.asRange(edit.range), edit.newText);
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -21,7 +21,7 @@
Output.log("Failed to decode DidOpenTextDocumentParams!\n");
return;
}
- Store.addDocument(DOTDP->textDocument.uri, DOTDP->textDocument.text);
+ Store.addDocument(DOTDP->textDocument.uri.file, DOTDP->textDocument.text);
}
void TextDocumentDidChangeHandler::handleNotification(
@@ -32,7 +32,7 @@
return;
}
// We only support full syncing right now.
- Store.addDocument(DCTDP->textDocument.uri, DCTDP->contentChanges[0].text);
+ Store.addDocument(DCTDP->textDocument.uri.file, DCTDP->contentChanges[0].text);
}
/// Turn a [line, column] pair into an offset in Code.
@@ -83,9 +83,6 @@
// Call clang-format.
// FIXME: Don't ignore style.
format::FormatStyle Style = format::getLLVMStyle();
- // On windows FileManager doesn't like file://. Just strip it, clang-format
- // doesn't need it.
- Filename.consume_front("file://");
tooling::Replacements Replacements =
format::reformat(Style, Code, Ranges, Filename);
@@ -102,12 +99,12 @@
return;
}
- std::string Code = Store.getDocument(DRFP->textDocument.uri);
+ std::string Code = Store.getDocument(DRFP->textDocument.uri.file);
size_t Begin = positionToOffset(Code, DRFP->range.start);
size_t Len = positionToOffset(Code, DRFP->range.end) - Begin;
- writeMessage(formatCode(Code, DRFP->textDocument.uri,
+ writeMessage(formatCode(Code, DRFP->textDocument.uri.file,
{clang::tooling::Range(Begin, Len)}, ID));
}
@@ -121,14 +118,14 @@
// Look for the previous opening brace from the character position and format
// starting from there.
- std::string Code = Store.getDocument(DOTFP->textDocument.uri);
+ std::string Code = Store.getDocument(DOTFP->textDocument.uri.file);
size_t CursorPos = positionToOffset(Code, DOTFP->position);
size_t PreviousLBracePos = StringRef(Code).find_last_of('{', CursorPos);
if (PreviousLBracePos == StringRef::npos)
PreviousLBracePos = CursorPos;
size_t Len = 1 + CursorPos - PreviousLBracePos;
- writeMessage(formatCode(Code, DOTFP->textDocument.uri,
+ writeMessage(formatCode(Code, DOTFP->textDocument.uri.file,
{clang::tooling::Range(PreviousLBracePos, Len)}, ID));
}
@@ -141,8 +138,8 @@
}
// Format everything.
- std::string Code = Store.getDocument(DFP->textDocument.uri);
- writeMessage(formatCode(Code, DFP->textDocument.uri,
+ std::string Code = Store.getDocument(DFP->textDocument.uri.file);
+ writeMessage(formatCode(Code, DFP->textDocument.uri.file,
{clang::tooling::Range(0, Code.size())}, ID));
}
@@ -156,7 +153,7 @@
// We provide a code action for each diagnostic at the requested location
// which has FixIts available.
- std::string Code =
[PATCH] D31401: [clangd] Extract FsPath from file:// uri
krasimir added inline comments.
Comment at: clangd/ASTManager.cpp:166
tooling::CompilationDatabase *
ASTManager::getOrCreateCompilationDatabaseForFile(StringRef Uri) {
auto = CompilationDatabases[Uri];
I think we should rename the `Uri` parameter here to `File`.
Comment at: clangd/ASTManager.cpp:178
std::unique_ptr
ASTManager::createASTUnitForFile(StringRef Uri, const DocumentStore ) {
tooling::CompilationDatabase *CDB =
I think we should rename the `Uri` parameter here to `File`.
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") {
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.
Comment at: clangd/Protocol.cpp:167
if (KeyValue == "uri") {
- Result.uri = Value->getValue(Storage);
+ Result.uri = Uri::parse(Value->getValue(Storage));
} else if (KeyValue == "languageId") {
Same thing: I think that the clients should be responsible of converting
`Result.uri` to a filename.
Comment at: clangd/Protocol.h:32
+struct Uri {
+ static std::string parse(llvm::StringRef uri);
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.
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),
By the way, what does this do? I'm not a typescript vs code guru.
https://reviews.llvm.org/D31401
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31401: [clangd] Extract FsPath from file:// uri
stanionascu created this revision.
rfc8089#appendix-E.2 specifies that paths can begin with a drive letter e.g. as
file:///c:/.
In this case just consuming front file:// is not enough and the 3rd slash must
be consumed to produce a valid path on windows.
The patch introduce a generic way of converting an uri to a filesystem path and
back.
https://reviews.llvm.org/D31401
Files:
clangd/ASTManager.cpp
clangd/Protocol.cpp
clangd/Protocol.h
clangd/clients/clangd-vscode/src/extension.ts
Index: clangd/clients/clangd-vscode/src/extension.ts
===
--- clangd/clients/clangd-vscode/src/extension.ts
+++ clangd/clients/clangd-vscode/src/extension.ts
@@ -23,7 +23,12 @@
const clientOptions: vscodelc.LanguageClientOptions = {
// Register the server for C/C++ files
-documentSelector: ['c', 'cc', 'cpp', 'h', 'hh', 'hpp']
+documentSelector: ['c', 'cc', 'cpp', 'h', 'hh', 'hpp'],
+uriConverters: {
+// FIXME: implement percent decoding in clangd
+code2Protocol: (uri: vscode.Uri) : string => uri.toString(true),
+protocol2Code: (file: string) : vscode.Uri => vscode.Uri.file(file)
+}
};
const clangdClient = new vscodelc.LanguageClient('Clang Language Server', serverOptions, clientOptions);
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -29,6 +29,11 @@
namespace clang {
namespace clangd {
+struct Uri {
+ static std::string parse(llvm::StringRef uri);
+ static std::string unparse(llvm::StringRef file);
+};
+
struct TextDocumentIdentifier {
/// The text document's URI.
std::string uri;
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -17,8 +17,30 @@
#include "llvm/ADT/SmallString.h"
#include "llvm/Support/Format.h"
#include "llvm/Support/raw_ostream.h"
+#include "llvm/Support/Path.h"
using namespace clang::clangd;
+std::string Uri::parse(llvm::StringRef uri) {
+ uri.consume_front("file://");
+ // For Windows paths e.g. /X:
+ if (uri.size() > 2 && uri[0] == '/' && uri[2] == ':')
+uri.consume_front("/");
+ // Make sure that file paths are in native separators
+ std::string Result = llvm::sys::path::convert_to_slash(uri);
+ return Result;
+}
+
+std::string Uri::unparse(llvm::StringRef file) {
+ using namespace llvm::sys;
+ std::string Result = "file://";
+ // For Windows paths e.g. X:
+ if (file.size() > 1 && file[1] == ':')
+Result += "/";
+ // Make sure that uri paths are with posix separators
+ Result += path::convert_to_slash(file, path::Style::posix);
+ return Result;
+}
+
llvm::Optional
TextDocumentIdentifier::parse(llvm::yaml::MappingNode *Params) {
TextDocumentIdentifier Result;
@@ -36,7 +58,7 @@
llvm::SmallString<10> Storage;
if (KeyValue == "uri") {
- Result.uri = Value->getValue(Storage);
+ Result.uri = Uri::parse(Value->getValue(Storage));
} else if (KeyValue == "version") {
// FIXME: parse version, but only for VersionedTextDocumentIdentifiers.
} else {
@@ -142,7 +164,7 @@
llvm::SmallString<10> Storage;
if (KeyValue == "uri") {
- Result.uri = Value->getValue(Storage);
+ Result.uri = Uri::parse(Value->getValue(Storage));
} else if (KeyValue == "languageId") {
Result.languageId = Value->getValue(Storage);
} else if (KeyValue == "version") {
Index: clangd/ASTManager.cpp
===
--- clangd/ASTManager.cpp
+++ clangd/ASTManager.cpp
@@ -28,7 +28,6 @@
std::vector RemappedFiles;
for (const auto : Docs.getAllDocuments()) {
StringRef FileName = P.first;
-FileName.consume_front("file://");
RemappedFiles.push_back(ASTUnit::RemappedFile(
FileName,
llvm::MemoryBuffer::getMemBufferCopy(P.second, FileName).release()));
@@ -138,7 +137,7 @@
Diagnostics.pop_back(); // Drop trailing comma.
Output.writeMessage(
R"({"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":")" +
- File + R"(","diagnostics":[)" + Diagnostics + R"(]}})");
+ Uri::unparse(File) + R"(","diagnostics":[)" + Diagnostics + R"(]}})");
}
ASTManager::~ASTManager() {
@@ -169,8 +168,6 @@
if (I)
return I.get();
- Uri.consume_front("file://");
-
std::string Error;
I = tooling::CompilationDatabase::autoDetectFromSource(Uri, Error);
Output.log("Failed to load compilation database: " + Twine(Error) + "\n");
@@ -182,7 +179,6 @@
tooling::CompilationDatabase *CDB =
getOrCreateCompilationDatabaseForFile(Uri);
- Uri.consume_front("file://");
std::vector Commands;
if (CDB) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
