[PATCH] D55363: [clangd] Expose FileStatus in LSP.

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In D55363#1329652 , @ilya-biryukov 
wrote:

> Sorry if I missed any important design discussions here, but wanted to clear 
> up what information we are trying to convey to the user with the status 
> messages?
>  E.g. why seeing "building preamble", "building file" or "queued" in the 
> status bar can be useful to the user? Those messages mention internals of 
> clangd, I'm not sure how someone unfamiliar with internals of clangd should 
> interpret this information.


I agree about not necessarily needing to expose clang internals, but I think 
the end goal of providing some feedback to the user (still thinking/done) is 
worthy.

If this is a clangd-specific event, shouldn't the name of the event reflect it? 
 As-is, it looks like an official LSP feature and can be confusing.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55363



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55250: [clangd] Enhance macro hover to see full definition

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/XRefs.cpp:572
+
+  // Try to get the full definition, not just the name
+  SourceLocation StartLoc = Decl.Info->getDefinitionLoc();

hokein wrote:
> if this is a complicated macro (like `AST_MATCHER`), do we still want to 
> return all the content? they might be less useful than the simple macros.
I think it would be hard to decide here what constitutes a complicated or 
not-complicated macro (one for which we want to return all the content or not). 
 Also, I think it's fine to return the whole content, since frontends generally 
have good ways to present large contents in popups (hover popups that can be 
scrolled).

If we realize later we need to limit the size, then we can add an option where 
the frontend can specify a size limit.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55250



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54077: [clangd] Implemented DraftFileSystem

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D54077#1287153, @klimek wrote:

> I'm in yet another camp: I carefully save when I have something that is 
> correct enough syntax, so I only want errors from with changes from the exact 
> file I'm editing and the rest of the files in saved state.


That sounds like the current behavior, doesn't it?

Personally, I would like the behavior proposed by this patch.  I think it 
perfectly in line with the idea of showing the same diagnostics as the compiler 
would produce.  It's just that it's what the compiler would produce if 
compiling the files as seen in the editor.

Also, in the LSP, after a `didOpen` and until the corresponding `didClose`, the 
source of truth for that file is supposed to be what is in the editor buffer.  
So I think it would make sense that if other files include the edited file, 
they should see the content from the editor.

Of course, that is dependent of having an efficient cancelling mechanism.  Even 
with some debouncing, an update to a header file can trigger the re-processing 
of many TUs, so if I do another edit to the same header shortly after, all the 
queued jobs from the first edit should be dropped.  But I think we already have 
that, don't we?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54077



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53220: Remove possibility to change compile database path at runtime

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE344614: Remove possibility to change compile database path 
at runtime (authored by simark, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53220?vs=169828=169833#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53220

Files:
  clangd/ClangdLSPServer.cpp
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/compile-commands-path.test

Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -665,10 +665,19 @@
 bool fromJSON(const json::Value ,
   ClangdConfigurationParamsChange ) {
   json::ObjectMapper O(Params);
-  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath) &&
+  return O &&
  O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges);
 }
 
+bool fromJSON(const json::Value , ClangdInitializationOptions ) {
+  if (!fromJSON(Params, Opts.ParamsChange)) {
+return false;
+  }
+
+  json::ObjectMapper O(Params);
+  return O && O.map("compilationDatabasePath", Opts.compilationDatabasePath);
+}
+
 bool fromJSON(const json::Value , ReferenceParams ) {
   TextDocumentPositionParams  = R;
   return fromJSON(Params, Base);
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -81,8 +81,16 @@
 } // namespace
 
 void ClangdLSPServer::onInitialize(InitializeParams ) {
-  if (Params.initializationOptions)
-applyConfiguration(*Params.initializationOptions);
+  if (Params.initializationOptions) {
+const ClangdInitializationOptions  = *Params.initializationOptions;
+
+// Explicit compilation database path.
+if (Opts.compilationDatabasePath.hasValue()) {
+  CDB.setCompileCommandsDir(Opts.compilationDatabasePath.getValue());
+}
+
+applyConfiguration(Opts.ParamsChange);
+  }
 
   if (Params.rootUri && *Params.rootUri)
 Server->setRootPath(Params.rootUri->file());
@@ -425,17 +433,10 @@
 }
 
 void ClangdLSPServer::applyConfiguration(
-const ClangdConfigurationParamsChange ) {
-  // Compilation database change.
-  if (Settings.compilationDatabasePath.hasValue()) {
-CDB.setCompileCommandsDir(Settings.compilationDatabasePath.getValue());
-
-reparseOpenedFiles();
-  }
-
-  // Update to the compilation database.
-  if (Settings.compilationDatabaseChanges) {
-const auto  = *Settings.compilationDatabaseChanges;
+const ClangdConfigurationParamsChange ) {
+  // Per-file update to the compilation database.
+  if (Params.compilationDatabaseChanges) {
+const auto  = *Params.compilationDatabaseChanges;
 bool ShouldReparseOpenFiles = false;
 for (auto  : CompileCommandUpdates) {
   /// The opened files need to be reparsed only when some existing
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -411,16 +411,21 @@
 /// "initialize" request and for the "workspace/didChangeConfiguration"
 /// notification since the data received is described as 'any' type in LSP.
 struct ClangdConfigurationParamsChange {
-  llvm::Optional compilationDatabasePath;
-
   // The changes that happened to the compilation database.
   // The key of the map is a file name.
   llvm::Optional>
   compilationDatabaseChanges;
 };
 bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
 
-struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {};
+struct ClangdInitializationOptions {
+  // What we can change throught the didChangeConfiguration request, we can
+  // also set through the initialize request (initializationOptions field).
+  ClangdConfigurationParamsChange ParamsChange;
+
+  llvm::Optional compilationDatabasePath;
+};
+bool fromJSON(const llvm::json::Value &, ClangdInitializationOptions &);
 
 struct InitializeParams {
   /// The process Id of the parent process that started
Index: test/clangd/compile-commands-path-in-initialize.test
===
--- test/clangd/compile-commands-path-in-initialize.test
+++ test/clangd/compile-commands-path-in-initialize.test
@@ -3,10 +3,8 @@
 
 # RUN: rm -rf %t.dir/* && mkdir -p %t.dir
 # RUN: mkdir %t.dir/build-1
-# RUN: mkdir %t.dir/build-2
 # RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
 # RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-1/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-2/compile_commands.json
 
 # RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s 

[PATCH] D53220: Remove possibility to change compile database path at runtime

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 169828.
simark marked 3 inline comments as done.
simark added a comment.

Address comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53220

Files:
  clangd/ClangdLSPServer.cpp
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/compile-commands-path.test

Index: test/clangd/compile-commands-path.test
===
--- test/clangd/compile-commands-path.test
+++ /dev/null
@@ -1,42 +0,0 @@
-# Test that we can switch between configuration/build using the
-# workspace/didChangeConfiguration notification.
-
-# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
-# RUN: mkdir %t.dir/build-1
-# RUN: mkdir %t.dir/build-2
-# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-1/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-2/compile_commands.json
-
-# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
-
-# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
-# (with the extra slash in the front), so we add it here.
-# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
-
-# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
-
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}

-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message (\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() {}\n"}}}
-# CHECK:   "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT:   "params": {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT:   {
-# CHECK-NEXT: "message": "MACRO is not defined",

-{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
-# CHECK:   "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT:   "params": {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT:   {
-# CHECK-NEXT: "message": "MACRO is one",

-{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
-# CHECK:   "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT:   "params": {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT:   {
-# CHECK-NEXT: "message": "MACRO is two",

-{"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: test/clangd/compile-commands-path-in-initialize.test
===
--- test/clangd/compile-commands-path-in-initialize.test
+++ test/clangd/compile-commands-path-in-initialize.test
@@ -3,10 +3,8 @@
 
 # RUN: rm -rf %t.dir/* && mkdir -p %t.dir
 # RUN: mkdir %t.dir/build-1
-# RUN: mkdir %t.dir/build-2
 # RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
 # RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-1/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-2/compile_commands.json
 
 # RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
 
@@ -18,18 +16,11 @@
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"initializationOptions":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
 ---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message (\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() {}\n"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() {}\n"}}}
 # CHECK:   "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT: "diagnostics": [
 # CHECK-NEXT:   {
 # CHECK-NEXT: "message": "MACRO is one",
 ---

[PATCH] D53220: Remove possibility to change compile database path at runtime

[Simon Marchi via Phabricator via cfe-commits]

simark marked 7 inline comments as done.
simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:433
 
 reparseOpenedFiles();
   }

sammccall wrote:
> sammccall wrote:
> > This isn't needed, the compilation database can only be set during 
> > initialization.
> It's still here... maybe forgot to upload a new diff?
> (Just to be clear: I meant `reparseOpenFiles` doesn't need to be called, as 
> there are none.)
Oh I have only done the change locally, I have not uploaded a new diff yet.



Comment at: clangd/ClangdLSPServer.h:90
   void reparseOpenedFiles();
+  void applyConfiguration(const ClangdInitializationOptions );
   void applyConfiguration(const ClangdConfigurationParamsChange );

sammccall wrote:
> simark wrote:
> > sammccall wrote:
> > > Prefer a different name for this function - an overload set should have 
> > > similar semantics and these are quite different (pseudo-constructor vs 
> > > mutation allowed at any time).
> > Ok, I'll think about a better name.
> (In fact, this could also live directly in `onInitialize`, I think?)
Indeed, since it's things that can only be set during initialization.



Comment at: clangd/Protocol.h:422
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};

sammccall wrote:
> simark wrote:
> > sammccall wrote:
> > > Can we just move this to InitializeParams as a clangd extension?
> > > Doing tricks with inheritance here makes the protocol harder to 
> > > understand.
> > > Can we just move this to InitializeParams as a clangd extension?
> > 
> > Do you mean move it in the JSON, so it looks like this on the wire?
> > 
> > ```
> > {
> >   "method": "initialize",
> >   "params": {
> > "compilationDatabasePath": "",
> > ...
> >   }
> > }
> > ```
> > 
> > instead of 
> > 
> > ```
> > {
> >   "method": "initialize",
> >   "params": {
> > "initializationOptions": {
> >   "compilationDatabasePath": ""
> > },
> > ...
> >   }
> > }
> > ```
> > 
> > ?
> > 
> > I think `initializationOptions` is a good place for this to be, I wouldn't 
> > change that..  If you don't like the inheritance, we can just get rid of it 
> > in our code and have two separate versions of the deserializing code.  We 
> > designed it so `didChangeConfiguration` and the initialization options 
> > would share the same structure, but it doesn't have to stay that way.
> > Do you mean move it in the JSON, so it looks like this on the wire?
> 
> Well, since you asked... :-) I'm not going to push hard for it (this patch is 
> certainly a win already), but I do think that would be much clearer.
> 
> The current protocol has `InitializeParams` and 
> `ClangdInitializationOptions`, and it's not clear semantically what the 
> distinction between them is.
> 
> With hindsight, I think something like this would be easier to follow:
> ```
> // Clangd options that may be set at startup.
> struct InitializeParams {
>   // Clangd extension: override the path used to load the CDB.
>   Optional compilationDatabasePath;
>   // Provides initial configuration as if by workspace/updateConfiguration.
>   Optional initialConfiguration;
> }
> // Clangd options that may be set dynamically at runtime.
> struct ClangdConfigurationParamsChange { ... }
> ```
> though even here, the benefit from being able to inline the initial 
> configuration into the initalize message is unclear to me. The implementation 
> has to support dynamic updates in any case, so why not make use of that?
> 
> > We designed it so didChangeConfiguration and the initialization options 
> > would share the same structure
> 
> This makes sense, but if they're diverging, I'm not sure that keeping them 
> *mostly* the same brings more benefits than confusion.
> 
> --
> 
> That said, if you prefer to keep the JSON as it is, that's fine. (If we grow 
> more extensions, we may want to reorganize in future though?)
> My main concern is the use of inheritance here, and how it provides a default 
> (configuration-change options can be provided at startup) that doesn't seem 
> obviously correct and is hard to opt out of.
> The current protocol has InitializeParams and ClangdInitializationOptions, 
> and it's not clear semantically what the distinction between them is.

`InitializeParams` is the type for the parameters of the `initialize` request.  
In it, there is this field:

```
/**
 * User provided initialization options.
 */
initializationOptions?: any;
```

which is made to pass such language-server-specific options.  Since it's of the 
`any` type, we gave it a name, `ClangdInitializationOptions`.

> With hindsight, I think something like this would be easier to follow:
>
> [snippet]

I don't really understand why putting fields as extra fields in 
`InitializeParams` would be any easier than putting them in the object 

[PATCH] D53220: Remove possibility to change compile database path at runtime

[Simon Marchi via Phabricator via cfe-commits]

simark marked an inline comment as done.
simark added inline comments.



Comment at: clangd/ClangdLSPServer.h:90
   void reparseOpenedFiles();
+  void applyConfiguration(const ClangdInitializationOptions );
   void applyConfiguration(const ClangdConfigurationParamsChange );

sammccall wrote:
> Prefer a different name for this function - an overload set should have 
> similar semantics and these are quite different (pseudo-constructor vs 
> mutation allowed at any time).
Ok, I'll think about a better name.



Comment at: clangd/Protocol.h:422
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};

sammccall wrote:
> Can we just move this to InitializeParams as a clangd extension?
> Doing tricks with inheritance here makes the protocol harder to understand.
> Can we just move this to InitializeParams as a clangd extension?

Do you mean move it in the JSON, so it looks like this on the wire?

```
{
  "method": "initialize",
  "params": {
"compilationDatabasePath": "",
...
  }
}
```

instead of 

```
{
  "method": "initialize",
  "params": {
"initializationOptions": {
  "compilationDatabasePath": ""
},
...
  }
}
```

?

I think `initializationOptions` is a good place for this to be, I wouldn't 
change that..  If you don't like the inheritance, we can just get rid of it in 
our code and have two separate versions of the deserializing code.  We designed 
it so `didChangeConfiguration` and the initialization options would share the 
same structure, but it doesn't have to stay that way.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53220



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51725: Allow un-setting the compilation database path

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

Thanks for your input.  I have opened https://reviews.llvm.org/D53220, which 
removes that option.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53220: Remove possibility to change compile database path at runtime

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, ioeric, 
ilya-biryukov.

This patch removes the possibility to change the compilation database
path at runtime using the didChangeConfiguration request.  Instead, it
is suggested to use the setting on the initialize request, and clangd
whenever the user wants to use a different build configuration.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53220

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/compile-commands-path.test

Index: test/clangd/compile-commands-path.test
===
--- test/clangd/compile-commands-path.test
+++ /dev/null
@@ -1,42 +0,0 @@
-# Test that we can switch between configuration/build using the
-# workspace/didChangeConfiguration notification.
-
-# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
-# RUN: mkdir %t.dir/build-1
-# RUN: mkdir %t.dir/build-2
-# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-1/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-2/compile_commands.json
-
-# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
-
-# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
-# (with the extra slash in the front), so we add it here.
-# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
-
-# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
-
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}

-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message (\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() {}\n"}}}
-# CHECK:   "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT:   "params": {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT:   {
-# CHECK-NEXT: "message": "MACRO is not defined",

-{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
-# CHECK:   "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT:   "params": {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT:   {
-# CHECK-NEXT: "message": "MACRO is one",

-{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
-# CHECK:   "method": "textDocument/publishDiagnostics",
-# CHECK-NEXT:   "params": {
-# CHECK-NEXT: "diagnostics": [
-# CHECK-NEXT:   {
-# CHECK-NEXT: "message": "MACRO is two",

-{"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: test/clangd/compile-commands-path-in-initialize.test
===
--- test/clangd/compile-commands-path-in-initialize.test
+++ test/clangd/compile-commands-path-in-initialize.test
@@ -3,10 +3,8 @@
 
 # RUN: rm -rf %t.dir/* && mkdir -p %t.dir
 # RUN: mkdir %t.dir/build-1
-# RUN: mkdir %t.dir/build-2
 # RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": "the-file.cpp"}]' > %t.dir/compile_commands.json
 # RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-1/compile_commands.json
-# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 the-file.cpp", "file": "../the-file.cpp"}]' > %t.dir/build-2/compile_commands.json
 
 # RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
 
@@ -18,18 +16,11 @@
 
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"initializationOptions":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
 ---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message (\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() {}\n"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 1\n#pragma message (\"MACRO is one\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() 

[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

I just tried this, this looks very promising!  It should help build our outline 
view in a much more robust way than we do currently.

A nit for the final patch, I would suggest omitting the fields that are 
optional, like `children` (when the list is empty) and `deprecated`.

In vscode, is there a way to get a tree representation of this data?  When I 
look at "Go to symbol in File..." (ctrl-shift-o) or the outline view at the 
bottom of the file explorer, they are both a flat list.  What difference does 
this patch make in how vscode shows the data?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52311



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51725: Allow un-setting the compilation database path

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

So I revisited this today.  In the end, restarting clangd in our IDE works 
well.  It should be merged anytime soon:

https://github.com/theia-ide/theia/pull/3017

But I am wondering what to do with the feature that allows clangd to change 
compilation database path using the `didChangeConfiguration` notification.  In 
my opinion, it's a bug that it's not possible to switch back to use no explicit 
compilation database path, so I'd still like to get this patch merged.  Or, if 
we decide this is really not useful, then it should be removed.  I don't think 
we should keep the feature in the current state, either we fix it or remove it.

Even if right now it's not very expensive to restart clangd, do you foresee a 
situation in the future where it could become more expensive?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

Ohh awesome, I didn't know the LSP supported that.  I'll try it it Theia when I 
have time.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52311



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51725: Allow un-setting the compilation database path

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D51725#1232748, @ilya-biryukov wrote:

> If this setting exposed directly the users in Theia or is it something that 
> is exposed via a custom UI (e.g. choosing named build configs or something 
> similar)?


It is through a custom UI, that we name exactly that, build configurations.  
There is an example of the corresponding configuration here:

https://github.com/theia-ide/theia/blob/master/packages/cpp/README.md

>> I'll investigate how difficult it is to make our frontend (Theia 
>> ) restart clangd when the user switches 
>> compilation database.
> 
> If that is feasible, might allow removing some code from clangd. Though don't 
> expect that amount to be too high.
>  One potential complication to restoring the state of the language server. It 
> should be just the list of open files, but I may be missing something.

I am not sure I understand correctly your last point.  Of course, when 
restarting clangd, we need to send again a `didOpen` for each file the user has 
currently open in the editor.  But that should be  it?

> If you'll decide to go with an option to reset the path, see the comment 
> about making empty path special. `Optional>` does not play nicely 
> with json serialization code and hard to read (even though it does look like 
> the right thing to do from the type system perspective).

Yes, noted.  I preferred to avoid giving a special meaning to an empty string 
because we could avoid it.  But I wouldn't mind changing it if we go that route.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51725: Allow un-setting the compilation database path

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D51725#1232092, @ilya-biryukov wrote:

> Wow, this is getting somewhat complicated.
>
> Have you considered rerunning clangd whenever someone changes an option like 
> that?
>  Would that be much more complicated on your side?
>
> Not opposed to having an option too, just want to be aware of the costs 
> involved on your end.


Since we already support changing the compilation database path at runtime, I 
don't think it's significantly more complex to support resetting that value to 
the default of empty/not-used.  If we can restart clangd when the users chooses 
to use no compilation database path, we might as well just restart it every 
time the user selects a new compilation database path.

I was assuming that restarting clangd might potentially be significantly more 
costly than just changing a setting, but maybe I'm wrong.  The point of 
switching compilation database path is usually to point to a different build 
that has different flags, so all open files will get reparsed anyway...  And 
just start clangd isn't particularly heavy.

I'll investigate how difficult it is to make our frontend (Theia 
) restart clangd when the user switches 
compilation database.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51725: Allow un-setting the compilation database path

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

Not sure who should review this, please feel free to add anybody who would be 
appropriate.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51725: Allow un-setting the compilation database path

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 164183.
simark added a comment.

Fix formatting.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/compile-commands-path.test

Index: test/clangd/compile-commands-path.test
===
--- test/clangd/compile-commands-path.test
+++ test/clangd/compile-commands-path.test
@@ -39,4 +39,11 @@
 # CHECK-NEXT:   {
 # CHECK-NEXT: "message": "MACRO is two",
 ---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":null}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is not defined",
+---
 {"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -358,7 +358,14 @@
 /// "initialize" request and for the "workspace/didChangeConfiguration"
 /// notification since the data received is described as 'any' type in LSP.
 struct ClangdConfigurationParamsChange {
-  llvm::Optional compilationDatabasePath;
+  /// Path to the directory containing the compile_commands.json file to use.
+  /// The outer Optional is empty if the compilationDatabasePath field was not
+  /// present at all in the request.
+  /// The inner Optional is empty if the compilationDatabasePath field was
+  /// preset, but the value was null.
+  /// If the value of the compilationDatabasePath in the request is a string,
+  /// both Optionals are instantiated.
+  llvm::Optional> compilationDatabasePath;
 
   // The changes that happened to the compilation database.
   // The key of the map is a file name.
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -609,10 +609,22 @@
  O.map("compilationCommand", CDbUpdate.compilationCommand);
 }
 
-bool fromJSON(const json::Value ,
+bool fromJSON(const json::Value ,
   ClangdConfigurationParamsChange ) {
-  json::ObjectMapper O(Params);
-  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath) &&
+  json::ObjectMapper O(Settings);
+  const json::Value *P = Settings.getAsObject()->get("compilationDatabasePath");
+
+  if (P) {
+// The field is present.
+CCPC.compilationDatabasePath.emplace();
+llvm::Optional S = P->getAsString();
+if (S) {
+  // The field has a string value.
+  CCPC.compilationDatabasePath->emplace(*S);
+}
+  }
+
+  return O &&
  O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges);
 }
 
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -63,7 +63,7 @@
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
   /// Set the compile commands directory to \p P.
-  void setCompileCommandsDir(Path P);
+  void setCompileCommandsDir(llvm::Optional P);
 
   /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -60,7 +60,8 @@
   return C;
 }
 
-void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(Path P) {
+void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(
+llvm::Optional P) {
   std::lock_guard Lock(Mutex);
   CompileCommandsDir = P;
   CompilationDatabases.clear();
Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -126,9 +126,10 @@
 void setExtraFlagsForFile(PathRef File,
   std::vector ExtraFlags);
 
-/// Set the compile commands directory to \p P.
+/// Set the compile commands directory to \p P.  An empty Optional value
+/// means to not use an explicit compile commands directory path.
 /// Only valid for directory-based CDB, no-op and error log on InMemoryCDB;
-void setCompileCommandsDir(Path P);
+void setCompileCommandsDir(llvm::Optional P);
 
 /// Returns a CDB that should be used to get compile commands for the
 /// current instance of ClangdLSPServer.
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -613,14 +613,15 @@
   

[PATCH] D51725: Allow un-setting the compilation database path

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, ioeric, 
ilya-biryukov.

It is currently possible to tell clangd where to find the
compile_commands.json file through the initializationOptions or the
didChangeConfiguration message.  However, it is not possible to tell
clangd to deselect any explicit compilation database path (i.e. go back
to the default).

This patch makes it possible by sending the value null:

  params: {
"settings": {
  "compilationDatabasePath": null
}
  }

Not including the compilationDatabasePath field doesn't change the value
(which means it doesn't deselect it if one is already set).  I chose to
do it this way because the other possible field,
compilationDatabaseChanges, just contains a delta.  So I think it makes
sense if compilationDatabasePath works the same way.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51725

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/compile-commands-path.test

Index: test/clangd/compile-commands-path.test
===
--- test/clangd/compile-commands-path.test
+++ test/clangd/compile-commands-path.test
@@ -39,4 +39,11 @@
 # CHECK-NEXT:   {
 # CHECK-NEXT: "message": "MACRO is two",
 ---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":null}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is not defined",
+---
 {"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -358,7 +358,14 @@
 /// "initialize" request and for the "workspace/didChangeConfiguration"
 /// notification since the data received is described as 'any' type in LSP.
 struct ClangdConfigurationParamsChange {
-  llvm::Optional compilationDatabasePath;
+  /// Path to the directory containing the compile_commands.json file to use.
+  /// The outer Optional is empty if the compilationDatabasePath field was not
+  /// present at all in the request.
+  /// The inner Optional is empty if the compilationDatabasePath field was
+  /// preset, but the value was null.
+  /// If the value of the compilationDatabasePath in the request is a string,
+  /// both Optionals are instantiated.
+  llvm::Optional> compilationDatabasePath;
 
   // The changes that happened to the compilation database.
   // The key of the map is a file name.
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -609,11 +609,22 @@
  O.map("compilationCommand", CDbUpdate.compilationCommand);
 }
 
-bool fromJSON(const json::Value ,
+bool fromJSON(const json::Value ,
   ClangdConfigurationParamsChange ) {
-  json::ObjectMapper O(Params);
-  return O && O.map("compilationDatabasePath", CCPC.compilationDatabasePath) &&
- O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges);
+  json::ObjectMapper O(Settings);
+  const json::Value *P = Settings.getAsObject()->get("compilationDatabasePath");
+
+  if (P) {
+// The field is present.
+CCPC.compilationDatabasePath.emplace();
+llvm::Optional S = P->getAsString();
+if (S) {
+  // The field has a string value.
+  CCPC.compilationDatabasePath->emplace(*S);
+}
+  }
+
+  return O && O.map("compilationDatabaseChanges", CCPC.compilationDatabaseChanges);
 }
 
 bool fromJSON(const json::Value , ReferenceParams ) {
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -63,7 +63,7 @@
   tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
   /// Set the compile commands directory to \p P.
-  void setCompileCommandsDir(Path P);
+  void setCompileCommandsDir(llvm::Optional P);
 
   /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -60,7 +60,7 @@
   return C;
 }
 
-void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(Path P) {
+void DirectoryBasedGlobalCompilationDatabase::setCompileCommandsDir(llvm::Optional P) {
   std::lock_guard Lock(Mutex);
   CompileCommandsDir = P;
   CompilationDatabases.clear();
Index: clangd/ClangdLSPServer.h

[PATCH] D51311: (WIP) Type hierarchy

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/Protocol.h:891
+
+  std::vector Parents;
+

ilya-biryukov wrote:
> What is the proposal to add children (i.e. overriden methods) to the 
> hierarchy?
> This seems like a place where we might want to have multiple requests from 
> the clients when expanding the nodes.
In my prototype, I fetch the whole parent hierarchy in a single request.  In 
the proposal at

  https://github.com/Microsoft/vscode-languageserver-node/pull/346

it has been suggested to only fetch one level at the time, and have the client 
issue as many request as it wants (until possibly getting the whole hierarchy). 
 I don't know what the protocol will end up like. 



Comment at: clangd/ProtocolHandlers.cpp:70
   Register("textDocument/hover", ::onHover);
+  Register("textDocument/typeHierarchy", ::onTypeHierarchy);
   Register("textDocument/documentSymbol", 
::onDocumentSymbol);

kadircet wrote:
> LSP doesn't have such an entry, maybe we should use [[ 
> https://microsoft.github.io/language-server-protocol/specification#workspace_executeCommand
>  | workspace/executeCommand ]]. WDYT @ilya-biryukov 
I don't intend to merge this patch before the protocol actually defines the way 
to get type hierarchy.  So this will be updated to reflect what the protocol 
will actually define.



Comment at: clangd/XRefs.cpp:669
+  const CXXMethodDecl *Candidate) {
+  // FIXME: How do I determine if Method overrides Candidate?
+

ilya-biryukov wrote:
> kadircet wrote:
> > ilya-biryukov wrote:
> > > simark wrote:
> > > > kadircet wrote:
> > > > > https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp
> > > > > 
> > > > > you can check this sample, which simply traverses all base classes 
> > > > > and gets methods with the same name, then checks whether one is 
> > > > > overload of the other. If it they are not overloads then one in the 
> > > > > base classes gets overriden by the other.
> > > > > 
> > > > > 
> > > > > Another approach could be to search for one method in others 
> > > > > overriden_methods.
> > > > > 
> > > > > But they are both inefficient, I would suggest calling these 
> > > > > functions only when one of them is defined in the base class of other 
> > > > > then you can just check for name equality and not being an overload.
> > > > > https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp
> > > > 
> > > > Did you mean to link to a particular line?
> > > > 
> > > > > you can check this sample, which simply traverses all base classes 
> > > > > and gets methods with the same name, then checks whether one is 
> > > > > overload of the other. If it they are not overloads then one in the 
> > > > > base classes gets overriden by the other.
> > > > 
> > > > > Another approach could be to search for one method in others 
> > > > > overriden_methods.
> > > > 
> > > > I have tried using `overriden_methods`, but it only contains methods 
> > > > marked virtual.  For this feature, I would like to consider non-virtual 
> > > > methods too.
> > > > 
> > > > > But they are both inefficient, I would suggest calling these 
> > > > > functions only when one of them is defined in the base class of other 
> > > > > then you can just check for name equality and not being an overload.
> > > > 
> > > > I am not sure I understand, but maybe it will be clearer when I will 
> > > > see the sample.
> > > See the code that actually computes a list for `override_methods()`: 
> > > Sema::AddOverriddenMethods.
> > > Did you mean to link to a particular line?
> > 
> > 
> > Yeah sorry, I was trying to link the function ilya mentioned.
> > https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp#L7615
> > 
> > Since you also mentioned checking non-virtual method as well you might 
> > wanna look at 
> > https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp#L7555
> >  as well.
> > 
> > Also I got the chance to look the rest of the patch, as I mentioned above 
> > you are already checking two methods iff one lies in the others base 
> > classes. So you can simply check for name equality and not being an 
> > overload case.
> Also wanted to bring up what @sammccall already mentioned on clangd-dev: we 
> probably shouldn't show methods that hide base methods rather than override 
> the virtual base methods (at least, by default).
> 
> Those might be useful, but unless we can have a clear UI indicator of whether 
> a method is overriding a virtual base method or is hiding some other method, 
> it would to add more confusion than benefit IMO.
> Also I got the chance to look the rest of the patch, as I mentioned above you 
> are already checking two methods iff one lies in the others base classes. So 
> you can simply check for name equality and not being an overload case.

To check for an overload case, you would use `Sema::IsOverload`?  

[PATCH] D51311: (WIP) Type hierarchy

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/Protocol.h:889
+  // Does this node implement the method targeted by the request?
+  bool DeclaresMethod;
+

kadircet wrote:
> I think comment and the name is in contradiction here, do you mean 
> DefinesMethod?
Actually I think the comment is wrong.  Even if we only see a declaration of 
the method, it's enough to say that this type has its own version of the method.



Comment at: clangd/XRefs.cpp:669
+  const CXXMethodDecl *Candidate) {
+  // FIXME: How do I determine if Method overrides Candidate?
+

kadircet wrote:
> https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp
> 
> you can check this sample, which simply traverses all base classes and gets 
> methods with the same name, then checks whether one is overload of the other. 
> If it they are not overloads then one in the base classes gets overriden by 
> the other.
> 
> 
> Another approach could be to search for one method in others 
> overriden_methods.
> 
> But they are both inefficient, I would suggest calling these functions only 
> when one of them is defined in the base class of other then you can just 
> check for name equality and not being an overload.
> https://github.com/llvm-mirror/clang/blob/master/lib/Sema/SemaDecl.cpp

Did you mean to link to a particular line?

> you can check this sample, which simply traverses all base classes and gets 
> methods with the same name, then checks whether one is overload of the other. 
> If it they are not overloads then one in the base classes gets overriden by 
> the other.

> Another approach could be to search for one method in others 
> overriden_methods.

I have tried using `overriden_methods`, but it only contains methods marked 
virtual.  For this feature, I would like to consider non-virtual methods too.

> But they are both inefficient, I would suggest calling these functions only 
> when one of them is defined in the base class of other then you can just 
> check for name equality and not being an overload.

I am not sure I understand, but maybe it will be clearer when I will see the 
sample.



Comment at: clangd/XRefs.cpp:693
+
+std::cerr << " >>> A parent is " << ParentDecl->getName().str()
+  << std::endl;

kadircet wrote:
> If you plan to keep it for later debugging info, consider using {d,v}log
Yes of course :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51311



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51311: (WIP) Type hierarchy

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, ioeric, 
ilya-biryukov.

This is a work in progress patch to support an eventual "get type
hierarchy" request that does not exist in the LSP today.  I only plan to
support getting parent types (i.e. base classes) for now, because it
doesn't require touching the index.  Finding child classes would come
later.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51311

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  clangd/XRefs.cpp
  clangd/XRefs.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -27,6 +27,7 @@
 
 namespace {
 using testing::ElementsAre;
+using testing::Eq;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
@@ -1068,6 +1069,166 @@
   ElementsAre(Location{FooCppUri, FooWithoutHeader.range()}));
 }
 
+TEST(TypeHierarchy, SimpleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+$ParentDef^struct Parent
+{
+  int a;
+};
+
+$Child1Def^struct Child1 : Parent
+{
+  int b;
+};
+
+struct Ch$p1^ild2 : Child1
+{
+  int c;
+};
+
+struct Child3 : Child2
+{
+  int d;
+};
+
+int main()
+{
+  Ch$p2^ild2 ch$p3^ild2;
+
+  parent.a = 1;
+  ch$p4^ild2.c = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  llvm::Optional Result;
+
+  TypeHierarchy ExpectedResult{{TypeHierarchyResult{
+  "Child1",
+  Source.point("Child1Def"),
+  false,
+  {TypeHierarchyResult{"Parent", Source.point("ParentDef"), false, {}};
+
+  for (auto Pt : {"p1", "p2", "p3", "p4"}) {
+Result = getTypeHierarchy(AST, Source.point(Pt));
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(*Result, Eq(ExpectedResult));
+  }
+}
+
+TEST(TypeHierarchy, MultipleInheritanceOnTypeOrVariable) {
+  Annotations Source(R"cpp(
+$Parent1Def^struct Parent1
+{
+  int a;
+};
+
+$Parent2Def^struct Parent2
+{
+  int b;
+};
+
+$Parent3Def^struct Parent3 : Parent2
+{
+  int c;
+};
+
+struct Ch$c1^ild : Parent1, Parent3
+{
+  int d;
+};
+
+int main()
+{
+  Ch$c2^ild  ch$c3^ild;
+
+  ch$c4^ild.a = 1;
+}
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  llvm::Optional Result;
+  TypeHierarchy ExpectedResult{{
+  TypeHierarchyResult{"Parent1", Source.point("Parent1Def"), false, {}},
+  TypeHierarchyResult{
+  "Parent3",
+  Source.point("Parent3Def"),
+  false,
+  {TypeHierarchyResult{
+  "Parent2", Source.point("Parent2Def"), false, {,
+  }};
+
+  for (auto Pt : {"c1", "c2", "c3", "c4"}) {
+Result = getTypeHierarchy(AST, Source.point(Pt));
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(*Result, Eq(ExpectedResult));
+  }
+}
+
+TEST(TypeHierarchy, OnMethod) {
+  Annotations Source(R"cpp(
+$ParentDef^struct Parent
+{
+  void method ();
+  void method () const;
+  void method (int x);
+  void method (char x);
+};
+
+$Child1Def^struct Child1 : Parent
+{
+  void method ();
+  void method (char x);
+};
+
+struct Child2 : Child1
+{
+  void met$p1^hod ();
+  void met$p2^hod (int x);
+};
+
+struct Child3 : Child2
+{
+  void method (int x);
+};
+)cpp");
+
+  TestTU TU = TestTU::withCode(Source.code());
+  auto AST = TU.build();
+
+  ASSERT_TRUE(AST.getDiagnostics().empty());
+
+  {
+TypeHierarchy ExpectedResult{{TypeHierarchyResult{
+"Child1",
+Source.point("Child1Def"),
+true,
+{TypeHierarchyResult{"Parent", Source.point("ParentDef"), true, {}};
+
+llvm::Optional Result =
+getTypeHierarchy(AST, Source.point("p1"));
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(*Result, Eq(ExpectedResult));
+  }
+
+  {
+TypeHierarchy ExpectedResult{{TypeHierarchyResult{
+"Child1",
+Source.point("Child1Def"),
+false,
+{TypeHierarchyResult{"Parent", Source.point("ParentDef"), true, {}};
+
+llvm::Optional Result =
+getTypeHierarchy(AST, Source.point("p2"));
+ASSERT_TRUE(bool(Result));
+EXPECT_THAT(*Result, Eq(ExpectedResult));
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/XRefs.h
===
--- clangd/XRefs.h
+++ clangd/XRefs.h
@@ -34,6 +34,9 @@
 /// Get the hover information when hovering at \p Pos.
 llvm::Optional getHover(ParsedAST , Position Pos);
 
+/// Get the type hierarchy information at \p Pos.
+llvm::Optional getTypeHierarchy(ParsedAST , Position Pos);
+
 } // namespace clangd
 } // namespace clang
 
Index: clangd/XRefs.cpp
===
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -17,6 

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: lib/Basic/FileManager.cpp:319
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
+  if (UFE.File) {
+if (auto Path = UFE.File->getName()) {

ioeric wrote:
> simark wrote:
> > ioeric wrote:
> > > simark wrote:
> > > > ioeric wrote:
> > > > > simark wrote:
> > > > > > What's the rationale for only computing the field if `UFE.File` is 
> > > > > > non-null?
> > > > > > 
> > > > > > Previously, if you looked up the file with `openFile == false` and 
> > > > > > then later `openFile == true`, the `RealPathName` field would not 
> > > > > > be set because of this.  That doesn't seem right.
> > > > > There has been no guarantee that RealFilePath is always set. I think 
> > > > > that's the reason why the acceasor is called tryGetRealPathName.
> > > > The way I understood it was that it could be empty because computing 
> > > > the real path can fail.  Not just because we didn't skipped computing 
> > > > it.
> > > I agree that the API is confusing and lack of documentation (and we 
> > > should fix), but the previous implementation did skip the computation if 
> > > File is not available though. 
> > > I agree that the API is confusing and lack of documentation (and we 
> > > should fix), but the previous implementation did skip the computation if 
> > > File is not available though.
> > 
> > Did it have a reason not to?  What is the `RealPathName` field useful for, 
> > if it's unreliable?
> I think the biggest concern is the performance.
>  
> For example, clangd code completion latency increased dramatically with 
> `real_path`:
> With `real_path`:
> {F7039629}
> {F7041680}
> 
> Wihtou `real_path`:
> {F7039630}
But with this patch, we are not using real_path anymore, so it can't be the 
reason for not computing `RealPathName` when not opening the file.  Since the 
non-real_path method is much more lightweight, would it make a significant 
difference if we always computed the field?

In the end I don't really mind, I am just looking for a rational explanation to 
why it's done this way.



Comment at: lib/Basic/FileManager.cpp:326
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
+}

ioeric wrote:
> simark wrote:
> > ioeric wrote:
> > > simark wrote:
> > > > ioeric wrote:
> > > > > simark wrote:
> > > > > > If the path contains symlinks, doesn't this put a non-real path in 
> > > > > > the RealPathName field?  Won't users (e.g. clangd) use this value 
> > > > > > thinking it is a real path, when it is actually not?
> > > > > This was the original behavior. In general, File Manager should never 
> > > > > call real_path for users because it can be very expensive. Users 
> > > > > should call real_path if they want to resolve symlinks. That said, 
> > > > > it's fair to say that "RealPathName" is just a wrong name, and we 
> > > > > should clean it up at some point.
> > > > Ok, then if the goal is not to actually have a real path (in the 
> > > > realpath(3) sense), that's fine.  But I think we should rename the 
> > > > field sooner than later, it's really confusing.
> > > > 
> > > > That also means that it's kind of useless for us in clangd, so we 
> > > > should always call real_path there and not rely on that field.
> > > > Ok, then if the goal is not to actually have a real path (in the 
> > > > realpath(3) sense), that's fine. But I think we should rename the field 
> > > > sooner than later, it's really confusing.
> > > +1
> > > 
> > > > That also means that it's kind of useless for us in clangd, so we 
> > > > should always call real_path there and not rely on that field.
> > > I guess it depends on whether you want symlink resolution or not. I know 
> > > that clangd's index symbol collector resolves symlink explicitly, but I 
> > > don't think clangd enforces symlink resolution in general?
> > > I guess it depends on whether you want symlink resolution or not. I know 
> > > that clangd's index symbol collector resolves symlink explicitly, but I 
> > > don't think clangd enforces symlink resolution in general?
> > 
> > If we don't, we probably risk having duplicate results similar to what
> > 
> >   https://reviews.llvm.org/D48687
> > 
> > fixed, by with paths differing because of symlinks instead of dot-dots.
> Was the bug addressed in D48687 actually caused by symlinks though? If want 
> we want is absolute paths with dotdot cleaned, it should be much cheaper to 
> call `VFS::makeAbsolutePath` with `remove_dot_dot`.
> 
> In general, it's unclear whether clangd should resolve symlink (it might not 
> always be what users want), and it should probably be a decision made by the 
> build system integration. I think we would need a much more careful design if 
> we decide to handle symlinks in clangd. 
> Was the bug addressed in D48687 actually 

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: lib/Basic/FileManager.cpp:319
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
+  if (UFE.File) {
+if (auto Path = UFE.File->getName()) {

ioeric wrote:
> simark wrote:
> > ioeric wrote:
> > > simark wrote:
> > > > What's the rationale for only computing the field if `UFE.File` is 
> > > > non-null?
> > > > 
> > > > Previously, if you looked up the file with `openFile == false` and then 
> > > > later `openFile == true`, the `RealPathName` field would not be set 
> > > > because of this.  That doesn't seem right.
> > > There has been no guarantee that RealFilePath is always set. I think 
> > > that's the reason why the acceasor is called tryGetRealPathName.
> > The way I understood it was that it could be empty because computing the 
> > real path can fail.  Not just because we didn't skipped computing it.
> I agree that the API is confusing and lack of documentation (and we should 
> fix), but the previous implementation did skip the computation if File is not 
> available though. 
> I agree that the API is confusing and lack of documentation (and we should 
> fix), but the previous implementation did skip the computation if File is not 
> available though.

Did it have a reason not to?  What is the `RealPathName` field useful for, if 
it's unreliable?



Comment at: lib/Basic/FileManager.cpp:326
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
+}

ioeric wrote:
> simark wrote:
> > ioeric wrote:
> > > simark wrote:
> > > > If the path contains symlinks, doesn't this put a non-real path in the 
> > > > RealPathName field?  Won't users (e.g. clangd) use this value thinking 
> > > > it is a real path, when it is actually not?
> > > This was the original behavior. In general, File Manager should never 
> > > call real_path for users because it can be very expensive. Users should 
> > > call real_path if they want to resolve symlinks. That said, it's fair to 
> > > say that "RealPathName" is just a wrong name, and we should clean it up 
> > > at some point.
> > Ok, then if the goal is not to actually have a real path (in the 
> > realpath(3) sense), that's fine.  But I think we should rename the field 
> > sooner than later, it's really confusing.
> > 
> > That also means that it's kind of useless for us in clangd, so we should 
> > always call real_path there and not rely on that field.
> > Ok, then if the goal is not to actually have a real path (in the 
> > realpath(3) sense), that's fine. But I think we should rename the field 
> > sooner than later, it's really confusing.
> +1
> 
> > That also means that it's kind of useless for us in clangd, so we should 
> > always call real_path there and not rely on that field.
> I guess it depends on whether you want symlink resolution or not. I know that 
> clangd's index symbol collector resolves symlink explicitly, but I don't 
> think clangd enforces symlink resolution in general?
> I guess it depends on whether you want symlink resolution or not. I know that 
> clangd's index symbol collector resolves symlink explicitly, but I don't 
> think clangd enforces symlink resolution in general?

If we don't, we probably risk having duplicate results similar to what

  https://reviews.llvm.org/D48687

fixed, by with paths differing because of symlinks instead of dot-dots.


Repository:
  rC Clang

https://reviews.llvm.org/D51159



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: lib/Basic/FileManager.cpp:319
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
+  if (UFE.File) {
+if (auto Path = UFE.File->getName()) {

ioeric wrote:
> simark wrote:
> > What's the rationale for only computing the field if `UFE.File` is non-null?
> > 
> > Previously, if you looked up the file with `openFile == false` and then 
> > later `openFile == true`, the `RealPathName` field would not be set because 
> > of this.  That doesn't seem right.
> There has been no guarantee that RealFilePath is always set. I think that's 
> the reason why the acceasor is called tryGetRealPathName.
The way I understood it was that it could be empty because computing the real 
path can fail.  Not just because we didn't skipped computing it.



Comment at: lib/Basic/FileManager.cpp:326
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
+}

ioeric wrote:
> simark wrote:
> > If the path contains symlinks, doesn't this put a non-real path in the 
> > RealPathName field?  Won't users (e.g. clangd) use this value thinking it 
> > is a real path, when it is actually not?
> This was the original behavior. In general, File Manager should never call 
> real_path for users because it can be very expensive. Users should call 
> real_path if they want to resolve symlinks. That said, it's fair to say that 
> "RealPathName" is just a wrong name, and we should clean it up at some point.
Ok, then if the goal is not to actually have a real path (in the realpath(3) 
sense), that's fine.  But I think we should rename the field sooner than later, 
it's really confusing.

That also means that it's kind of useless for us in clangd, so we should always 
call real_path there and not rely on that field.


Repository:
  rC Clang

https://reviews.llvm.org/D51159



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51159: [FileManager] Do not call 'real_path' in getFile().

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: lib/Basic/FileManager.cpp:319
 
-  SmallString<128> RealPathName;
-  if (!FS->getRealPath(InterndFileName, RealPathName))
-UFE.RealPathName = RealPathName.str();
+  if (UFE.File) {
+if (auto Path = UFE.File->getName()) {

What's the rationale for only computing the field if `UFE.File` is non-null?

Previously, if you looked up the file with `openFile == false` and then later 
`openFile == true`, the `RealPathName` field would not be set because of this.  
That doesn't seem right.



Comment at: lib/Basic/FileManager.cpp:326
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE.RealPathName = AbsPath.str();
+}

If the path contains symlinks, doesn't this put a non-real path in the 
RealPathName field?  Won't users (e.g. clangd) use this value thinking it is a 
real path, when it is actually not?


Repository:
  rC Clang

https://reviews.llvm.org/D51159



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/SourceCode.h:65
 
 /// Get the absolute file path of a given file entry.
 TextEdit toTextEdit(const FixItHint , const SourceManager ,

hokein wrote:
> nit: this comment is not needed.
Woops, merge mistake.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE339483: [clangd] Avoid duplicates in findDefinitions 
response (authored by simark, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48687?vs=160175=160197#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687

Files:
  clangd/FindSymbols.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestFS.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -312,27 +312,65 @@
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
+  // The source is in "/clangd-test/src".
+  // We build in "/clangd-test/build".
+
   Annotations SourceAnnotations(R"cpp(
+#include "header_in_preamble.h"
 int [[foo]];
-int baz = f^oo;
+#include "header_not_in_preamble.h"
+int baz = f$p1^oo + bar_pre$p2^amble + bar_not_pre$p3^amble;
+)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
+)cpp");
+
+  Annotations HeaderNotInPreambleAnnotations(R"cpp(
+int [[bar_not_preamble]];
 )cpp");
 
+  // Make the compilation paths appear as ../src/foo.cpp in the compile
+  // commands.
+  SmallString<32> RelPathPrefix("..");
+  llvm::sys::path::append(RelPathPrefix, "src");
+  std::string BuildDir = testPath("build");
+  MockCompilationDatabase CDB(BuildDir, RelPathPrefix);
+
   IgnoreDiagnostics DiagConsumer;
-  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
-  auto FooCpp = testPath("foo.cpp");
+  // Fill the filesystem.
+  auto FooCpp = testPath("src/foo.cpp");
   FS.Files[FooCpp] = "";
+  auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
+  FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
+  auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
+  FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
-  Server.addDocument(FooCpp, SourceAnnotations.code());
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
+
+  // Go to a definition in main source file.
   auto Locations =
-  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-
   EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
SourceAnnotations.range()}));
+
+  // Go to a definition in header_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderInPreambleH},
+   HeaderInPreambleAnnotations.range()}));
+
+  // Go to a definition in header_not_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+   HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -32,22 +32,36 @@
   return MemFS;
 }
 
-MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
-: ExtraClangFlags({"-ffreestanding"}), UseRelPaths(UseRelPaths) {
+MockCompilationDatabase::MockCompilationDatabase(StringRef Directory,
+ StringRef RelPathPrefix)
+: ExtraClangFlags({"-ffreestanding"}), Directory(Directory),
+  RelPathPrefix(RelPathPrefix) {
   // -ffreestanding avoids implicit stdc-predef.h.
 }
 
 Optional
 MockCompilationDatabase::getCompileCommand(PathRef File) const {
   if (ExtraClangFlags.empty())
 return None;
 
-  auto CommandLine = ExtraClangFlags;
   auto FileName = sys::path::filename(File);
+
+  // Build the compile command.
+  auto CommandLine = ExtraClangFlags;
   CommandLine.insert(CommandLine.begin(), "clang");
-  CommandLine.insert(CommandLine.end(), UseRelPaths ? FileName : File);
-  return {tooling::CompileCommand(sys::path::parent_path(File), FileName,
-  std::move(CommandLine), "")};
+  if (RelPathPrefix.empty()) {
+// Use the absolute path in the compile command.
+CommandLine.push_back(File);
+  } else {
+// Build a relative path using RelPathPrefix.
+SmallString<32> RelativeFilePath(RelPathPrefix);
+llvm::sys::path::append(RelativeFilePath, FileName);

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 160175.
simark marked 4 inline comments as done.
simark added a comment.

Latest update.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687

Files:
  clangd/FindSymbols.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestFS.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -312,27 +312,65 @@
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
+  // The source is in "/clangd-test/src".
+  // We build in "/clangd-test/build".
+
   Annotations SourceAnnotations(R"cpp(
+#include "header_in_preamble.h"
 int [[foo]];
-int baz = f^oo;
+#include "header_not_in_preamble.h"
+int baz = f$p1^oo + bar_pre$p2^amble + bar_not_pre$p3^amble;
+)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
+)cpp");
+
+  Annotations HeaderNotInPreambleAnnotations(R"cpp(
+int [[bar_not_preamble]];
 )cpp");
 
+  // Make the compilation paths appear as ../src/foo.cpp in the compile
+  // commands.
+  SmallString<32> RelPathPrefix("..");
+  llvm::sys::path::append(RelPathPrefix, "src");
+  std::string BuildDir = testPath("build");
+  MockCompilationDatabase CDB(BuildDir, RelPathPrefix);
+
   IgnoreDiagnostics DiagConsumer;
-  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
-  auto FooCpp = testPath("foo.cpp");
+  // Fill the filesystem.
+  auto FooCpp = testPath("src/foo.cpp");
   FS.Files[FooCpp] = "";
+  auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
+  FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
+  auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
+  FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
-  Server.addDocument(FooCpp, SourceAnnotations.code());
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
+
+  // Go to a definition in main source file.
   auto Locations =
-  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-
   EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
SourceAnnotations.range()}));
+
+  // Go to a definition in header_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderInPreambleH},
+   HeaderInPreambleAnnotations.range()}));
+
+  // Go to a definition in header_not_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+   HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {
Index: unittests/clangd/TestFS.h
===
--- unittests/clangd/TestFS.h
+++ unittests/clangd/TestFS.h
@@ -40,15 +40,22 @@
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
-  /// When \p UseRelPaths is true, uses relative paths in compile commands.
-  /// When \p UseRelPaths is false, uses absoulte paths.
-  MockCompilationDatabase(bool UseRelPaths = false);
+  /// If \p Directory is not empty, use that as the Directory field of the
+  /// CompileCommand.
+  ///
+  /// If \p RelPathPrefix is not empty, use that as a prefix in front of the
+  /// source file name, instead of using an absolute path.
+  MockCompilationDatabase(StringRef Directory = StringRef(),
+  StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
   getCompileCommand(PathRef File) const override;
 
   std::vector ExtraClangFlags;
-  const bool UseRelPaths;
+
+private:
+  StringRef Directory;
+  StringRef RelPathPrefix;
 };
 
 // Returns an absolute (fake) test directory for this OS.
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -32,22 +32,36 @@
   return MemFS;
 }
 
-MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
-: ExtraClangFlags({"-ffreestanding"}), UseRelPaths(UseRelPaths) {
+MockCompilationDatabase::MockCompilationDatabase(StringRef Directory,
+ StringRef RelPathPrefix)
+: 

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D48687#1195840, @hokein wrote:

> Looks good with a few nits. Looks like you didn't update the patch correctly. 
> You have marked comments done, but your code doesn't get changed accordingly. 
> Please double check with it.
>
> I tried your patch and it did fix the duplicated issue I encountered before. 
> Thanks for fixing it!


Ah sorry, I applied the fixes locally, but was waiting for Ilya's feedback for 
the function comment.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark marked 8 inline comments as done.
simark added inline comments.



Comment at: clangd/SourceCode.h:69
 
+llvm::Optional getRealPath(const FileEntry *F,
+const SourceManager );

ilya-biryukov wrote:
> This function looks like a good default choice for normalizing paths before 
> putting them into LSP structs, ClangdServer responses, etc.
> I suggest we add a small comment here with a guideline for everyone to 
> attempt using it whenever possible. WDYT?
What about this:

```
/// Get the real/canonical path of \p F.  This means:
///
///   - Absolute path
///   - Symlinks resolved
///   - No "." or ".." component
///   - No duplicate or trailing directory separator
///
/// This function should be used when sending paths to clients, so that paths
/// are normalized as much as possible.
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 159972.
simark marked an inline comment as done.
simark added a comment.

Replace

  RelPathPrefix == StringRef()

with

  RelPathPrefix.empty()


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687

Files:
  clangd/FindSymbols.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestFS.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -312,27 +312,65 @@
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
+  // The source is in "/clangd-test/src".
+  // We build in "/clangd-test/build".
+
   Annotations SourceAnnotations(R"cpp(
+#include "header_in_preamble.h"
 int [[foo]];
-int baz = f^oo;
+#include "header_not_in_preamble.h"
+int baz = f$p1^oo + bar_pre$p2^amble + bar_not_pre$p3^amble;
+)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
+)cpp");
+
+  Annotations HeaderNotInPreambleAnnotations(R"cpp(
+int [[bar_not_preamble]];
 )cpp");
 
+  // Make the compilation paths appear as ../src/foo.cpp in the compile
+  // commands.
+  SmallString<32> RelPathPrefix("..");
+  llvm::sys::path::append(RelPathPrefix, "src");
+  std::string BuildDir = testPath("build");
+  MockCompilationDatabase CDB(BuildDir, RelPathPrefix);
+
   IgnoreDiagnostics DiagConsumer;
-  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
-  auto FooCpp = testPath("foo.cpp");
+  // Fill the filesystem.
+  auto FooCpp = testPath("src/foo.cpp");
   FS.Files[FooCpp] = "";
+  auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
+  FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
+  auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
+  FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
-  Server.addDocument(FooCpp, SourceAnnotations.code());
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
+
+  // Go to a definition in main source file.
   auto Locations =
-  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-
   EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
SourceAnnotations.range()}));
+
+  // Go to a definition in header_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderInPreambleH},
+   HeaderInPreambleAnnotations.range()}));
+
+  // Go to a definition in header_not_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+   HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {
Index: unittests/clangd/TestFS.h
===
--- unittests/clangd/TestFS.h
+++ unittests/clangd/TestFS.h
@@ -40,15 +40,22 @@
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
-  /// When \p UseRelPaths is true, uses relative paths in compile commands.
-  /// When \p UseRelPaths is false, uses absoulte paths.
-  MockCompilationDatabase(bool UseRelPaths = false);
+  /// If \p Directory is not null, use that as the Directory field of the
+  /// CompileCommand.
+  ///
+  /// If \p RelPathPrefix is not null, use that as a prefix in front of the
+  /// source file name, instead of using an absolute path.
+  MockCompilationDatabase(StringRef Directory = StringRef(),
+  StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
   getCompileCommand(PathRef File) const override;
 
   std::vector ExtraClangFlags;
-  const bool UseRelPaths;
+
+private:
+  StringRef Directory;
+  StringRef RelPathPrefix;
 };
 
 // Returns an absolute (fake) test directory for this OS.
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -32,22 +32,36 @@
   return MemFS;
 }
 
-MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
-: ExtraClangFlags({"-ffreestanding"}), UseRelPaths(UseRelPaths) {
+MockCompilationDatabase::MockCompilationDatabase(StringRef Directory,
+

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark marked 3 inline comments as done.
simark added a comment.

In https://reviews.llvm.org/D48687#1193470, @hokein wrote:

> Sorry for the delay. I thought there was a dependent patch of this patch, but 
> it seems resolved, right?
>
> A rough round of review.


Right, the patch this one depends on is in clang, and there does not seem to be 
regressions this time.

>> New version. I tried to share the code between this and SymbolCollector, but
>>  didn't find a good clean way. Do you have concrete suggestions on how to do
>>  this? Otherwise, would it be acceptable to merge it as-is?
> 
> Yeah, I'm fine with the current change. I was not aware of the 
> `getAbsoluteFilePath` has been pulled out to the `SourceCode.h`. I think the 
> SymbolCollector could use this function as well (but that's out of scope of 
> this patch).

Thanks.




Comment at: clangd/SourceCode.h:65
 /// Get the absolute file path of a given file entry.
-llvm::Optional getAbsoluteFilePath(const FileEntry *F,
-const SourceManager 
);
+llvm::Optional getRealPath(const FileEntry *F,
+const SourceManager );

hokein wrote:
> Why rename this function?
> 
>  Is it guaranteed that `RealPath` is always an absolute path?
Before, it only made sure that the path was absolute, now it goes a step 
further and makes it a "real path", resolving symlinks and removing `.` and 
`..`.  When we talk about a "real path", it refers to the unix realpath syscall:

http://man7.org/linux/man-pages/man3/realpath.3.html

So yes, the result is guaranteed to be absolute too.



Comment at: unittests/clangd/TestFS.cpp:52
   CommandLine.insert(CommandLine.begin(), "clang");
-  CommandLine.insert(CommandLine.end(), UseRelPaths ? FileName : File);
-  return {tooling::CompileCommand(sys::path::parent_path(File), FileName,
-  std::move(CommandLine), "")};
+  if (RelPathPrefix == StringRef()) {
+// Use the absolute path in the compile command.

hokein wrote:
> Can we use `RelPathPrefix.empty()` instead of comparing with `StringRef()`?
Done.



Comment at: unittests/clangd/XRefsTests.cpp:325
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];

hokein wrote:
> It seems that there is no difference between `HeaderInPreambleAnnotations` 
> and `HeaderNotInPreambleAnnotations` in the test. Both of them are treated 
> equally.
> 
The difference is that one is included in the main file as part of the preamble 
and the other is out of the preamble.  Since they take different code paths in 
clang, I think it's good to test both.  At some point, I had a similar bug that 
would only happen with a header included in the preamble.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 159768.
simark added a comment.

Formatting.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687

Files:
  clangd/FindSymbols.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestFS.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -312,27 +312,65 @@
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
+  // The source is in "/clangd-test/src".
+  // We build in "/clangd-test/build".
+
   Annotations SourceAnnotations(R"cpp(
+#include "header_in_preamble.h"
 int [[foo]];
-int baz = f^oo;
+#include "header_not_in_preamble.h"
+int baz = f$p1^oo + bar_pre$p2^amble + bar_not_pre$p3^amble;
+)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
+)cpp");
+
+  Annotations HeaderNotInPreambleAnnotations(R"cpp(
+int [[bar_not_preamble]];
 )cpp");
 
+  // Make the compilation paths appear as ../src/foo.cpp in the compile
+  // commands.
+  SmallString<32> RelPathPrefix("..");
+  llvm::sys::path::append(RelPathPrefix, "src");
+  std::string BuildDir = testPath("build");
+  MockCompilationDatabase CDB(BuildDir, RelPathPrefix);
+
   IgnoreDiagnostics DiagConsumer;
-  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
-  auto FooCpp = testPath("foo.cpp");
+  // Fill the filesystem.
+  auto FooCpp = testPath("src/foo.cpp");
   FS.Files[FooCpp] = "";
+  auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
+  FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
+  auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
+  FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
-  Server.addDocument(FooCpp, SourceAnnotations.code());
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
+
+  // Go to a definition in main source file.
   auto Locations =
-  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-
   EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
SourceAnnotations.range()}));
+
+  // Go to a definition in header_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderInPreambleH},
+   HeaderInPreambleAnnotations.range()}));
+
+  // Go to a definition in header_not_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+   HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {
Index: unittests/clangd/TestFS.h
===
--- unittests/clangd/TestFS.h
+++ unittests/clangd/TestFS.h
@@ -40,15 +40,22 @@
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
-  /// When \p UseRelPaths is true, uses relative paths in compile commands.
-  /// When \p UseRelPaths is false, uses absoulte paths.
-  MockCompilationDatabase(bool UseRelPaths = false);
+  /// If \p Directory is not null, use that as the Directory field of the
+  /// CompileCommand.
+  ///
+  /// If \p RelPathPrefix is not null, use that as a prefix in front of the
+  /// source file name, instead of using an absolute path.
+  MockCompilationDatabase(StringRef Directory = StringRef(),
+  StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
   getCompileCommand(PathRef File) const override;
 
   std::vector ExtraClangFlags;
-  const bool UseRelPaths;
+
+private:
+  StringRef Directory;
+  StringRef RelPathPrefix;
 };
 
 // Returns an absolute (fake) test directory for this OS.
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -32,22 +32,36 @@
   return MemFS;
 }
 
-MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
-: ExtraClangFlags({"-ffreestanding"}), UseRelPaths(UseRelPaths) {
+MockCompilationDatabase::MockCompilationDatabase(StringRef Directory,
+ StringRef RelPathPrefix)
+: ExtraClangFlags({"-ffreestanding"}), 

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 159764.
simark added a comment.
Herald added a subscriber: arphaman.

New version.  I tried to share the code between this and SymbolCollector, but
didn't find a good clean way.  Do you have concrete suggestions on how to do
this?  Otherwise, would it be acceptable to merge it as-is?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687

Files:
  clangd/FindSymbols.cpp
  clangd/SourceCode.cpp
  clangd/SourceCode.h
  clangd/XRefs.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestFS.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -312,27 +312,65 @@
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
+  // The source is in "/clangd-test/src".
+  // We build in "/clangd-test/build".
+
   Annotations SourceAnnotations(R"cpp(
+#include "header_in_preamble.h"
 int [[foo]];
-int baz = f^oo;
+#include "header_not_in_preamble.h"
+int baz = f$p1^oo + bar_pre$p2^amble + bar_not_pre$p3^amble;
+)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
+)cpp");
+
+  Annotations HeaderNotInPreambleAnnotations(R"cpp(
+int [[bar_not_preamble]];
 )cpp");
 
+  // Make the compilation paths appear as ../src/foo.cpp in the compile
+  // commands.
+  SmallString<32> RelPathPrefix("..");
+  llvm::sys::path::append(RelPathPrefix, "src");
+  std::string BuildDir = testPath("build");
+  MockCompilationDatabase CDB(BuildDir, RelPathPrefix);
+
   IgnoreDiagnostics DiagConsumer;
-  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
-  auto FooCpp = testPath("foo.cpp");
+  // Fill the filesystem.
+  auto FooCpp = testPath("src/foo.cpp");
   FS.Files[FooCpp] = "";
+  auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
+  FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
+  auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
+  FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
-  Server.addDocument(FooCpp, SourceAnnotations.code());
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
+
+  // Go to a definition in main source file.
   auto Locations =
-  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-
   EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
SourceAnnotations.range()}));
+
+  // Go to a definition in header_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderInPreambleH},
+   HeaderInPreambleAnnotations.range()}));
+
+  // Go to a definition in header_not_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+   HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {
Index: unittests/clangd/TestFS.h
===
--- unittests/clangd/TestFS.h
+++ unittests/clangd/TestFS.h
@@ -40,15 +40,22 @@
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
-  /// When \p UseRelPaths is true, uses relative paths in compile commands.
-  /// When \p UseRelPaths is false, uses absoulte paths.
-  MockCompilationDatabase(bool UseRelPaths = false);
+  /// If \p Directory is not null, use that as the Directory field of the
+  /// CompileCommand.
+  ///
+  /// If \p RelPathPrefix is not null, use that as a prefix in front of the
+  /// source file name, instead of using an absolute path.
+  MockCompilationDatabase(StringRef Directory = StringRef(),
+  StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
   getCompileCommand(PathRef File) const override;
 
   std::vector ExtraClangFlags;
-  const bool UseRelPaths;
+
+private:
+  StringRef Directory;
+  StringRef RelPathPrefix;
 };
 
 // Returns an absolute (fake) test directory for this OS.
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -32,22 +32,36 @@
   return MemFS;
 }
 
-MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
-: 

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC339063: [VirtualFileSystem] InMemoryFileSystem::status: 
Return a Status with the… (authored by simark, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48903?vs=158625=159401#toc

Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  test/PCH/case-insensitive-include.c
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: test/PCH/case-insensitive-include.c
===
--- test/PCH/case-insensitive-include.c
+++ test/PCH/case-insensitive-include.c
@@ -13,7 +13,7 @@
 // RUN: touch -r %t-dir/case-insensitive-include.h %t.copy
 // RUN: mv %t.copy %t-dir/case-insensitive-include.h
 
-// RUN: %clang_cc1 -fsyntax-only %s -include-pch %t.pch -I %t-dir -verify
+// RUN: %clang_cc1 -Wno-nonportable-include-path -fsyntax-only %s -include-pch %t.pch -I %t-dir -verify
 
 // expected-no-diagnostics
 
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -315,9 +315,11 @@
   UFE.InPCH = Data.InPCH;
   UFE.File = std::move(F);
   UFE.IsValid = true;
-  if (UFE.File)
-if (auto RealPathName = UFE.File->getName())
-  UFE.RealPathName = *RealPathName;
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE.RealPathName = RealPathName.str();
+
   return 
 }
 
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -474,12 +474,28 @@
   Status Stat;
   InMemoryNodeKind Kind;
 
+protected:
+  /// Return Stat.  This should only be used for internal/debugging use.  When
+  /// clients wants the Status of this node, they should use
+  /// \p getStatus(StringRef).
+  const Status () const { return Stat; }
+
 public:
   InMemoryNode(Status Stat, InMemoryNodeKind Kind)
   : Stat(std::move(Stat)), Kind(Kind) {}
   virtual ~InMemoryNode() = default;
 
-  const Status () const { return Stat; }
+  /// Return the \p Status for this node. \p RequestedName should be the name
+  /// through which the caller referred to this node. It will override
+  /// \p Status::Name in the return value, to mimic the behavior of \p RealFile.
+  Status getStatus(StringRef RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+  }
+
+  /// Get the filename of this node (the name without the directory part).
+  StringRef getFileName() const {
+return llvm::sys::path::filename(Stat.getName());
+  }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -504,14 +520,21 @@
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
+/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
+/// \p RealFile.
 class InMemoryFileAdaptor : public File {
   InMemoryFile 
+  /// The name to use when returning a Status for this file.
+  std::string RequestedName;
 
 public:
-  explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+  explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+  : Node(Node), RequestedName(std::move(RequestedName)) {}
 
-  llvm::ErrorOr status() override { return Node.getStatus(); }
+  llvm::ErrorOr status() override {
+return Node.getStatus(RequestedName);
+  }
 
   llvm::ErrorOr>
   getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
@@ -711,7 +734,7 @@
 llvm::ErrorOr InMemoryFileSystem::status(const Twine ) {
   auto Node = lookupInMemoryNode(*this, Root.get(), Path);
   if (Node)
-return (*Node)->getStatus();
+return (*Node)->getStatus(Path.str());
   return Node.getError();
 }
 
@@ -724,7 +747,8 @@
   // When we have a file provide a heap-allocated wrapper for the memory buffer
   // to match the ownership semantics for File.
   if (auto *F = dyn_cast(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(
+new detail::InMemoryFileAdaptor(*F, Path.str()));
 
   // FIXME: errc::not_a_file?
   return make_error_code(llvm::errc::invalid_argument);
@@ -736,21 +760,33 @@
 class InMemoryDirIterator : public clang::vfs::detail::DirIterImpl {
   detail::InMemoryDirectory::const_iterator I;
   detail::InMemoryDirectory::const_iterator E;
+  std::string RequestedDirName;
+
+  void setCurrentEntry() {
+if (I != E) {
+  SmallString<256> Path(RequestedDirName);
+  llvm::sys::path::append(Path, I->second->getFileName());
+  CurrentEntry = I->second->getStatus(Path);
+} else {
+  // When we're at the end, make CurrentEntry invalid and DirIterImpl will
+  // do the rest.
+

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D48903#1189056, @ilya-biryukov wrote:

> I see, thanks for the explanation.
>
> LGTM for the update revision, given we have confirmation the tests pass on 
> Windows.


Thanks, I'll push it, let's hope this time is the right time!


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D48903#1188566, @malaperle wrote:

> Both check-clang and check-clang-tools pass successfully for me on Windows 
> with the patch.


Awesome, thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50255: Add test for changing build configuration

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D50255#1187871, @arphaman wrote:

> Thanks! This LGTM.


Well, thanks for the hints!


Repository:
  rL LLVM

https://reviews.llvm.org/D50255



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50255: Add test for changing build configuration

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL338914: [clangd] Add test for changing build configuration 
(authored by simark, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D50255

Files:
  clang-tools-extra/trunk/test/clangd/compile-commands-path-in-initialize.test
  clang-tools-extra/trunk/test/clangd/compile-commands-path.test


Index: 
clang-tools-extra/trunk/test/clangd/compile-commands-path-in-initialize.test
===
--- clang-tools-extra/trunk/test/clangd/compile-commands-path-in-initialize.test
+++ clang-tools-extra/trunk/test/clangd/compile-commands-path-in-initialize.test
@@ -0,0 +1,35 @@
+# Test that we can set choose a configuration/build directly in the initialize
+# request.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": 
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-1/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-2/compile_commands.json
+
+# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
+
+# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
+# (with the extra slash in the front), so we add it here.
+# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
+
+# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"initializationOptions":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if
 !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 
1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message 
(\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() 
{}\n"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is one",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is two",
+---
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: clang-tools-extra/trunk/test/clangd/compile-commands-path.test
===
--- clang-tools-extra/trunk/test/clangd/compile-commands-path.test
+++ clang-tools-extra/trunk/test/clangd/compile-commands-path.test
@@ -0,0 +1,42 @@
+# Test that we can switch between configuration/build using the
+# workspace/didChangeConfiguration notification.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": 
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-1/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-2/compile_commands.json
+
+# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
+
+# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
+# (with the extra slash in the front), so we add it here.
+# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
+
+# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if
 !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 
1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message 
(\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() 
{}\n"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is not defined",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
+# CHECK:  

[PATCH] D50255: Add test for changing build configuration

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 159050.
simark added a comment.

The tests now work on Windows, I added some path adjustment steps.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50255

Files:
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/compile-commands-path.test


Index: test/clangd/compile-commands-path.test
===
--- /dev/null
+++ test/clangd/compile-commands-path.test
@@ -0,0 +1,42 @@
+# Test that we can switch between configuration/build using the
+# workspace/didChangeConfiguration notification.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": 
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-1/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-2/compile_commands.json
+
+# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
+
+# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
+# (with the extra slash in the front), so we add it here.
+# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
+
+# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if
 !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 
1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message 
(\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() 
{}\n"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is not defined",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is one",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is two",
+---
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: test/clangd/compile-commands-path-in-initialize.test
===
--- /dev/null
+++ test/clangd/compile-commands-path-in-initialize.test
@@ -0,0 +1,35 @@
+# Test that we can set choose a configuration/build directly in the initialize
+# request.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%/t.dir", "command": "c++ the-file.cpp", "file": 
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-1", "command": "c++ -DMACRO=1 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-1/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir/build-2", "command": "c++ -DMACRO=2 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-2/compile_commands.json
+
+# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
+
+# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
+# (with the extra slash in the front), so we add it here.
+# RUN: sed -e "s|file://\([A-Z]\):/|file:///\1:/|g" %t.test.1 > %t.test
+
+# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"initializationOptions":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if
 !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 
1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message 
(\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() 
{}\n"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is one",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
+# CHECK:   

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark requested review of this revision.
simark added a comment.

In https://reviews.llvm.org/D48903#1187596, @ilya-biryukov wrote:

> This revision got 'reopened' and is now in the list of accepted revision. 
> Should we close it again?


It got reverted a second time because it was breaking a test on Windows.  The 
new bit is the change to `test/PCH/case-insensitive-include.c`, so it would 
need review again.  If somebody else could run the tests on Windows, it would 
make me a bit more confident too.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50255: Add test for changing build configuration

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

This was not tested on windows yet.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50255



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50255: Add test for changing build configuration

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, arphaman, jkorous, ioeric, ilya-biryukov.

This patch adds tests for the two ways of changing build configuration
(pointing to a particular compile_commands.json):

- Through the workspace/didChangeConfiguration notification.
- Through the initialize request.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50255

Files:
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/compile-commands-path.test


Index: test/clangd/compile-commands-path.test
===
--- /dev/null
+++ test/clangd/compile-commands-path.test
@@ -0,0 +1,37 @@
+# Test that we can switch between configuration/build using the
+# workspace/didChangeConfiguration notification.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%t.dir", "command": "c++ the-file.cpp", "file": 
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%t.dir/build-1", "command": "c++ -DMACRO=1 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-1/compile_commands.json
+# RUN: echo '[{"directory": "%t.dir/build-2", "command": "c++ -DMACRO=2 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-2/compile_commands.json
+# RUN: sed -e "s|INPUT_DIR|%t.dir|g" %s > %t.test
+
+# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if
 !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 
1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message 
(\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() 
{}\n"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is not defined",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is one",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is two",
+---
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: test/clangd/compile-commands-path-in-initialize.test
===
--- /dev/null
+++ test/clangd/compile-commands-path-in-initialize.test
@@ -0,0 +1,30 @@
+# Test that we can set choose a configuration/build directly in the initialize
+# request.
+
+# RUN: rm -rf %t.dir/* && mkdir -p %t.dir
+# RUN: mkdir %t.dir/build-1
+# RUN: mkdir %t.dir/build-2
+# RUN: echo '[{"directory": "%t.dir", "command": "c++ the-file.cpp", "file": 
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%t.dir/build-1", "command": "c++ -DMACRO=1 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-1/compile_commands.json
+# RUN: echo '[{"directory": "%t.dir/build-2", "command": "c++ -DMACRO=2 
the-file.cpp", "file": "../the-file.cpp"}]' > 
%t.dir/build-2/compile_commands.json
+# RUN: sed -e "s|INPUT_DIR|%t.dir|g" %s > %t.test
+
+# RUN: clangd -lit-test < %t.test | FileCheck -strict-whitespace %t.test
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"initializationOptions":{"compilationDatabasePath":"INPUT_DIR/build-1"}}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file://INPUT_DIR/the-file.cpp","languageId":"cpp","version":1,"text":"#if
 !defined(MACRO)\n#pragma message (\"MACRO is not defined\")\n#elif MACRO == 
1\n#pragma message (\"MACRO is one\")\n#elif MACRO == 2\n#pragma message 
(\"MACRO is two\")\n#else\n#pragma message (\"woops\")\n#endif\nint main() 
{}\n"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is one",
+---
+{"jsonrpc":"2.0","id":0,"method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabasePath":"INPUT_DIR/build-2"}}}
+# CHECK:   "method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:   "params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:   {
+# CHECK-NEXT: "message": "MACRO is two",
+---
+{"jsonrpc":"2.0","id":1,"method":"shutdown"}


Index: 

[PATCH] D50154: [clangd] capitalize diagnostic messages if '-capitialize-diagnostic-text' is provided

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D50154#1185605, @sammccall wrote:

> Capitalizing sounds nice. +1 to just do it without an option.
>
> My favorite essay on this is
>  http://neugierig.org/software/blog/2018/07/options.html


Agreed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50154



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50154: [clangd] capitalize diagnostic messages if '-capitialize-diagnostic-text' is provided

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

That's fine with me.  I'll try it and time will tell if I like it or not :).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50154



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 158625.
simark added a comment.

Add -Wno-nonportable-include-path to test/PCH/case-insensitive-include.c


Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  test/PCH/case-insensitive-include.c
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -13,6 +13,7 @@
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/SourceMgr.h"
 #include "gtest/gtest.h"
 #include 
@@ -151,6 +152,13 @@
 addEntry(Path, S);
   }
 };
+
+/// Replace back-slashes by front-slashes.
+std::string getPosixPath(std::string S) {
+  SmallString<128> Result;
+  llvm::sys::path::native(S, Result, llvm::sys::path::Style::posix);
+  return Result.str();
+};
 } // end anonymous namespace
 
 TEST(VirtualFileSystemTest, StatusQueries) {
@@ -782,7 +790,9 @@
 
   I = FS.dir_begin("/b", EC);
   ASSERT_FALSE(EC);
-  ASSERT_EQ("/b/c", I->getName());
+  // When on Windows, we end up with "/b\\c" as the name.  Convert to Posix
+  // path for the sake of the comparison.
+  ASSERT_EQ("/b/c", getPosixPath(I->getName()));
   I.increment(EC);
   ASSERT_FALSE(EC);
   ASSERT_EQ(vfs::directory_iterator(), I);
@@ -794,23 +804,19 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
 
-  auto ReplaceBackslashes = [](std::string S) {
-std::replace(S.begin(), S.end(), '\\', '/');
-return S;
-  };
   NormalizedFS.setCurrentWorkingDirectory("/b/c");
   NormalizedFS.setCurrentWorkingDirectory(".");
-  ASSERT_EQ("/b/c", ReplaceBackslashes(
-NormalizedFS.getCurrentWorkingDirectory().get()));
+  ASSERT_EQ("/b/c",
+getPosixPath(NormalizedFS.getCurrentWorkingDirectory().get()));
   NormalizedFS.setCurrentWorkingDirectory("..");
-  ASSERT_EQ("/b", ReplaceBackslashes(
-  NormalizedFS.getCurrentWorkingDirectory().get()));
+  ASSERT_EQ("/b",
+getPosixPath(NormalizedFS.getCurrentWorkingDirectory().get()));
 }
 
 #if !defined(_WIN32)
@@ -919,6 +925,39 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using a directory iterator.
+  std::error_code EC;
+  clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
+  // When on Windows, we end up with "../b\\c" as the name.  Convert to Posix
+  // path for the sake of the comparison.
+  ASSERT_EQ("../b/c", getPosixPath(It->getName()));
+}
+
 // NOTE: in the tests below, we use '//root/' as our root directory, since it is
 // a legal *absolute* path on Windows as well as *nix.
 class VFSFromYAMLTest : public ::testing::Test {
Index: 

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark added a subscriber: eric_niebler.
simark added a comment.

@eric_niebler, question for you:

This patch causes clang to emit a `-Wnonportable-include-path` warning where it 
did not before.  It affects the following test on Windows:
https://github.com/llvm-mirror/clang/blob/master/test/PCH/case-insensitive-include.c

The warning is currently not emitted for the **last** clang invocation (the one 
that includes PCH), because the real path value is not available, and therefore 
this condition is false:
https://github.com/llvm-mirror/clang/blob/fe1098c84823b8eac46b0bfffc5f5788b6c26d1a/lib/Lex/PPDirectives.cpp#L2015

With this patch, the real path value is available, so we emit the warning.  Is 
it on purpose that this warning is not emitted in this case?  If not should I 
simply add `-Wno-nonportable-include-path` to the last clang invocation, as was 
done earlier with the first invocation?

It seems to me like the warning is valid, even though we use precompiled 
headers.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

[Simon Marchi via Phabricator via cfe-commits]

simark marked 2 inline comments as done.
simark added a comment.

Thanks, done.


Repository:
  rL LLVM

https://reviews.llvm.org/D49833



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL338518: [clangd] Receive compilationDatabasePath in 
initialize request (authored by simark, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49833?vs=158366=158503#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49833

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/ClangdLSPServer.h
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h

Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.h
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.h
@@ -82,6 +82,7 @@
   /// may be very expensive.  This method is normally called when the
   /// compilation database is changed.
   void reparseOpenedFiles();
+  void applyConfiguration(const ClangdConfigurationParamsChange );
 
   JSONOutput 
   /// Used to indicate that the 'shutdown' request was received from the
Index: clang-tools-extra/trunk/clangd/Protocol.cpp
===
--- clang-tools-extra/trunk/clangd/Protocol.cpp
+++ clang-tools-extra/trunk/clangd/Protocol.cpp
@@ -263,7 +263,7 @@
   O.map("rootPath", R.rootPath);
   O.map("capabilities", R.capabilities);
   O.map("trace", R.trace);
-  // initializationOptions, capabilities unused
+  O.map("initializationOptions", R.initializationOptions);
   return true;
 }
 
Index: clang-tools-extra/trunk/clangd/Protocol.h
===
--- clang-tools-extra/trunk/clangd/Protocol.h
+++ clang-tools-extra/trunk/clangd/Protocol.h
@@ -322,6 +322,16 @@
 
 bool fromJSON(const llvm::json::Value &, ClientCapabilities &);
 
+/// Clangd extension to set clangd-specific "initializationOptions" in the
+/// "initialize" request and for the "workspace/didChangeConfiguration"
+/// notification since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};
+bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
+
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {};
+
 struct InitializeParams {
   /// The process Id of the parent process that started
   /// the server. Is null if the process has not been started by another
@@ -348,6 +358,10 @@
 
   /// The initial trace setting. If omitted trace is disabled ('off').
   llvm::Optional trace;
+
+  // We use this predefined struct because it is easier to use
+  // than the protocol specified type of 'any'.
+  llvm::Optional initializationOptions;
 };
 bool fromJSON(const llvm::json::Value &, InitializeParams &);
 
@@ -419,13 +433,6 @@
 };
 bool fromJSON(const llvm::json::Value &, DidChangeWatchedFilesParams &);
 
-/// Clangd extension to manage a workspace/didChangeConfiguration notification
-/// since the data received is described as 'any' type in LSP.
-struct ClangdConfigurationParamsChange {
-  llvm::Optional compilationDatabasePath;
-};
-bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
-
 struct DidChangeConfigurationParams {
   // We use this predefined struct because it is easier to use
   // than the protocol specified type of 'any'.
Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
@@ -72,6 +72,9 @@
 } // namespace
 
 void ClangdLSPServer::onInitialize(InitializeParams ) {
+  if (Params.initializationOptions)
+applyConfiguration(*Params.initializationOptions);
+
   if (Params.rootUri && *Params.rootUri)
 Server.setRootPath(Params.rootUri->file());
   else if (Params.rootPath && !Params.rootPath->empty())
@@ -398,11 +401,8 @@
});
 }
 
-// FIXME: This function needs to be properly tested.
-void ClangdLSPServer::onChangeConfiguration(
-DidChangeConfigurationParams ) {
-  ClangdConfigurationParamsChange  = Params.settings;
-
+void ClangdLSPServer::applyConfiguration(
+const ClangdConfigurationParamsChange ) {
   // Compilation database change.
   if (Settings.compilationDatabasePath.hasValue()) {
 NonCachedCDB.setCompileCommandsDir(
@@ -413,6 +413,12 @@
   }
 }
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(
+DidChangeConfigurationParams ) {
+  applyConfiguration(Params.settings);
+}
+
 ClangdLSPServer::ClangdLSPServer(JSONOutput ,
  const clangd::CodeCompleteOptions ,
  llvm::Optional CompileCommandsDir,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 158366.
simark added a comment.

Address Ilya's comment (add an indirection for initializationOptions).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49833

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h

Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -322,6 +322,16 @@
 
 bool fromJSON(const llvm::json::Value &, ClientCapabilities &);
 
+/// Clangd extension to set clangd-specific "initializationOptions" in the
+/// "initialize" request and for the "workspace/didChangeConfiguration"
+/// notification since the data received is described as 'any' type in LSP.
+struct ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};
+bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
+
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {};
+
 struct InitializeParams {
   /// The process Id of the parent process that started
   /// the server. Is null if the process has not been started by another
@@ -348,6 +358,10 @@
 
   /// The initial trace setting. If omitted trace is disabled ('off').
   llvm::Optional trace;
+
+  // We use this predefined struct because it is easier to use
+  // than the protocol specified type of 'any'.
+  llvm::Optional initializationOptions;
 };
 bool fromJSON(const llvm::json::Value &, InitializeParams &);
 
@@ -419,13 +433,6 @@
 };
 bool fromJSON(const llvm::json::Value &, DidChangeWatchedFilesParams &);
 
-/// Clangd extension to manage a workspace/didChangeConfiguration notification
-/// since the data received is described as 'any' type in LSP.
-struct ClangdConfigurationParamsChange {
-  llvm::Optional compilationDatabasePath;
-};
-bool fromJSON(const llvm::json::Value &, ClangdConfigurationParamsChange &);
-
 struct DidChangeConfigurationParams {
   // We use this predefined struct because it is easier to use
   // than the protocol specified type of 'any'.
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -263,7 +263,7 @@
   O.map("rootPath", R.rootPath);
   O.map("capabilities", R.capabilities);
   O.map("trace", R.trace);
-  // initializationOptions, capabilities unused
+  O.map("initializationOptions", R.initializationOptions);
   return true;
 }
 
Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -82,6 +82,7 @@
   /// may be very expensive.  This method is normally called when the
   /// compilation database is changed.
   void reparseOpenedFiles();
+  void applyConfiguration(const ClangdConfigurationParamsChange );
 
   JSONOutput 
   /// Used to indicate that the 'shutdown' request was received from the
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -72,6 +72,9 @@
 } // namespace
 
 void ClangdLSPServer::onInitialize(InitializeParams ) {
+  if (Params.initializationOptions)
+applyConfiguration(*Params.initializationOptions);
+
   if (Params.rootUri && *Params.rootUri)
 Server.setRootPath(Params.rootUri->file());
   else if (Params.rootPath && !Params.rootPath->empty())
@@ -398,11 +401,8 @@
});
 }
 
-// FIXME: This function needs to be properly tested.
-void ClangdLSPServer::onChangeConfiguration(
-DidChangeConfigurationParams ) {
-  ClangdConfigurationParamsChange  = Params.settings;
-
+void ClangdLSPServer::applyConfiguration(
+const ClangdConfigurationParamsChange ) {
   // Compilation database change.
   if (Settings.compilationDatabasePath.hasValue()) {
 NonCachedCDB.setCompileCommandsDir(
@@ -413,6 +413,13 @@
   }
 }
 
+// FIXME: This function needs to be properly tested.
+void ClangdLSPServer::onChangeConfiguration(
+DidChangeConfigurationParams ) {
+  ClangdConfigurationParamsChange  = Params.settings;
+  applyConfiguration(Settings);
+}
+
 ClangdLSPServer::ClangdLSPServer(JSONOutput ,
  const clangd::CodeCompleteOptions ,
  llvm::Optional CompileCommandsDir,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49833: [clangd] Receive compilationDatabasePath in 'initialize' request

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D49833#1182615, @malaperle wrote:

> @simark would you mind finishing this patch and committing it? I won't be 
> able to finish it given that I started it at a different company, etc.


Yes, I'll take it over, thanks for getting it started.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49833



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC338057: [VirtualFileSystem] InMemoryFileSystem::status: 
Return a Status with the… (authored by simark, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48903?vs=157467=157548#toc

Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -315,9 +315,11 @@
   UFE.InPCH = Data.InPCH;
   UFE.File = std::move(F);
   UFE.IsValid = true;
-  if (UFE.File)
-if (auto RealPathName = UFE.File->getName())
-  UFE.RealPathName = *RealPathName;
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE.RealPathName = RealPathName.str();
+
   return 
 }
 
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -474,12 +474,28 @@
   Status Stat;
   InMemoryNodeKind Kind;
 
+protected:
+  /// Return Stat.  This should only be used for internal/debugging use.  When
+  /// clients wants the Status of this node, they should use
+  /// \p getStatus(StringRef).
+  const Status () const { return Stat; }
+
 public:
   InMemoryNode(Status Stat, InMemoryNodeKind Kind)
   : Stat(std::move(Stat)), Kind(Kind) {}
   virtual ~InMemoryNode() = default;
 
-  const Status () const { return Stat; }
+  /// Return the \p Status for this node. \p RequestedName should be the name
+  /// through which the caller referred to this node. It will override
+  /// \p Status::Name in the return value, to mimic the behavior of \p RealFile.
+  Status getStatus(StringRef RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+  }
+
+  /// Get the filename of this node (the name without the directory part).
+  StringRef getFileName() const {
+return llvm::sys::path::filename(Stat.getName());
+  }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -504,14 +520,21 @@
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
+/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
+/// \p RealFile.
 class InMemoryFileAdaptor : public File {
   InMemoryFile 
+  /// The name to use when returning a Status for this file.
+  std::string RequestedName;
 
 public:
-  explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+  explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+  : Node(Node), RequestedName(std::move(RequestedName)) {}
 
-  llvm::ErrorOr status() override { return Node.getStatus(); }
+  llvm::ErrorOr status() override {
+return Node.getStatus(RequestedName);
+  }
 
   llvm::ErrorOr>
   getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
@@ -711,7 +734,7 @@
 llvm::ErrorOr InMemoryFileSystem::status(const Twine ) {
   auto Node = lookupInMemoryNode(*this, Root.get(), Path);
   if (Node)
-return (*Node)->getStatus();
+return (*Node)->getStatus(Path.str());
   return Node.getError();
 }
 
@@ -724,7 +747,8 @@
   // When we have a file provide a heap-allocated wrapper for the memory buffer
   // to match the ownership semantics for File.
   if (auto *F = dyn_cast(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(
+new detail::InMemoryFileAdaptor(*F, Path.str()));
 
   // FIXME: errc::not_a_file?
   return make_error_code(llvm::errc::invalid_argument);
@@ -736,21 +760,33 @@
 class InMemoryDirIterator : public clang::vfs::detail::DirIterImpl {
   detail::InMemoryDirectory::const_iterator I;
   detail::InMemoryDirectory::const_iterator E;
+  std::string RequestedDirName;
+
+  void setCurrentEntry() {
+if (I != E) {
+  SmallString<256> Path(RequestedDirName);
+  llvm::sys::path::append(Path, I->second->getFileName());
+  CurrentEntry = I->second->getStatus(Path);
+} else {
+  // When we're at the end, make CurrentEntry invalid and DirIterImpl will
+  // do the rest.
+  CurrentEntry = Status();
+}
+  }
 
 public:
   InMemoryDirIterator() = default;
 
-  explicit InMemoryDirIterator(detail::InMemoryDirectory )
-  : I(Dir.begin()), E(Dir.end()) {
-if (I != E)
-  CurrentEntry = I->second->getStatus();
+  explicit InMemoryDirIterator(detail::InMemoryDirectory ,
+   std::string RequestedDirName)
+  : I(Dir.begin()), E(Dir.end()),
+RequestedDirName(std::move(RequestedDirName)) {
+setCurrentEntry();
   }
 
   std::error_code increment() override {
 ++I;
-// When we're at 

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 157467.
simark marked an inline comment as done.
simark added a comment.

I think this addresses all of Ilya's comments.


Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -13,6 +13,7 @@
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/SourceMgr.h"
 #include "gtest/gtest.h"
 #include 
@@ -151,6 +152,13 @@
 addEntry(Path, S);
   }
 };
+
+/// Replace back-slashes by front-slashes.
+std::string getPosixPath(std::string S) {
+  SmallString<128> Result;
+  llvm::sys::path::native(S, Result, llvm::sys::path::Style::posix);
+  return Result.str();
+};
 } // end anonymous namespace
 
 TEST(VirtualFileSystemTest, StatusQueries) {
@@ -782,7 +790,9 @@
 
   I = FS.dir_begin("/b", EC);
   ASSERT_FALSE(EC);
-  ASSERT_EQ("/b/c", I->getName());
+  // When on Windows, we end up with "/b\\c" as the name.  Convert to Posix
+  // path for the sake of the comparison.
+  ASSERT_EQ("/b/c", getPosixPath(I->getName()));
   I.increment(EC);
   ASSERT_FALSE(EC);
   ASSERT_EQ(vfs::directory_iterator(), I);
@@ -794,23 +804,19 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
 
-  auto ReplaceBackslashes = [](std::string S) {
-std::replace(S.begin(), S.end(), '\\', '/');
-return S;
-  };
   NormalizedFS.setCurrentWorkingDirectory("/b/c");
   NormalizedFS.setCurrentWorkingDirectory(".");
-  ASSERT_EQ("/b/c", ReplaceBackslashes(
-NormalizedFS.getCurrentWorkingDirectory().get()));
+  ASSERT_EQ("/b/c",
+getPosixPath(NormalizedFS.getCurrentWorkingDirectory().get()));
   NormalizedFS.setCurrentWorkingDirectory("..");
-  ASSERT_EQ("/b", ReplaceBackslashes(
-  NormalizedFS.getCurrentWorkingDirectory().get()));
+  ASSERT_EQ("/b",
+getPosixPath(NormalizedFS.getCurrentWorkingDirectory().get()));
 }
 
 #if !defined(_WIN32)
@@ -919,6 +925,39 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using a directory iterator.
+  std::error_code EC;
+  clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
+  // When on Windows, we end up with "../b\\c" as the name.  Convert to Posix
+  // path for the sake of the comparison.
+  ASSERT_EQ("../b/c", getPosixPath(It->getName()));
+}
+
 // NOTE: in the tests below, we use '//root/' as our root directory, since it is
 // a legal *absolute* path on Windows as well as *nix.
 class VFSFromYAMLTest : public ::testing::Test {
Index: lib/Basic/VirtualFileSystem.cpp

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark marked 7 inline comments as done.
simark added inline comments.



Comment at: lib/Basic/VirtualFileSystem.cpp:475
   InMemoryNodeKind Kind;
+  Status Stat;
+

ilya-biryukov wrote:
> NIT: maybe keep the order of members the same to keep the patch more focused? 
> Unless there's a good reason to swap them.
Oops, that was not intended.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49783: [clangd] Do not rebuild AST if inputs have not changed

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

Thanks, that's simple and efficient.  I'll update 
https://reviews.llvm.org/D49267 (to call `reparseOpenFiles` once again) once 
this is merged.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49783



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 157287.
simark added a comment.

Fix tests on Windows

Fix InMemoryFileSystem tests on Windows.  The paths returned by the
InMemoryFileSystem directory iterators in the tests mix posix and windows
directory separators.  THis is because we do queries with posix-style
separators ("/a/b") but filnames are appended using native-style separators
(backslash on Windows).  So we end up with "/a/b\c".

I fixed the test by re-using the ReplaceBackslashes function defined in another
test.  I'm not sure this is the best fix, but the only alternative I see would
be to completely rewrite tests to use posix-style paths on non-Windows and
Windows-style paths on Windows.  That would lead to quite a bit of
duplication...


Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -151,6 +151,11 @@
 addEntry(Path, S);
   }
 };
+
+auto ReplaceBackslashes = [](std::string S) {
+  std::replace(S.begin(), S.end(), '\\', '/');
+  return S;
+};
 } // end anonymous namespace
 
 TEST(VirtualFileSystemTest, StatusQueries) {
@@ -782,7 +787,7 @@
 
   I = FS.dir_begin("/b", EC);
   ASSERT_FALSE(EC);
-  ASSERT_EQ("/b/c", I->getName());
+  ASSERT_EQ("/b/c", ReplaceBackslashes(I->getName()));
   I.increment(EC);
   ASSERT_FALSE(EC);
   ASSERT_EQ(vfs::directory_iterator(), I);
@@ -794,16 +799,12 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
 
-  auto ReplaceBackslashes = [](std::string S) {
-std::replace(S.begin(), S.end(), '\\', '/');
-return S;
-  };
   NormalizedFS.setCurrentWorkingDirectory("/b/c");
   NormalizedFS.setCurrentWorkingDirectory(".");
   ASSERT_EQ("/b/c", ReplaceBackslashes(
@@ -919,6 +920,37 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using a directory iterator.
+  std::error_code EC;
+  clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
+  ASSERT_EQ("../b/c", ReplaceBackslashes(It->getName()));
+}
+
 // NOTE: in the tests below, we use '//root/' as our root directory, since it is
 // a legal *absolute* path on Windows as well as *nix.
 class VFSFromYAMLTest : public ::testing::Test {
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -471,15 +471,31 @@
 /// The in memory file system is a tree of Nodes. Every node can either be a
 /// file or a directory.
 class InMemoryNode {
-  Status Stat;
   InMemoryNodeKind Kind;
+  Status Stat;
+
+protected:
+  /// Return Stat.  This should only be used for 

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark reopened this revision.
simark added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D48903#1175317, @ioeric wrote:

> It would make it easier for your reviewers to look at the new changes if
>  you just reopen this patch and update the diff :)


I tried but didn't know how.  But I just saw the "Add Action" drop down with 
"Reopen" in it.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

I uploaded a new version of this patch here:
https://reviews.llvm.org/D49804


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49804: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name (take 2)

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
simark added reviewers: malaperle, ilya-biryukov, bkramer.

This is a new version of https://reviews.llvm.org/D48903, which has been 
reverted.  @ioeric fixed
the issues caused by this patch downstream, so he told me it was good to
go again.  I also fixed the test failures on Windows.  The issue is that
the paths returned by the InMemoryFileSystem directory iterators in the
tests mix posix and windows directory separators.  This is because we do
queries with posix-style separators ("/a/b") but filenames are appended
using native-style separators (backslash on Windows).  So we end up with
"/a/b\c".

I fixed the test by re-using the ReplaceBackslashes function defined in
another test.  I'm not sure this is the best fix, but the only
alternative I see would be to completely rewrite tests to use
posix-style paths on non-Windows and Windows-style paths on Windows.
That would lead to quite a bit of duplication...

Here's the original commit message:

InMemoryFileSystem::status behaves differently than
RealFileSystem::status.  The Name contained in the Status returned by
RealFileSystem::status will be the path as requested by the caller,
whereas InMemoryFileSystem::status returns the normalized path.

For example, when requested the status for "../src/first.h",
RealFileSystem returns a Status with "../src/first.h" as the Name.
InMemoryFileSystem returns "/absolute/path/to/src/first.h".

The reason for this change is that I want to make a unit test in the
clangd testsuite (where we use an InMemoryFileSystem) to reproduce a
bug I get with the clangd program (where a RealFileSystem is used).
This difference in behavior "hides" the bug in the unit test version.


Repository:
  rC Clang

https://reviews.llvm.org/D49804

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -151,6 +151,11 @@
 addEntry(Path, S);
   }
 };
+
+auto ReplaceBackslashes = [](std::string S) {
+  std::replace(S.begin(), S.end(), '\\', '/');
+  return S;
+};
 } // end anonymous namespace
 
 TEST(VirtualFileSystemTest, StatusQueries) {
@@ -782,7 +787,7 @@
 
   I = FS.dir_begin("/b", EC);
   ASSERT_FALSE(EC);
-  ASSERT_EQ("/b/c", I->getName());
+  ASSERT_EQ("/b/c", ReplaceBackslashes(I->getName()));
   I.increment(EC);
   ASSERT_FALSE(EC);
   ASSERT_EQ(vfs::directory_iterator(), I);
@@ -794,16 +799,12 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
 
-  auto ReplaceBackslashes = [](std::string S) {
-std::replace(S.begin(), S.end(), '\\', '/');
-return S;
-  };
   NormalizedFS.setCurrentWorkingDirectory("/b/c");
   NormalizedFS.setCurrentWorkingDirectory(".");
   ASSERT_EQ("/b/c", ReplaceBackslashes(
@@ -919,6 +920,37 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< 

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:430
 CDB.clear();
-
-reparseOpenedFiles();
+compileCommandsChangePost(CCChangeData);
   }

malaperle wrote:
> ilya-biryukov wrote:
> > Maybe keep the old logic of reparsing all open files? This would make the 
> > change way simpler and I don't think we need this extra complexity in the 
> > long run, when we have better integration with the build system.
> > 
> > ClangdServer will reuse the preamble if compile command didn't change 
> > anyway, so reparse will be very fast and shouldn't be affected.
> > If the compile command does change, we'll retrigger the full rebuild.
> I think the change is not that complex but brings much added value. About the 
> integration with the build system, there are many build systems out there so 
> I don't think better integration will be useful in many scenarios (plain 
> make, custom builds, etc). This solution is generic enough so that any build 
> system that generates compile_commands.json will be supported in a pretty 
> good way.
@malaperle also suggested an alternative way of doing it.  Instead of saving 
the the compile commands in a custom structure before clearing the cache, we 
could save the compile flags in the `ParsedAST` object.  When some 
compile_commands.json changes, we can compare the new compile flags with this 
saved copy.  I think it would make the code a bit more straightforward.



Comment at: clangd/clients/clangd-vscode/src/extension.ts:35
 ['cpp', 'c', 'cc', 'cxx', 'c++', 'm', 'mm', 'h', 'hh', 'hpp', 'hxx', 
'inc'].join() + '}';
+const compileCommandsFilePattern: string = '**/compile_commands.json';
 const clientOptions: vscodelc.LanguageClientOptions = {

ilya-biryukov wrote:
> These watches apply to the children of the workspace root only, right?
> E.g., I quite often open the workspace in 
> 'llvm/tools/clang/tools/extra/clangd', would my `compile_commands.json`, 
> that's outside the workspace, be watched for changes?
You are right, I don't think files outside the workspace would get watched.

For your use case, we'll need clangd to watch (or ask the client to watch) the 
file.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D49267#1173266, @ilya-biryukov wrote:

> The approach is not ideal, but may be a good middle ground before we figure 
> out how we approach file watching in clangd. Note that there are other things 
> that won't force the updates currently, e.g. changes to the included headers 
> won't cause rebuilds of the source files until user modifies them. And those 
> are much more frequent than changes to compile_commands.json, so they seem 
> like a more pressing problem.


Ok, I agree that having clangd watch files itself could be necessary at some 
point (when the client does not support it), but it would have to be 
configurable.  In our case, we have efficient enough file watching in the 
client, and already watch the files in the workspace.  Since the inotify 
watches are limited per-user, having clangd watch them too means we'll have 
duplicates, and therefore waste a rather limited resource.

Actually, clangd could simply use the client capabilities. If the client 
advertises support for file watching with dynamic registration, use that, 
otherwise use the internal mechanism.

In the mean time, I don't think the current patch paints us in a corner.  The 
logic of checking whether the effective compile commands for open files changes 
would stay even if the file watching mechanism changes.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D49267#1171286, @ilya-biryukov wrote:

> Thanks for putting up this change! It can be really annoying that clangd does 
> not pick up the compile commands that got updated.
>
> A few things of the top of my head on why we might want to punt on using the 
> LSP watches:
>
> - File watching may not be implemented in the language server at all. That's 
> certainly the case for our hacked-up ycmd<->lsp bridge.


I guess you mean language client here, not language server.  In our case we 
already have a client capable of file watching, so it's convenient for us to do 
that (plus, this is what the LSP specification recommends).  If you want to 
make clangd capable of watching files natively, I am not against it, but it's 
not on our critical path.  As long as the file is watched, I'm happy.

Our client does not yet support file watch registration, that's why I did not 
implement it, but once we upgrade to a client that does, the plan is to make 
clangd ask the client to watch `**/compile_commands.json`.  Right now, we 
hardcoded in our client that we want to watch any file called 
`compile_commands.json`:

https://github.com/theia-ide/theia/pull/2405/commits/fe48e8d63f98edd879252238844a58f2cfa1

> - At some point we'll be interested in changes to the headers our sources 
> have included. This would involve a **lot** of watches for a dynamically 
> changing set of files. There's a good chance we'll have to consider using 
> native file watch APIs instead of the LSP methods to handle those (e.g., for 
> performance reasons).

What would be the performance bottleneck?  There would be a lot of watched 
files fore sure, but very little change events.

> We've been talking about starting to implement a better buildsystem 
> integration lately and taking compile_commands.json changes into account is 
> certainly on the table. We'll probably have some concrete proposals in 
> August/September.
> 
> In the meanwhile, maybe removing the caching of compile commands could fix 
> the problem? I don't have any data on how important the caching is to 
> performance (I would assume it to not be very important, since we only 
> request compile commands on the file changes; completion, findDefinitions, 
> etc reuse the last compile command anyway). 
>  https://reviews.llvm.org/D48071 added an option to disable the caching, it 
> never landed because we didn't agree on the default, but we can make it 
> happen.
>  It looks like a simpler workaround that works on all clients and we can 
> figure out how to properly integrate file watching later. WDYT?

... see answer below, as you have already provided an updated to this ...

In https://reviews.llvm.org/D49267#1171531, @ilya-biryukov wrote:

> @sammccall pointed out that I've been looking at a different layer of caching.
>  Clangd does per-directory (to avoid reloading compilation database multiple 
> times) and per-file (to avoid calling into compilation database multiple 
> times, it's expensive for our internal CDB) caching of compile commands.
>  https://reviews.llvm.org/D48071 allows to get rid of per-file cache, but 
> still keeps the per-directory cache, so this patch alone won't fix the 
> problem.


Indeed.

> We have a somewhat simple fix in mind: for interactive tools, like clangd, 
> the `JSONCompilationDatabase` could be configured to check for changes to 
> `compile_commands.json` and rebuild the compile commands on each call to its 
> methods, if necessary. E.g. we can still keep the per-directory "caching" of 
> `CompilationDatabase`s inside clangd, but CompilationDatabase instances will 
> start returning new compile commands when compile_commands.json is updated.
>  This would mean more FS accesses (and, potentially, json parsing) on 
> `getCompileCommands` and the same instance of `CompilationDatabase` will now 
> be potentially providing different compile commands for the same file on 
> different `getCompileCommands` calls.
>  However,
> 
> - The FS access concern shouldn't be noticeable for the clang tools: 
> parsing/preprocessing of files is usually done after the `getCompileCommands` 
> call, so the overhead of files access to compile_commands.json and json reads 
> should be negligible, compared to the work to process C/C++ files.
> - The fact that const object now changes seems to be a potential source of 
> confusion for the tools and (maybe?) some of them are not prepared for this, 
> so we should probably allow turning this behavior on and off. clangd (and 
> other interactive tools that might be interested in this), would turn it on, 
> while it will be off by default.
> 
>   It seems this could be implemented somewhere around the 
> `JSONCompilationDatabasePlugin` pretty easily. WDYT? I'm also happy to come 
> up with a patch to `JSONCompilationDatabasePlugin`, at this point I'm pretty 
> familiar with it. Unless you think this approach is flawed or want to do it 
> yourself, of course. In any 

[PATCH] D49253: [clangd] Fix category in clangd-vscode's package.json

[Simon Marchi via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL337697: [clangd] Fix category in clangd-vscodes 
package.json (authored by simark, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D49253

Files:
  clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json


Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json
@@ -9,7 +9,7 @@
 "vscode": "^1.18.0"
 },
 "categories": [
-"Languages",
+"Programming Languages",
 "Linters",
 "Snippets"
 ],


Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json
@@ -9,7 +9,7 @@
 "vscode": "^1.18.0"
 },
 "categories": [
-"Languages",
+"Programming Languages",
 "Linters",
 "Snippets"
 ],
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC337284: [Tooling] Add operator== to CompileCommand (authored 
by simark, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D49265?vs=155773=155883#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49265

Files:
  include/clang/Tooling/CompilationDatabase.h
  unittests/Tooling/CompilationDatabaseTest.cpp


Index: include/clang/Tooling/CompilationDatabase.h
===
--- include/clang/Tooling/CompilationDatabase.h
+++ include/clang/Tooling/CompilationDatabase.h
@@ -59,6 +59,15 @@
 
   /// The output file associated with the command.
   std::string Output;
+
+  friend bool operator==(const CompileCommand , const CompileCommand ) 
{
+return LHS.Directory == RHS.Directory && LHS.Filename == RHS.Filename &&
+   LHS.CommandLine == RHS.CommandLine && LHS.Output == RHS.Output;
+  }
+
+  friend bool operator!=(const CompileCommand , const CompileCommand ) 
{
+return !(LHS == RHS);
+  }
 };
 
 /// Interface for compilation databases.
Index: unittests/Tooling/CompilationDatabaseTest.cpp
===
--- unittests/Tooling/CompilationDatabaseTest.cpp
+++ unittests/Tooling/CompilationDatabaseTest.cpp
@@ -736,5 +736,33 @@
   EXPECT_EQ(getCommand("foo/bar/baz/shout.C"), "clang -D 
FOO/BAR/BAZ/SHOUT.cc");
 }
 
+TEST(CompileCommandTest, EqualityOperator) {
+  CompileCommand CCRef("/foo/bar", "hello.c", {"a", "b"}, "hello.o");
+  CompileCommand CCTest = CCRef;
+
+  EXPECT_TRUE(CCRef == CCTest);
+  EXPECT_FALSE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Directory = "/foo/baz";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Filename = "bonjour.c";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.CommandLine.push_back("c");
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Output = "bonjour.o";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+}
+
 } // end namespace tooling
 } // end namespace clang


Index: include/clang/Tooling/CompilationDatabase.h
===
--- include/clang/Tooling/CompilationDatabase.h
+++ include/clang/Tooling/CompilationDatabase.h
@@ -59,6 +59,15 @@
 
   /// The output file associated with the command.
   std::string Output;
+
+  friend bool operator==(const CompileCommand , const CompileCommand ) {
+return LHS.Directory == RHS.Directory && LHS.Filename == RHS.Filename &&
+   LHS.CommandLine == RHS.CommandLine && LHS.Output == RHS.Output;
+  }
+
+  friend bool operator!=(const CompileCommand , const CompileCommand ) {
+return !(LHS == RHS);
+  }
 };
 
 /// Interface for compilation databases.
Index: unittests/Tooling/CompilationDatabaseTest.cpp
===
--- unittests/Tooling/CompilationDatabaseTest.cpp
+++ unittests/Tooling/CompilationDatabaseTest.cpp
@@ -736,5 +736,33 @@
   EXPECT_EQ(getCommand("foo/bar/baz/shout.C"), "clang -D FOO/BAR/BAZ/SHOUT.cc");
 }
 
+TEST(CompileCommandTest, EqualityOperator) {
+  CompileCommand CCRef("/foo/bar", "hello.c", {"a", "b"}, "hello.o");
+  CompileCommand CCTest = CCRef;
+
+  EXPECT_TRUE(CCRef == CCTest);
+  EXPECT_FALSE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Directory = "/foo/baz";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Filename = "bonjour.c";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.CommandLine.push_back("c");
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Output = "bonjour.o";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+}
+
 } // end namespace tooling
 } // end namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL337284: [Tooling] Add operator== to CompileCommand (authored 
by simark, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D49265

Files:
  cfe/trunk/include/clang/Tooling/CompilationDatabase.h
  cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp


Index: cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
+++ cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -736,5 +736,33 @@
   EXPECT_EQ(getCommand("foo/bar/baz/shout.C"), "clang -D 
FOO/BAR/BAZ/SHOUT.cc");
 }
 
+TEST(CompileCommandTest, EqualityOperator) {
+  CompileCommand CCRef("/foo/bar", "hello.c", {"a", "b"}, "hello.o");
+  CompileCommand CCTest = CCRef;
+
+  EXPECT_TRUE(CCRef == CCTest);
+  EXPECT_FALSE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Directory = "/foo/baz";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Filename = "bonjour.c";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.CommandLine.push_back("c");
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Output = "bonjour.o";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: cfe/trunk/include/clang/Tooling/CompilationDatabase.h
===
--- cfe/trunk/include/clang/Tooling/CompilationDatabase.h
+++ cfe/trunk/include/clang/Tooling/CompilationDatabase.h
@@ -59,6 +59,15 @@
 
   /// The output file associated with the command.
   std::string Output;
+
+  friend bool operator==(const CompileCommand , const CompileCommand ) 
{
+return LHS.Directory == RHS.Directory && LHS.Filename == RHS.Filename &&
+   LHS.CommandLine == RHS.CommandLine && LHS.Output == RHS.Output;
+  }
+
+  friend bool operator!=(const CompileCommand , const CompileCommand ) 
{
+return !(LHS == RHS);
+  }
 };
 
 /// Interface for compilation databases.


Index: cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
===
--- cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
+++ cfe/trunk/unittests/Tooling/CompilationDatabaseTest.cpp
@@ -736,5 +736,33 @@
   EXPECT_EQ(getCommand("foo/bar/baz/shout.C"), "clang -D FOO/BAR/BAZ/SHOUT.cc");
 }
 
+TEST(CompileCommandTest, EqualityOperator) {
+  CompileCommand CCRef("/foo/bar", "hello.c", {"a", "b"}, "hello.o");
+  CompileCommand CCTest = CCRef;
+
+  EXPECT_TRUE(CCRef == CCTest);
+  EXPECT_FALSE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Directory = "/foo/baz";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Filename = "bonjour.c";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.CommandLine.push_back("c");
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Output = "bonjour.o";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: cfe/trunk/include/clang/Tooling/CompilationDatabase.h
===
--- cfe/trunk/include/clang/Tooling/CompilationDatabase.h
+++ cfe/trunk/include/clang/Tooling/CompilationDatabase.h
@@ -59,6 +59,15 @@
 
   /// The output file associated with the command.
   std::string Output;
+
+  friend bool operator==(const CompileCommand , const CompileCommand ) {
+return LHS.Directory == RHS.Directory && LHS.Filename == RHS.Filename &&
+   LHS.CommandLine == RHS.CommandLine && LHS.Output == RHS.Output;
+  }
+
+  friend bool operator!=(const CompileCommand , const CompileCommand ) {
+return !(LHS == RHS);
+  }
 };
 
 /// Interface for compilation databases.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 155773.
simark added a comment.

Add tests for both == and !=.

I need to rebuild ~800 files (because I pulled llvm/clang), so I did not
actually test it, but I'll do so before pushing tomorrow, of course.


Repository:
  rC Clang

https://reviews.llvm.org/D49265

Files:
  include/clang/Tooling/CompilationDatabase.h
  unittests/Tooling/CompilationDatabaseTest.cpp


Index: unittests/Tooling/CompilationDatabaseTest.cpp
===
--- unittests/Tooling/CompilationDatabaseTest.cpp
+++ unittests/Tooling/CompilationDatabaseTest.cpp
@@ -736,5 +736,33 @@
   EXPECT_EQ(getCommand("foo/bar/baz/shout.C"), "clang -D 
FOO/BAR/BAZ/SHOUT.cc");
 }
 
+TEST(CompileCommandTest, EqualityOperator) {
+  CompileCommand CCRef("/foo/bar", "hello.c", {"a", "b"}, "hello.o");
+  CompileCommand CCTest = CCRef;
+
+  EXPECT_TRUE(CCRef == CCTest);
+  EXPECT_FALSE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Directory = "/foo/baz";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Filename = "bonjour.c";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.CommandLine.push_back("c");
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Output = "bonjour.o";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: include/clang/Tooling/CompilationDatabase.h
===
--- include/clang/Tooling/CompilationDatabase.h
+++ include/clang/Tooling/CompilationDatabase.h
@@ -59,6 +59,15 @@
 
   /// The output file associated with the command.
   std::string Output;
+
+  friend bool operator==(const CompileCommand , const CompileCommand ) 
{
+return LHS.Directory == RHS.Directory && LHS.Filename == RHS.Filename &&
+   LHS.CommandLine == RHS.CommandLine && LHS.Output == RHS.Output;
+  }
+
+  friend bool operator!=(const CompileCommand , const CompileCommand ) 
{
+return !(LHS == RHS);
+  }
 };
 
 /// Interface for compilation databases.


Index: unittests/Tooling/CompilationDatabaseTest.cpp
===
--- unittests/Tooling/CompilationDatabaseTest.cpp
+++ unittests/Tooling/CompilationDatabaseTest.cpp
@@ -736,5 +736,33 @@
   EXPECT_EQ(getCommand("foo/bar/baz/shout.C"), "clang -D FOO/BAR/BAZ/SHOUT.cc");
 }
 
+TEST(CompileCommandTest, EqualityOperator) {
+  CompileCommand CCRef("/foo/bar", "hello.c", {"a", "b"}, "hello.o");
+  CompileCommand CCTest = CCRef;
+
+  EXPECT_TRUE(CCRef == CCTest);
+  EXPECT_FALSE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Directory = "/foo/baz";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Filename = "bonjour.c";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.CommandLine.push_back("c");
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+
+  CCTest = CCRef;
+  CCTest.Output = "bonjour.o";
+  EXPECT_FALSE(CCRef == CCTest);
+  EXPECT_TRUE(CCRef != CCTest);
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: include/clang/Tooling/CompilationDatabase.h
===
--- include/clang/Tooling/CompilationDatabase.h
+++ include/clang/Tooling/CompilationDatabase.h
@@ -59,6 +59,15 @@
 
   /// The output file associated with the command.
   std::string Output;
+
+  friend bool operator==(const CompileCommand , const CompileCommand ) {
+return LHS.Directory == RHS.Directory && LHS.Filename == RHS.Filename &&
+   LHS.CommandLine == RHS.CommandLine && LHS.Output == RHS.Output;
+  }
+
+  friend bool operator!=(const CompileCommand , const CompileCommand ) {
+return !(LHS == RHS);
+  }
 };
 
 /// Interface for compilation databases.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D49265#1164227, @dblaikie wrote:

> In theory you'd need separate tests for op== and op!= returning false 
> (currently all the tests would pass if both implementations returned true in 
> all cases), but not the biggest deal.


Good point, I'll update it, it will take a second.


Repository:
  rC Clang

https://reviews.llvm.org/D49265



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 155753.
simark added a comment.

- Add test
- Make operator== a function instead of method
- Add operator!= (so I can use EXPECT_NE in the test, and because it may be 
useful in general)


Repository:
  rC Clang

https://reviews.llvm.org/D49265

Files:
  include/clang/Tooling/CompilationDatabase.h
  unittests/Tooling/CompilationDatabaseTest.cpp


Index: unittests/Tooling/CompilationDatabaseTest.cpp
===
--- unittests/Tooling/CompilationDatabaseTest.cpp
+++ unittests/Tooling/CompilationDatabaseTest.cpp
@@ -736,5 +736,28 @@
   EXPECT_EQ(getCommand("foo/bar/baz/shout.C"), "clang -D 
FOO/BAR/BAZ/SHOUT.cc");
 }
 
+TEST(CompileCommandTest, EqualityOperator) {
+  CompileCommand CCRef("/foo/bar", "hello.c", {"a", "b"}, "hello.o");
+  CompileCommand CCTest = CCRef;
+
+  EXPECT_EQ(CCRef, CCTest);
+
+  CCTest = CCRef;
+  CCTest.Directory = "/foo/baz";
+  EXPECT_NE(CCRef, CCTest);
+
+  CCTest = CCRef;
+  CCTest.Filename = "bonjour.c";
+  EXPECT_NE(CCRef, CCTest);
+
+  CCTest = CCRef;
+  CCTest.CommandLine.push_back("c");
+  EXPECT_NE(CCRef, CCTest);
+
+  CCTest = CCRef;
+  CCTest.Output = "bonjour.o";
+  EXPECT_NE(CCRef, CCTest);
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: include/clang/Tooling/CompilationDatabase.h
===
--- include/clang/Tooling/CompilationDatabase.h
+++ include/clang/Tooling/CompilationDatabase.h
@@ -59,6 +59,15 @@
 
   /// The output file associated with the command.
   std::string Output;
+
+  friend bool operator==(const CompileCommand , const CompileCommand ) 
{
+return LHS.Directory == RHS.Directory && LHS.Filename == RHS.Filename &&
+   LHS.CommandLine == RHS.CommandLine && LHS.Output == RHS.Output;
+  }
+
+  friend bool operator!=(const CompileCommand , const CompileCommand ) 
{
+return !(LHS == RHS);
+  }
 };
 
 /// Interface for compilation databases.


Index: unittests/Tooling/CompilationDatabaseTest.cpp
===
--- unittests/Tooling/CompilationDatabaseTest.cpp
+++ unittests/Tooling/CompilationDatabaseTest.cpp
@@ -736,5 +736,28 @@
   EXPECT_EQ(getCommand("foo/bar/baz/shout.C"), "clang -D FOO/BAR/BAZ/SHOUT.cc");
 }
 
+TEST(CompileCommandTest, EqualityOperator) {
+  CompileCommand CCRef("/foo/bar", "hello.c", {"a", "b"}, "hello.o");
+  CompileCommand CCTest = CCRef;
+
+  EXPECT_EQ(CCRef, CCTest);
+
+  CCTest = CCRef;
+  CCTest.Directory = "/foo/baz";
+  EXPECT_NE(CCRef, CCTest);
+
+  CCTest = CCRef;
+  CCTest.Filename = "bonjour.c";
+  EXPECT_NE(CCRef, CCTest);
+
+  CCTest = CCRef;
+  CCTest.CommandLine.push_back("c");
+  EXPECT_NE(CCRef, CCTest);
+
+  CCTest = CCRef;
+  CCTest.Output = "bonjour.o";
+  EXPECT_NE(CCRef, CCTest);
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: include/clang/Tooling/CompilationDatabase.h
===
--- include/clang/Tooling/CompilationDatabase.h
+++ include/clang/Tooling/CompilationDatabase.h
@@ -59,6 +59,15 @@
 
   /// The output file associated with the command.
   std::string Output;
+
+  friend bool operator==(const CompileCommand , const CompileCommand ) {
+return LHS.Directory == RHS.Directory && LHS.Filename == RHS.Filename &&
+   LHS.CommandLine == RHS.CommandLine && LHS.Output == RHS.Output;
+  }
+
+  friend bool operator!=(const CompileCommand , const CompileCommand ) {
+return !(LHS == RHS);
+  }
 };
 
 /// Interface for compilation databases.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D49265#1163740, @dblaikie wrote:

> Any chance this can/should be unit tested? (also, in general (though might
>  not matter in this instance), symmetric operators like == should be
>  implemented as non-members (though they can still be friends and if they
>  are, can be defined inline in the class definition as a member could be),
>  so any implicit conversions apply equally to the LHS as the RHS of the
>  expression)


Of course, I'm on it.


Repository:
  rC Clang

https://reviews.llvm.org/D49265



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 155284.
simark added a comment.

Remove unintended changes


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49267

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.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
@@ -30,14 +30,18 @@
 }
 const serverOptions: vscodelc.ServerOptions = clangd;
 
-const filePattern: string = '**/*.{' +
+const sourceFilePattern: string = '**/*.{' +
 ['cpp', 'c', 'cc', 'cxx', 'c++', 'm', 'mm', 'h', 'hh', 'hpp', 'hxx', 'inc'].join() + '}';
+const compileCommandsFilePattern: string = '**/compile_commands.json';
 const clientOptions: vscodelc.LanguageClientOptions = {
 // Register the server for C/C++ files
-documentSelector: [{ scheme: 'file', pattern: filePattern }],
+documentSelector: [{ scheme: 'file', pattern: sourceFilePattern }],
 synchronize: !syncFileEvents ? undefined : {
-fileEvents: vscode.workspace.createFileSystemWatcher(filePattern)
-},
+fileEvents : [
+  vscode.workspace.createFileSystemWatcher(sourceFilePattern),
+  vscode.workspace.createFileSystemWatcher(compileCommandsFilePattern),
+]
+  },
 // Resolve symlinks for all files provided by clangd.
 // This is a workaround for a bazel + clangd issue - bazel produces a symlink tree to build in,
 // and when navigating to the included file, clangd passes its path inside the symlink tree
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -48,7 +48,7 @@
   virtual void onSignatureHelp(TextDocumentPositionParams ) = 0;
   virtual void onGoToDefinition(TextDocumentPositionParams ) = 0;
   virtual void onSwitchSourceHeader(TextDocumentIdentifier ) = 0;
-  virtual void onFileEvent(DidChangeWatchedFilesParams ) = 0;
+  virtual void onDidChangeWatchedFiles(DidChangeWatchedFilesParams ) = 0;
   virtual void onCommand(ExecuteCommandParams ) = 0;
   virtual void onWorkspaceSymbol(WorkspaceSymbolParams ) = 0;
   virtual void onRename(RenameParams ) = 0;
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -68,7 +68,8 @@
   Register("textDocument/rename", ::onRename);
   Register("textDocument/hover", ::onHover);
   Register("textDocument/documentSymbol", ::onDocumentSymbol);
-  Register("workspace/didChangeWatchedFiles", ::onFileEvent);
+  Register("workspace/didChangeWatchedFiles",
+   ::onDidChangeWatchedFiles);
   Register("workspace/executeCommand", ::onCommand);
   Register("textDocument/documentHighlight",
::onDocumentHighlight);
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -68,6 +68,9 @@
   /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
 
+  /// Forget the compilation flags cached in this object.
+  void clear();
+
 private:
   tooling::CompilationDatabase *getCDBForFile(PathRef File) const;
   tooling::CompilationDatabase *getCDBInDirLocked(PathRef File) const;
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -66,6 +66,11 @@
   CompilationDatabases.clear();
 }
 
+void DirectoryBasedGlobalCompilationDatabase::clear() {
+  std::lock_guard Lock(Mutex);
+  CompilationDatabases.clear();
+}
+
 void DirectoryBasedGlobalCompilationDatabase::setExtraFlagsForFile(
 PathRef File, std::vector ExtraFlags) {
   std::lock_guard Lock(Mutex);
Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -69,19 +69,45 @@
   void onGoToDefinition(TextDocumentPositionParams ) override;
   void onSwitchSourceHeader(TextDocumentIdentifier ) override;
   void onDocumentHighlight(TextDocumentPositionParams ) override;
-  void onFileEvent(DidChangeWatchedFilesParams ) override;
+  void onDidChangeWatchedFiles(DidChangeWatchedFilesParams ) override;
   void onCommand(ExecuteCommandParams ) override;
   void onWorkspaceSymbol(WorkspaceSymbolParams ) override;
   void onRename(RenameParams ) override;
   void onHover(TextDocumentPositionParams ) override;
   void 

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

Note, https://reviews.llvm.org/D49265 in clang is a prerequisite.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49267



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, jkorous, MaskRay, ioeric, ilya-biryukov.

This patch adds support for watching for changes to
compile_commands.json, and reparsing files if needed.

The watching is done using the "workspace/didChangeWatchedFiles"
notification, so the actual file watching is the frontend's problem.

To keep things simple, we react to any change involving a file called
"compile_commands.json".

The chosen strategy tries to avoid useless reparses.  We don't want to
reparse a file if its compile commands don't change.  So when we get a
change notification, we:

1. Save the compile commands of all open files on the side.
2. Clear everything we know about compilation databases and compilation 
commands.
3. Query the compile commands again for all open files (which will go read the 
possibly updated compile commands).  If the commands are different than the 
saved ones, we reparse the file.

I updated the vscode-clangd extension.  If you don't feel inspired, you
can use this small .cpp to test it:

  #ifdef MACRO
  #warning "MACRO is defined"
  #else
  #warning "MACRO is not defined"
  #endif
  
  int main() {}

and this compile_commands.json:

  [{
"directory": ".",
"file": "test.cpp",
"arguments": ["g++", "-c", "test.cpp", "-DMACRO=2"]
  }]

Adding and removing the "-DMACRO=2" argument, you should see a different


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49267

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/GlobalCompilationDatabase.cpp
  clangd/GlobalCompilationDatabase.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.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
@@ -30,25 +30,32 @@
 }
 const serverOptions: vscodelc.ServerOptions = clangd;
 
-const filePattern: string = '**/*.{' +
-['cpp', 'c', 'cc', 'cxx', 'c++', 'm', 'mm', 'h', 'hh', 'hpp', 'hxx', 'inc'].join() + '}';
+const sourceFilePattern: string = '**/*.{' + [
+  'cpp', 'c', 'cc', 'cxx', 'c++', 'm', 'mm', 'h', 'hh', 'hpp', 'hxx', 'inc'
+].join() + '}';
+const compileCommandsFilePattern: string = '**/compile_commands.json';
 const clientOptions: vscodelc.LanguageClientOptions = {
-// Register the server for C/C++ files
-documentSelector: [{ scheme: 'file', pattern: filePattern }],
-synchronize: !syncFileEvents ? undefined : {
-fileEvents: vscode.workspace.createFileSystemWatcher(filePattern)
-},
-// Resolve symlinks for all files provided by clangd.
-// This is a workaround for a bazel + clangd issue - bazel produces a symlink tree to build in,
-// and when navigating to the included file, clangd passes its path inside the symlink tree
-// rather than its filesystem path.
-// FIXME: remove this once clangd knows enough about bazel to resolve the
-// symlinks where needed (or if this causes problems for other workflows).
-uriConverters: {
-code2Protocol: (value: vscode.Uri) => value.toString(),
-protocol2Code: (value: string) =>
-vscode.Uri.file(realpathSync(vscode.Uri.parse(value).fsPath))
-}
+  // Register the server for C/C++ files
+  documentSelector : [ {scheme : 'file', pattern : sourceFilePattern} ],
+  synchronize : !syncFileEvents ? undefined : {
+fileEvents : [
+  vscode.workspace.createFileSystemWatcher(sourceFilePattern),
+  vscode.workspace.createFileSystemWatcher(compileCommandsFilePattern),
+]
+  },
+  // Resolve symlinks for all files provided by clangd.
+  // This is a workaround for a bazel + clangd issue - bazel produces a
+  // symlink tree to build in,
+  // and when navigating to the included file, clangd passes its path inside
+  // the symlink tree
+  // rather than its filesystem path.
+  // FIXME: remove this once clangd knows enough about bazel to resolve the
+  // symlinks where needed (or if this causes problems for other workflows).
+  uriConverters : {
+code2Protocol : (value: vscode.Uri) => value.toString(),
+protocol2Code : (value: string) =>
+vscode.Uri.file(realpathSync(vscode.Uri.parse(value).fsPath))
+  }
 };
 
 const clangdClient = new vscodelc.LanguageClient('Clang Language Server', serverOptions, clientOptions);
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -48,7 +48,7 @@
   virtual void onSignatureHelp(TextDocumentPositionParams ) = 0;
   virtual void onGoToDefinition(TextDocumentPositionParams ) = 0;
   virtual void 

[PATCH] D49265: [Tooling] Add operator== to CompileCommand

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, ioeric, ilya-biryukov.

It does the obvious thing of comparing all fields.  This will be needed
for a clangd patch I have in the pipeline.


Repository:
  rC Clang

https://reviews.llvm.org/D49265

Files:
  include/clang/Tooling/CompilationDatabase.h


Index: include/clang/Tooling/CompilationDatabase.h
===
--- include/clang/Tooling/CompilationDatabase.h
+++ include/clang/Tooling/CompilationDatabase.h
@@ -59,6 +59,11 @@
 
   /// The output file associated with the command.
   std::string Output;
+
+  bool operator==(const CompileCommand ) const {
+return Directory == RHS.Directory && Filename == RHS.Filename &&
+   CommandLine == RHS.CommandLine && Output == RHS.Output;
+  }
 };
 
 /// Interface for compilation databases.


Index: include/clang/Tooling/CompilationDatabase.h
===
--- include/clang/Tooling/CompilationDatabase.h
+++ include/clang/Tooling/CompilationDatabase.h
@@ -59,6 +59,11 @@
 
   /// The output file associated with the command.
   std::string Output;
+
+  bool operator==(const CompileCommand ) const {
+return Directory == RHS.Directory && Filename == RHS.Filename &&
+   CommandLine == RHS.CommandLine && Output == RHS.Output;
+  }
 };
 
 /// Interface for compilation databases.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49260: [clangd] JSONTracer: flush after writing event

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, omtcyfz, jkorous, MaskRay, ioeric, 
ilya-biryukov.

Let's say I use "CLANGD_TRACE=/tmp/clangd.json" and "tail -F
/tmp/clangd.json", I'll often see unfinished lines, where the rest of
the data is still in clangd's output buffer.  Doing a flush in
rawEvent makes sure we can see all the data immediatly.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49260

Files:
  clangd/Trace.cpp


Index: clangd/Trace.cpp
===
--- clangd/Trace.cpp
+++ clangd/Trace.cpp
@@ -151,6 +151,7 @@
 Event["ph"] = Phase;
 Out << Sep << formatv(JSONFormat, json::Value(std::move(Event)));
 Sep = ",\n";
+Out.flush();
   }
 
   // If we haven't already, emit metadata describing this thread.


Index: clangd/Trace.cpp
===
--- clangd/Trace.cpp
+++ clangd/Trace.cpp
@@ -151,6 +151,7 @@
 Event["ph"] = Phase;
 Out << Sep << formatv(JSONFormat, json::Value(std::move(Event)));
 Sep = ",\n";
+Out.flush();
   }
 
   // If we haven't already, emit metadata describing this thread.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49253: [clangd] Fix category in clangd-vscode's package.json

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 155232.
simark added a comment.

Oops.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49253

Files:
  clangd/clients/clangd-vscode/package.json


Index: clangd/clients/clangd-vscode/package.json
===
--- clangd/clients/clangd-vscode/package.json
+++ clangd/clients/clangd-vscode/package.json
@@ -9,7 +9,7 @@
 "vscode": "^1.18.0"
 },
 "categories": [
-"Languages",
+"Programming Languages",
 "Linters",
 "Snippets"
 ],


Index: clangd/clients/clangd-vscode/package.json
===
--- clangd/clients/clangd-vscode/package.json
+++ clangd/clients/clangd-vscode/package.json
@@ -9,7 +9,7 @@
 "vscode": "^1.18.0"
 },
 "categories": [
-"Languages",
+"Programming Languages",
 "Linters",
 "Snippets"
 ],
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49253: [clangd] Fix category in clangd-vscode's package.json

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 155213.
simark added a comment.

- [clangd] JSONTracer: flush after writing event


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49253

Files:
  clangd/Trace.cpp
  clangd/clients/clangd-vscode/package.json


Index: clangd/clients/clangd-vscode/package.json
===
--- clangd/clients/clangd-vscode/package.json
+++ clangd/clients/clangd-vscode/package.json
@@ -9,7 +9,7 @@
 "vscode": "^1.18.0"
 },
 "categories": [
-"Languages",
+"Programming Languages",
 "Linters",
 "Snippets"
 ],
Index: clangd/Trace.cpp
===
--- clangd/Trace.cpp
+++ clangd/Trace.cpp
@@ -151,6 +151,7 @@
 Event["ph"] = Phase;
 Out << Sep << formatv(JSONFormat, json::Value(std::move(Event)));
 Sep = ",\n";
+Out.flush();
   }
 
   // If we haven't already, emit metadata describing this thread.


Index: clangd/clients/clangd-vscode/package.json
===
--- clangd/clients/clangd-vscode/package.json
+++ clangd/clients/clangd-vscode/package.json
@@ -9,7 +9,7 @@
 "vscode": "^1.18.0"
 },
 "categories": [
-"Languages",
+"Programming Languages",
 "Linters",
 "Snippets"
 ],
Index: clangd/Trace.cpp
===
--- clangd/Trace.cpp
+++ clangd/Trace.cpp
@@ -151,6 +151,7 @@
 Event["ph"] = Phase;
 Out << Sep << formatv(JSONFormat, json::Value(std::move(Event)));
 Sep = ",\n";
+Out.flush();
   }
 
   // If we haven't already, emit metadata describing this thread.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49253: [clangd] Fix category in clangd-vscode's package.json

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added subscribers: cfe-commits, jkorous, MaskRay, ioeric, ilya-biryukov.

When opening package.json, vscode shows:

  Use 'Programming  Languages' instead

Replacing "Languages" with this fixes it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49253

Files:
  clangd/clients/clangd-vscode/package.json


Index: clangd/clients/clangd-vscode/package.json
===
--- clangd/clients/clangd-vscode/package.json
+++ clangd/clients/clangd-vscode/package.json
@@ -9,7 +9,7 @@
 "vscode": "^1.18.0"
 },
 "categories": [
-"Languages",
+"Programming Languages",
 "Linters",
 "Snippets"
 ],


Index: clangd/clients/clangd-vscode/package.json
===
--- clangd/clients/clangd-vscode/package.json
+++ clangd/clients/clangd-vscode/package.json
@@ -9,7 +9,7 @@
 "vscode": "^1.18.0"
 },
 "categories": [
-"Languages",
+"Programming Languages",
 "Linters",
 "Snippets"
 ],
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D48903#1159303, @ioeric wrote:

> For example, suppose you have header search directories `-I/path/to/include` 
> and `-I.` in your compile command. When preprocessor searches for an #include 
> "x.h", it will try to stat "/path/to/include/x.h" and "./x.h" and take the 
> first one that exists. This can introduce indeterminism for the path (./x.h 
> or /abs/x.h) you later get for the header file, e.g. when you try to look up 
> file name by `FileID` through `SourceManager`. The path you get for a 
> `FileEntry` or `FileID`  would depend on how clang looks up a file and how a 
> file is first opened into `SourceManager`/`FileManager`.  It seems that the 
> current behavior of clangd + in-memory file system would give you paths that 
> are relative to the working directory for both cases. I'm not sure if that's 
> the best behavior, but I think the consistency has its value. For example, in 
> unit tests where in-memory file systems are heavily used, it's important to 
> have a way to compare the reported file path (e.g. file path corresponding to 
> a source location) with the expected paths. We could choose to always return 
> real path, which could be potentially expensive, or we could require users to 
> always compare real paths (it's unclear how well this would work though e.g. 
> ClangTool doesn't expose its vfs), but they need to be enforced by the API. 
> Otherwise, I worry it would cause more confusions for folks who use clang 
> with in-memory file system in the future.


It's hard to tell without seeing an actual failing use case.  I understand the 
argument, but I think the necessity to work as closely as `RealFileSystem` as 
possible is important.  Otherwise, it becomes impossible to reproduce bugs 
happening in the real world in unittests.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: lib/Basic/FileManager.cpp:395
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE->RealPathName = RealPathName.str();

ioeric wrote:
> simark wrote:
> > ioeric wrote:
> > > It seems that at this point, we have failed to find `FileName` in vfs 
> > > (with `getStatValue` above), so `getRealPath` would also fail? 
> > > 
> > > In general, the virtual file here can be an actual *virtual* file that 
> > > doesn't exist anywhere, and I think there are users who use this to map 
> > > virtual file (possibly with relative paths) into file manager (while they 
> > > should really use overlay vfs!).
> > > It seems that at this point, we have failed to find FileName in vfs (with 
> > > getStatValue above), so getRealPath would also fail?
> > 
> > From what I understood, this code will be executed if the file actually 
> > exists but it's the first time we access it (we won't take the return at 
> > line 373).  If we take the return at line 373, then some previous 
> > invocation of getFile or getVirtualFile has created the file, and that 
> > invocation would have been responsible for computing the real path.
> > 
> > > In general, the virtual file here can be an actual *virtual* file that 
> > > doesn't exist anywhere, and I think there are users who use this to map 
> > > virtual file (possibly with relative paths) into file manager (while they 
> > > should really use overlay vfs!).
> > 
> > My thinking was that the worst that could happen is that `getRealPath` 
> > fails in that case.
> > From what I understood, this code will be executed if the file actually 
> > exists but it's the first time we access it (we won't take the return at 
> > line 373). If we take the return at line 373, then some previous invocation 
> > of getFile or getVirtualFile has created the file, and that invocation 
> > would have been responsible for computing the real path.
> I see. Thanks for the explanation!
> > My thinking was that the worst that could happen is that getRealPath fails 
> > in that case.
> It might make sense to take a look at how `getVirtualFile` is used in clang. 
> For example, in CompilerInstance, it's used to create virtual FileEntry for 
> remapped files 
> (https://github.com/llvm-mirror/clang/blob/master/lib/Frontend/CompilerInstance.cpp#L329),
>  and contents will be overwritten in `SourceManager`. This usage is arguable 
> as we have overlay file system nowadays. But in this case, if we try to 
> `getRealPath` on a remapped file, it might happen that we accidentally get an 
> absolute path on the real file system, if there happens to be a file with the 
> same path (relative to the CWD) on disk. This can be surprising and could be 
> hard to debug. This is not unusual as people tend to use file remapping to 
> overwrite the content of disk files with dirty buffers. 
> 
> In general, virtual files in FileManager and virtual files in vfs are 
> different things, and mixing them up further would likely cause more 
> confusion in the future. We should really get rid of the remapped virtual 
> files in `FileManager` and implement them properly with an overlay fs...
Ok, I'm not familiar with what file remapping is.

An alternative approach would be to compute the real path in `getFile` when 
there is a cache hit but the file entry previously represented a virtual file.  
Eessentially, we would "upgrade" the virtual file to a standard one.


Repository:
  rC Clang

https://reviews.llvm.org/D49197



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D49197#1159704, @ioeric wrote:

> In https://reviews.llvm.org/D49197#1158972, @simark wrote:
>
> > Background:  I'm trying to fix the cases why we receive a FileEntry without 
> > a real path computed in clangd, so we can avoid having to compute it 
> > ourselves.
>
>
> I'm curious how you use `getVirtualFile` in your clangd tests. In general, 
> I'd highly recommend virtual file system in favor of remapped files for 
> clangd tests.


I don't use getVirtualFile directly.  I just use `ClangdServer` and look at the 
paths it outputs.  It just happens that for some file, clang first opened it 
internally using `getVirtualFile` before using `getFile`, so the real path ends 
up missing`.  Patch https://reviews.llvm.org/D48687 adds code to compensate 
that, but I added some logging to know when it happens, so we can try to fix it 
at the root (ideally, clang could always give us the real path so we don't have 
to compute it ourselves):

  ./tools/clang/tools/extra/unittests/clangd/ClangdTests 
--gtest_filter='*RelPathsInCompileCommand*'
  Note: Google Test filter = *RelPathsInCompileCommand*
  [==] Running 1 test from 1 test case.
  [--] Global test environment set-up.
  [--] 1 test from GoToDefinition
  [ RUN  ] GoToDefinition.RelPathsInCompileCommand
  Updating file /clangd-test/src/foo.cpp with command [/clangd-test/build] 
clang -ffreestanding ../src/foo.cpp 
-resource-dir=/home/emaisin/build/llvm/tools/clang/tools/extra/unittests/clangd/../lib/clang/7.0.0
  Preamble for file /clangd-test/src/foo.cpp cannot be reused. Attempting to 
rebuild it.
  Built preamble of size 171284 for file /clangd-test/src/foo.cpp
  FileEntry for ../src/foo.cpp did not contain the real path. < HERE
  FileEntry for /clangd-test/src/header_in_preamble.h did not contain the real 
path. < AND HERE
  [   OK ] GoToDefinition.RelPathsInCompileCommand (42 ms)
  [--] 1 test from GoToDefinition (42 ms total)
  
  [--] Global test environment tear-down
  [==] 1 test from 1 test case ran. (43 ms total)
  [  PASSED  ] 1 test.

I'm investigating why clang failed to provide that to us in these two cases.


Repository:
  rC Clang

https://reviews.llvm.org/D49197



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 155057.
simark added a comment.

This is an update of my work in progress.  I haven't done any sharing with
SymbolCollector, as it's not clear to me how to proceed with that.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687

Files:
  clangd/SourceCode.cpp
  unittests/clangd/TestFS.cpp
  unittests/clangd/TestFS.h
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -312,27 +312,65 @@
 }
 
 TEST(GoToDefinition, RelPathsInCompileCommand) {
+  // The source is in "/clangd-test/src".
+  // We build in "/clangd-test/build".
+
   Annotations SourceAnnotations(R"cpp(
+#include "header_in_preamble.h"
 int [[foo]];
-int baz = f^oo;
+#include "header_not_in_preamble.h"
+int baz = f$p1^oo + bar_pre$p2^amble + bar_not_pre$p3^amble;
+)cpp");
+
+  Annotations HeaderInPreambleAnnotations(R"cpp(
+int [[bar_preamble]];
+)cpp");
+
+  Annotations HeaderNotInPreambleAnnotations(R"cpp(
+int [[bar_not_preamble]];
 )cpp");
 
+  // Make the compilation paths appear as ../src/foo.cpp in the compile
+  // commands.
+  SmallString<32> RelPathPrefix("..");
+  llvm::sys::path::append(RelPathPrefix, "src");
+  std::string BuildDir = testPath("build");
+  MockCompilationDatabase CDB(BuildDir, RelPathPrefix);
+
   IgnoreDiagnostics DiagConsumer;
-  MockCompilationDatabase CDB(/*UseRelPaths=*/true);
   MockFSProvider FS;
   ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
 
-  auto FooCpp = testPath("foo.cpp");
+  // Fill the filesystem.
+  auto FooCpp = testPath("src/foo.cpp");
   FS.Files[FooCpp] = "";
+  auto HeaderInPreambleH = testPath("src/header_in_preamble.h");
+  FS.Files[HeaderInPreambleH] = HeaderInPreambleAnnotations.code();
+  auto HeaderNotInPreambleH = testPath("src/header_not_in_preamble.h");
+  FS.Files[HeaderNotInPreambleH] = HeaderNotInPreambleAnnotations.code();
 
-  Server.addDocument(FooCpp, SourceAnnotations.code());
   runAddDocument(Server, FooCpp, SourceAnnotations.code());
+
+  // Go to a definition in main source file.
   auto Locations =
-  runFindDefinitions(Server, FooCpp, SourceAnnotations.point());
+  runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p1"));
   EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
-
   EXPECT_THAT(*Locations, ElementsAre(Location{URIForFile{FooCpp},
SourceAnnotations.range()}));
+
+  // Go to a definition in header_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p2"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderInPreambleH},
+   HeaderInPreambleAnnotations.range()}));
+
+  // Go to a definition in header_not_in_preamble.h.
+  Locations = runFindDefinitions(Server, FooCpp, SourceAnnotations.point("p3"));
+  EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
+  EXPECT_THAT(*Locations,
+  ElementsAre(Location{URIForFile{HeaderNotInPreambleH},
+   HeaderNotInPreambleAnnotations.range()}));
 }
 
 TEST(Hover, All) {
Index: unittests/clangd/TestFS.h
===
--- unittests/clangd/TestFS.h
+++ unittests/clangd/TestFS.h
@@ -39,15 +39,22 @@
 // A Compilation database that returns a fixed set of compile flags.
 class MockCompilationDatabase : public GlobalCompilationDatabase {
 public:
-  /// When \p UseRelPaths is true, uses relative paths in compile commands.
-  /// When \p UseRelPaths is false, uses absoulte paths.
-  MockCompilationDatabase(bool UseRelPaths = false);
+  /// If \p Directory is not null, use that as the Directory field of the
+  /// CompileCommand.
+  ///
+  /// If \p RelPathPrefix is not null, use that as a prefix in front of the
+  /// source file name, instead of using an absolute path.
+  MockCompilationDatabase(StringRef Directory = StringRef(),
+  StringRef RelPathPrefix = StringRef());
 
   llvm::Optional
   getCompileCommand(PathRef File) const override;
 
   std::vector ExtraClangFlags;
-  const bool UseRelPaths;
+
+private:
+  StringRef Directory;
+  StringRef RelPathPrefix;
 };
 
 // Returns an absolute (fake) test directory for this OS.
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -30,22 +30,36 @@
   return MemFS;
 }
 
-MockCompilationDatabase::MockCompilationDatabase(bool UseRelPaths)
-: ExtraClangFlags({"-ffreestanding"}), UseRelPaths(UseRelPaths) {
+MockCompilationDatabase::MockCompilationDatabase(StringRef Directory,
+ StringRef 

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Simon Marchi via Phabricator via cfe-commits]

simark added inline comments.



Comment at: lib/Basic/FileManager.cpp:395
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE->RealPathName = RealPathName.str();

ioeric wrote:
> It seems that at this point, we have failed to find `FileName` in vfs (with 
> `getStatValue` above), so `getRealPath` would also fail? 
> 
> In general, the virtual file here can be an actual *virtual* file that 
> doesn't exist anywhere, and I think there are users who use this to map 
> virtual file (possibly with relative paths) into file manager (while they 
> should really use overlay vfs!).
> It seems that at this point, we have failed to find FileName in vfs (with 
> getStatValue above), so getRealPath would also fail?

From what I understood, this code will be executed if the file actually exists 
but it's the first time we access it (we won't take the return at line 373).  
If we take the return at line 373, then some previous invocation of getFile or 
getVirtualFile has created the file, and that invocation would have been 
responsible for computing the real path.

> In general, the virtual file here can be an actual *virtual* file that 
> doesn't exist anywhere, and I think there are users who use this to map 
> virtual file (possibly with relative paths) into file manager (while they 
> should really use overlay vfs!).

My thinking was that the worst that could happen is that `getRealPath` fails in 
that case.


Repository:
  rC Clang

https://reviews.llvm.org/D49197



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D48903#1159023, @ioeric wrote:

> Would you mind reverting this patch for now so that we can come up with a 
> solution to address those use cases?
>
> Sorry again about missing the discussion earlier!


Of course, feel free to revert if needed (I'm not sure how to do that).  Are 
you able to come up with a test case that covers the use case you mention?


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

Background:  I'm trying to fix the cases why we receive a FileEntry without a 
real path computed in clangd, so we can avoid having to compute it ourselves.


Repository:
  rC Clang

https://reviews.llvm.org/D49197



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

I'm not sure who should review this patch, please add anybody you think is 
qualified/responsible.


Repository:
  rC Clang

https://reviews.llvm.org/D49197



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 155027.
simark added a comment.

Update commit message.


Repository:
  rC Clang

https://reviews.llvm.org/D49197

Files:
  lib/Basic/FileManager.cpp
  unittests/Basic/FileManagerTest.cpp


Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -322,4 +322,40 @@
   EXPECT_EQ(Path, ExpectedResult);
 }
 
+/// When calling getVirtualFile then getFile for the same file, the real path
+/// should be computed (in other words, getVirtualFile should compute the real
+/// path).
+TEST_F(FileManagerTest, getVirtualFileThenGetFileRealPathName) {
+  SmallString<64> RealPath;
+  SmallString<64> NonRealPath;
+  SmallString<64> WorkingDirectory;
+
+#ifdef _WIN32
+  RealPath = "C:\\a\\b";
+  NonRealPath = "C:\\a\\..\\a\\b";
+  WorkingDirectory = "C:\\";
+#else
+  RealPath = "/a/b";
+  NonRealPath = "/a/../a/b";
+  WorkingDirectory = "/";
+#endif
+
+  auto FS =
+  IntrusiveRefCntPtr(new vfs::InMemoryFileSystem);
+  ASSERT_TRUE(!FS->setCurrentWorkingDirectory(WorkingDirectory));
+
+  FS->addFile(RealPath, 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+  FileSystemOptions Opts;
+  FileManager Manager(Opts, FS);
+
+  const FileEntry *VFE = Manager.getVirtualFile(NonRealPath, 0, 0);
+  ASSERT_TRUE(VFE != nullptr);
+  EXPECT_EQ(VFE->tryGetRealPathName(), RealPath);
+
+  const FileEntry *FE = Manager.getFile(NonRealPath);
+  ASSERT_TRUE(FE != nullptr);
+  EXPECT_EQ(FE->tryGetRealPathName(), RealPath);
+}
+
 } // anonymous namespace
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -390,6 +390,11 @@
   UFE->UID = NextFileUID++;
   UFE->IsValid = true;
   UFE->File.reset();
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE->RealPathName = RealPathName.str();
+
   return UFE;
 }
 


Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -322,4 +322,40 @@
   EXPECT_EQ(Path, ExpectedResult);
 }
 
+/// When calling getVirtualFile then getFile for the same file, the real path
+/// should be computed (in other words, getVirtualFile should compute the real
+/// path).
+TEST_F(FileManagerTest, getVirtualFileThenGetFileRealPathName) {
+  SmallString<64> RealPath;
+  SmallString<64> NonRealPath;
+  SmallString<64> WorkingDirectory;
+
+#ifdef _WIN32
+  RealPath = "C:\\a\\b";
+  NonRealPath = "C:\\a\\..\\a\\b";
+  WorkingDirectory = "C:\\";
+#else
+  RealPath = "/a/b";
+  NonRealPath = "/a/../a/b";
+  WorkingDirectory = "/";
+#endif
+
+  auto FS =
+  IntrusiveRefCntPtr(new vfs::InMemoryFileSystem);
+  ASSERT_TRUE(!FS->setCurrentWorkingDirectory(WorkingDirectory));
+
+  FS->addFile(RealPath, 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+  FileSystemOptions Opts;
+  FileManager Manager(Opts, FS);
+
+  const FileEntry *VFE = Manager.getVirtualFile(NonRealPath, 0, 0);
+  ASSERT_TRUE(VFE != nullptr);
+  EXPECT_EQ(VFE->tryGetRealPathName(), RealPath);
+
+  const FileEntry *FE = Manager.getFile(NonRealPath);
+  ASSERT_TRUE(FE != nullptr);
+  EXPECT_EQ(FE->tryGetRealPathName(), RealPath);
+}
+
 } // anonymous namespace
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -390,6 +390,11 @@
   UFE->UID = NextFileUID++;
   UFE->IsValid = true;
   UFE->File.reset();
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE->RealPathName = RealPathName.str();
+
   return UFE;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49197: FileManager: Try to compute RealPathName in getVirtualFile

[Simon Marchi via Phabricator via cfe-commits]

simark created this revision.
Herald added a subscriber: cfe-commits.

I noticed that when getVirtualFile is called for a file, subsequent
calls to getFile will return a FileEntry without the RealPathName
computed:

  const FileEntry *VFE = Mgr->getVirtualFile("/foo/../bar", ...);
  const FileEntry *FE = Mgr->getFile("/foo/../bar");
  
  FE->tryGetRealPathName() returns an empty string.

This is because getVirtualFile creates a FileEntry without computing the
RealPathName field and stores it in SeenFileEntries.  getFile then
simply retrieves this object and returns it, when asked for the same
file.

I think it's reasonable to try to compute RealPathName in getVirtualFile
too.

There might be a similar issue with the File field.  If you call
getVirtualFile, then call getFile with open=true, you may get a
FileEntry with the File field NULL, which is not what you requested.  I
have not addressed this issue in this patch though.


Repository:
  rC Clang

https://reviews.llvm.org/D49197

Files:
  lib/Basic/FileManager.cpp
  unittests/Basic/FileManagerTest.cpp


Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -322,4 +322,40 @@
   EXPECT_EQ(Path, ExpectedResult);
 }
 
+/// When calling getVirtualFile then getFile for the same file, the real path
+/// should be computed (in other words, getVirtualFile should compute the real
+/// path).
+TEST_F(FileManagerTest, getVirtualFileThenGetFileRealPathName) {
+  SmallString<64> RealPath;
+  SmallString<64> NonRealPath;
+  SmallString<64> WorkingDirectory;
+
+#ifdef _WIN32
+  RealPath = "C:\\a\\b";
+  NonRealPath = "C:\\a\\..\\a\\b";
+  WorkingDirectory = "C:\\";
+#else
+  RealPath = "/a/b";
+  NonRealPath = "/a/../a/b";
+  WorkingDirectory = "/";
+#endif
+
+  auto FS =
+  IntrusiveRefCntPtr(new vfs::InMemoryFileSystem);
+  ASSERT_TRUE(!FS->setCurrentWorkingDirectory(WorkingDirectory));
+
+  FS->addFile(RealPath, 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+  FileSystemOptions Opts;
+  FileManager Manager(Opts, FS);
+
+  const FileEntry *VFE = Manager.getVirtualFile(NonRealPath, 0, 0);
+  ASSERT_TRUE(VFE != nullptr);
+  EXPECT_EQ(VFE->tryGetRealPathName(), RealPath);
+
+  const FileEntry *FE = Manager.getFile(NonRealPath);
+  ASSERT_TRUE(FE != nullptr);
+  EXPECT_EQ(FE->tryGetRealPathName(), RealPath);
+}
+
 } // anonymous namespace
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -390,6 +390,11 @@
   UFE->UID = NextFileUID++;
   UFE->IsValid = true;
   UFE->File.reset();
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE->RealPathName = RealPathName.str();
+
   return UFE;
 }
 


Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -322,4 +322,40 @@
   EXPECT_EQ(Path, ExpectedResult);
 }
 
+/// When calling getVirtualFile then getFile for the same file, the real path
+/// should be computed (in other words, getVirtualFile should compute the real
+/// path).
+TEST_F(FileManagerTest, getVirtualFileThenGetFileRealPathName) {
+  SmallString<64> RealPath;
+  SmallString<64> NonRealPath;
+  SmallString<64> WorkingDirectory;
+
+#ifdef _WIN32
+  RealPath = "C:\\a\\b";
+  NonRealPath = "C:\\a\\..\\a\\b";
+  WorkingDirectory = "C:\\";
+#else
+  RealPath = "/a/b";
+  NonRealPath = "/a/../a/b";
+  WorkingDirectory = "/";
+#endif
+
+  auto FS =
+  IntrusiveRefCntPtr(new vfs::InMemoryFileSystem);
+  ASSERT_TRUE(!FS->setCurrentWorkingDirectory(WorkingDirectory));
+
+  FS->addFile(RealPath, 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+  FileSystemOptions Opts;
+  FileManager Manager(Opts, FS);
+
+  const FileEntry *VFE = Manager.getVirtualFile(NonRealPath, 0, 0);
+  ASSERT_TRUE(VFE != nullptr);
+  EXPECT_EQ(VFE->tryGetRealPathName(), RealPath);
+
+  const FileEntry *FE = Manager.getFile(NonRealPath);
+  ASSERT_TRUE(FE != nullptr);
+  EXPECT_EQ(FE->tryGetRealPathName(), RealPath);
+}
+
 } // anonymous namespace
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -390,6 +390,11 @@
   UFE->UID = NextFileUID++;
   UFE->IsValid = true;
   UFE->File.reset();
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE->RealPathName = RealPathName.str();
+
   return UFE;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC336807: [VirtualFileSystem] InMemoryFileSystem::status: 
Return a Status with the… (authored by simark, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D48903?vs=154835=154995#toc

Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -315,9 +315,11 @@
   UFE.InPCH = Data.InPCH;
   UFE.File = std::move(F);
   UFE.IsValid = true;
-  if (UFE.File)
-if (auto RealPathName = UFE.File->getName())
-  UFE.RealPathName = *RealPathName;
+
+  SmallString<128> RealPathName;
+  if (!FS->getRealPath(InterndFileName, RealPathName))
+UFE.RealPathName = RealPathName.str();
+
   return 
 }
 
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -471,15 +471,33 @@
 /// The in memory file system is a tree of Nodes. Every node can either be a
 /// file or a directory.
 class InMemoryNode {
-  Status Stat;
   InMemoryNodeKind Kind;
+  Status Stat;
+
+protected:
+  /// Return Stat.  This should only be used for internal/debugging use.  When
+  /// clients wants the Status of this node, they should use
+  /// \p getStatus(StringRef).
+  const Status& getStatus() const {
+return Stat;
+  }
 
 public:
   InMemoryNode(Status Stat, InMemoryNodeKind Kind)
-  : Stat(std::move(Stat)), Kind(Kind) {}
+  : Kind(Kind), Stat(std::move(Stat)) {}
   virtual ~InMemoryNode() = default;
 
-  const Status () const { return Stat; }
+  /// Return the \p Status for this node. \p RequestedName should be the name
+  /// through which the caller referred to this node. It will override
+  /// \p Status::Name in the return value, to mimic the behavior of \p RealFile.
+  Status getStatus(StringRef RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+  }
+
+  /// Get the filename of this node (the name without the directory part).
+  StringRef getFileName() const {
+return llvm::sys::path::filename(Stat.getName());
+  }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -504,14 +522,22 @@
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
+/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
+/// \p RealFile.
 class InMemoryFileAdaptor : public File {
   InMemoryFile 
 
+  /// The name to use when returning a Status for this file.
+  std::string RequestedName;
+
 public:
-  explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+  explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+  : Node(Node), RequestedName(std::move(RequestedName)) {}
 
-  llvm::ErrorOr status() override { return Node.getStatus(); }
+  llvm::ErrorOr status() override {
+return Node.getStatus(RequestedName);
+  }
 
   llvm::ErrorOr>
   getBuffer(const Twine , int64_t FileSize, bool RequiresNullTerminator,
@@ -711,7 +737,7 @@
 llvm::ErrorOr InMemoryFileSystem::status(const Twine ) {
   auto Node = lookupInMemoryNode(*this, Root.get(), Path);
   if (Node)
-return (*Node)->getStatus();
+return (*Node)->getStatus(Path.str());
   return Node.getError();
 }
 
@@ -724,7 +750,8 @@
   // When we have a file provide a heap-allocated wrapper for the memory buffer
   // to match the ownership semantics for File.
   if (auto *F = dyn_cast(*Node))
-return std::unique_ptr(new detail::InMemoryFileAdaptor(*F));
+return std::unique_ptr(
+new detail::InMemoryFileAdaptor(*F, Path.str()));
 
   // FIXME: errc::not_a_file?
   return make_error_code(llvm::errc::invalid_argument);
@@ -736,21 +763,33 @@
 class InMemoryDirIterator : public clang::vfs::detail::DirIterImpl {
   detail::InMemoryDirectory::const_iterator I;
   detail::InMemoryDirectory::const_iterator E;
+  std::string RequestedDirName;
+
+  void setCurrentEntry() {
+if (I != E) {
+  SmallString<256> Path(RequestedDirName);
+  llvm::sys::path::append(Path, I->second->getFileName());
+  CurrentEntry = I->second->getStatus(Path.str());
+} else {
+  // When we're at the end, make CurrentEntry invalid and DirIterImpl will
+  // do the rest.
+  CurrentEntry = Status();
+}
+  }
 
 public:
   InMemoryDirIterator() = default;
 
-  explicit InMemoryDirIterator(detail::InMemoryDirectory )
-  : I(Dir.begin()), E(Dir.end()) {
-if (I != E)
-  CurrentEntry = I->second->getStatus();
+  explicit InMemoryDirIterator(detail::InMemoryDirectory ,
+   std::string 

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.
Herald added a subscriber: omtcyfz.

I took a look at `SymbolCollector`'s `toURI`, and I am not sure what to get 
from it. It seems like a lot of it could be replaced with a call to 
`FileSystem::getRealPath`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48687



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 154835.
simark added a comment.

Bump SmallString size from 32 to 256


Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -794,7 +794,7 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
@@ -919,6 +919,37 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using a directory iterator.
+  std::error_code EC;
+  clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
+  ASSERT_EQ("../b/c", It->getName());
+}
+
 // NOTE: in the tests below, we use '//root/' as our root directory, since it is
 // a legal *absolute* path on Windows as well as *nix.
 class VFSFromYAMLTest : public ::testing::Test {
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -471,15 +471,33 @@
 /// The in memory file system is a tree of Nodes. Every node can either be a
 /// file or a directory.
 class InMemoryNode {
-  Status Stat;
   InMemoryNodeKind Kind;
+  Status Stat;
+
+protected:
+  /// Return Stat.  This should only be used for internal/debugging use.  When
+  /// clients wants the Status of this node, they should use
+  /// \p getStatus(StringRef).
+  const Status& getStatus() const {
+return Stat;
+  }
 
 public:
   InMemoryNode(Status Stat, InMemoryNodeKind Kind)
-  : Stat(std::move(Stat)), Kind(Kind) {}
+  : Kind(Kind), Stat(std::move(Stat)) {}
   virtual ~InMemoryNode() = default;
 
-  const Status () const { return Stat; }
+  /// Return the \p Status for this node. \p RequestedName should be the name
+  /// through which the caller referred to this node. It will override
+  /// \p Status::Name in the return value, to mimic the behavior of \p RealFile.
+  Status getStatus(StringRef RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+  }
+
+  /// Get the filename of this node (the name without the directory part).
+  StringRef getFileName() const {
+return llvm::sys::path::filename(Stat.getName());
+  }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -504,14 +522,22 @@
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
+/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
+/// \p RealFile.
 class InMemoryFileAdaptor : public File {
   InMemoryFile 
 
+  /// The name to use when returning a Status for this file.
+  std::string RequestedName;
+
 public:
-  explicit InMemoryFileAdaptor(InMemoryFile ) : 

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D48903#1157330, @ilya-biryukov wrote:

> LGTM if that does not introduce any regressions in clang and clang-tools.


Thanks.  I have seen no failures in `check-clang` and `check-clang-tools`, so I 
will push it.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 154782.
simark marked 5 inline comments as done.
simark added a comment.

- Make InMemoryNode::Stat private again, add protected accessor.
- Change condition formatting.


Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -794,7 +794,7 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
@@ -919,6 +919,37 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using a directory iterator.
+  std::error_code EC;
+  clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
+  ASSERT_EQ("../b/c", It->getName());
+}
+
 // NOTE: in the tests below, we use '//root/' as our root directory, since it is
 // a legal *absolute* path on Windows as well as *nix.
 class VFSFromYAMLTest : public ::testing::Test {
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -471,15 +471,33 @@
 /// The in memory file system is a tree of Nodes. Every node can either be a
 /// file or a directory.
 class InMemoryNode {
-  Status Stat;
   InMemoryNodeKind Kind;
+  Status Stat;
+
+protected:
+  /// Return Stat.  This should only be used for internal/debugging use.  When
+  /// clients wants the Status of this node, they should use
+  /// \p getStatus(StringRef).
+  const Status& getStatus() const {
+return Stat;
+  }
 
 public:
   InMemoryNode(Status Stat, InMemoryNodeKind Kind)
-  : Stat(std::move(Stat)), Kind(Kind) {}
+  : Kind(Kind), Stat(std::move(Stat)) {}
   virtual ~InMemoryNode() = default;
 
-  const Status () const { return Stat; }
+  /// Return the \p Status for this node. \p RequestedName should be the name
+  /// through which the caller referred to this node. It will override
+  /// \p Status::Name in the return value, to mimic the behavior of \p RealFile.
+  Status getStatus(StringRef RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+  }
+
+  /// Get the filename of this node (the name without the directory part).
+  StringRef getFileName() const {
+return llvm::sys::path::filename(Stat.getName());
+  }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -504,14 +522,22 @@
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
+/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
+/// \p RealFile.
 class InMemoryFileAdaptor : public File {
   InMemoryFile 
 
+  /// The name to use when returning a Status for this 

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 154662.
simark added a comment.

- Change InMemoryNode::getName to InMemoryNode::getFileName, to reduce the risk

of mis-using it.  Make the Stat field protected, make the subclasses' toString
access it directly.


Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -794,7 +794,7 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
@@ -919,6 +919,37 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using a directory iterator.
+  std::error_code EC;
+  clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
+  ASSERT_EQ("../b/c", It->getName());
+}
+
 // NOTE: in the tests below, we use '//root/' as our root directory, since it is
 // a legal *absolute* path on Windows as well as *nix.
 class VFSFromYAMLTest : public ::testing::Test {
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -471,15 +471,27 @@
 /// The in memory file system is a tree of Nodes. Every node can either be a
 /// file or a directory.
 class InMemoryNode {
-  Status Stat;
   InMemoryNodeKind Kind;
 
+protected:
+  Status Stat;
+
 public:
   InMemoryNode(Status Stat, InMemoryNodeKind Kind)
-  : Stat(std::move(Stat)), Kind(Kind) {}
+  : Kind(Kind), Stat(std::move(Stat)) {}
   virtual ~InMemoryNode() = default;
 
-  const Status () const { return Stat; }
+  /// Return the \p Status for this node. \p RequestedName should be the name
+  /// through which the caller referred to this node. It will override
+  /// \p Status::Name in the return value, to mimic the behavior of \p RealFile.
+  Status getStatus(StringRef RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+  }
+
+  /// Get the filename of this node (the name without the directory part).
+  StringRef getFileName() const {
+return llvm::sys::path::filename(Stat.getName());
+  }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -496,22 +508,30 @@
   llvm::MemoryBuffer *getBuffer() { return Buffer.get(); }
 
   std::string toString(unsigned Indent) const override {
-return (std::string(Indent, ' ') + getStatus().getName() + "\n").str();
+return (std::string(Indent, ' ') + Stat.getName() + "\n").str();
   }
 
   static bool classof(const InMemoryNode *N) {
 return N->getKind() == IME_File;
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
+/// \p InMemoryFileAdaptor mimic as 

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 154631.
simark added a comment.

- Use FileSystem::getRealPath in FileManager::getFile


Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/FileManager.cpp
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -794,7 +794,7 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
@@ -919,6 +919,37 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using a directory iterator.
+  std::error_code EC;
+  clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
+  ASSERT_EQ("../b/c", It->getName());
+}
+
 // NOTE: in the tests below, we use '//root/' as our root directory, since it is
 // a legal *absolute* path on Windows as well as *nix.
 class VFSFromYAMLTest : public ::testing::Test {
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -479,7 +479,13 @@
   : Stat(std::move(Stat)), Kind(Kind) {}
   virtual ~InMemoryNode() = default;
 
-  const Status () const { return Stat; }
+  /// Return the \p Status for this node. \p RequestedName should be the name
+  /// through which the caller referred to this node. It will override
+  /// \p Status::Name in the return value, to mimic the behavior of \p RealFile.
+  Status getStatus(StringRef RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+  }
+  StringRef getName() const { return Stat.getName(); }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -496,22 +502,30 @@
   llvm::MemoryBuffer *getBuffer() { return Buffer.get(); }
 
   std::string toString(unsigned Indent) const override {
-return (std::string(Indent, ' ') + getStatus().getName() + "\n").str();
+return (std::string(Indent, ' ') + getName() + "\n").str();
   }
 
   static bool classof(const InMemoryNode *N) {
 return N->getKind() == IME_File;
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
+/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
+/// \p RealFile.
 class InMemoryFileAdaptor : public File {
   InMemoryFile 
 
+  /// The name to use when returning a Status for this file.
+  std::string RequestedName;
+
 public:
-  explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+  explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+  : Node(Node), RequestedName(std::move(RequestedName)) {}
 
-  llvm::ErrorOr status() override { return Node.getStatus(); }
+  llvm::ErrorOr status() override {
+return 

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

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

In https://reviews.llvm.org/D48903#1155403, @ilya-biryukov wrote:

> In https://reviews.llvm.org/D48903#1154846, @simark wrote:
>
> > With the `InMemoryFileSystem`, this now returns a non-real path.  The 
> > result is that we fill `RealPathName` with that non-real path.  I see two 
> > options here:
> >
> > 1. Either the FileManager is wrong to assume that `File::getName` returns a 
> > real path, and should call `FS->getRealPath` to do the job.
> > 2. If the contract is that the ` File::getName` interface should return a 
> > real path, then we should fix the `File::getName` implementation to do that 
> > somehow.
> >
> >   I would opt for 1.  This way, we could compute the `RealPathName` field 
> > even if we don't open the file (unlike currently).
>
>
> I'd also say `FileManager` should use `FileSystem::getRealPath`. The code 
> that does it differently was there before `FileSystem::getRealPath` was added.
>  And `RealFile` should probably not do any magic in `getName`, we could add a 
> separate method for (`getRealName`?) if that's absolutely needed.


I made `FileManager::getFile` use `FileSystem::getRealPath` and see no 
regression in clang and clang-tools-extra.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark marked 6 inline comments as done.
simark added inline comments.



Comment at: lib/Basic/VirtualFileSystem.cpp:488
+  }
+  StringRef getName() const { return Stat.getName(); }
   InMemoryNodeKind getKind() const { return Kind; }

ilya-biryukov wrote:
> Given that this method is inconsistent with `getStatus()` and seems to be 
> only used in `toString` methods, maybe we could make it `protected`? 
> Otherwise it's really easy to write code that gets the wrong path.
I now use it in `InMemoryDirIterator::setCurrentEntry`.  I will write a comment 
to the `getName` method to clarify this.


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

I found something fishy.  There is this code in FileManager.cpp:

  if (UFE.File)
if (auto RealPathName = UFE.File->getName())
  UFE.RealPathName = *RealPathName;

The real path is obtained from `UFE.File->getName()`.  In the `RealFile` 
implementation, it returns the `RealName` field, which is fine.  For other 
implementations, it uses `File::getName`, which is:

  /// Get the name of the file
  virtual llvm::ErrorOr getName() {
if (auto Status = status())
  return Status->getName().str();
else
  return Status.getError();
  }

With the `InMemoryFileSystem`, this now returns a non-real path.  The result is 
that we fill `RealPathName` with that non-real path.  I see two options here:

1. Either the FileManager is wrong to assume that `File::getName` returns a 
real path, and should call `FS->getRealPath` to do the job.
2. If the contract is that the ` File::getName` interface should return a real 
path, then we should fix the `File::getName` implementation to do that somehow.

I would opt for 1.  This way, we could compute the `RealPathName` field even if 
we don't open the file (unlike currently).


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 154422.
simark marked an inline comment as done.
simark added a comment.

- Use StringRef in InMemoryNode::getStatus
- Remove unused variable in TEST_F(InMemoryFileSystemTest, StatusName)


Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -794,7 +794,7 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
@@ -919,6 +919,37 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using a directory iterator.
+  std::error_code EC;
+  clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
+  ASSERT_EQ("../b/c", It->getName());
+}
+
 // NOTE: in the tests below, we use '//root/' as our root directory, since it is
 // a legal *absolute* path on Windows as well as *nix.
 class VFSFromYAMLTest : public ::testing::Test {
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -479,7 +479,13 @@
   : Stat(std::move(Stat)), Kind(Kind) {}
   virtual ~InMemoryNode() = default;
 
-  const Status () const { return Stat; }
+  /// Return the \p Status for this node. \p RequestedName should be the name
+  /// through which the caller referred to this node. It will override
+  /// \p Status::Name in the return value, to mimic the behavior of \p RealFile.
+  Status getStatus(StringRef RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+  }
+  StringRef getName() const { return Stat.getName(); }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -496,22 +502,30 @@
   llvm::MemoryBuffer *getBuffer() { return Buffer.get(); }
 
   std::string toString(unsigned Indent) const override {
-return (std::string(Indent, ' ') + getStatus().getName() + "\n").str();
+return (std::string(Indent, ' ') + getName() + "\n").str();
   }
 
   static bool classof(const InMemoryNode *N) {
 return N->getKind() == IME_File;
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
+/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
+/// \p RealFile.
 class InMemoryFileAdaptor : public File {
   InMemoryFile 
 
+  /// The name to use when returning a Status for this file.
+  std::string RequestedName;
+
 public:
-  explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+  explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+  : Node(Node), RequestedName(std::move(RequestedName)) {}
 
-  llvm::ErrorOr status() override { return Node.getStatus(); }

[PATCH] D49008: [clangd] Upgrade logging facilities with levels and formatv.

[Simon Marchi via Phabricator via cfe-commits]

simark added a comment.

In https://reviews.llvm.org/D49008#1154196, @sammccall wrote:

> @simark - is it OK if I pick this up?


Yes, totally fine.  I didn't have time to follow up on my patch, and I think 
you did a great job here.  I tested it, and I see that my main pain point was 
addressed.  With the patch, clangd is more quiet by default, only outputting 
errors, so I'm happy :).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49008



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark updated this revision to Diff 154321.
simark marked 4 inline comments as done.
simark added a comment.

- Add RequestedName to InMemoryNode::getStatus.
- Also fix the directory_iterator code path.


Repository:
  rC Clang

https://reviews.llvm.org/D48903

Files:
  lib/Basic/VirtualFileSystem.cpp
  unittests/Basic/VirtualFileSystemTest.cpp
  unittests/Driver/ToolChainTest.cpp

Index: unittests/Driver/ToolChainTest.cpp
===
--- unittests/Driver/ToolChainTest.cpp
+++ unittests/Driver/ToolChainTest.cpp
@@ -113,7 +113,7 @@
   std::replace(S.begin(), S.end(), '\\', '/');
 #endif
   EXPECT_EQ("Found candidate GCC installation: "
-"/home/test/lib/gcc/arm-linux-gnueabi/4.6.1\n"
+"/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Selected GCC installation: "
 "/home/test/bin/../lib/gcc/arm-linux-gnueabi/4.6.1\n"
 "Candidate multilib: .;@m32\n"
Index: unittests/Basic/VirtualFileSystemTest.cpp
===
--- unittests/Basic/VirtualFileSystemTest.cpp
+++ unittests/Basic/VirtualFileSystemTest.cpp
@@ -794,7 +794,7 @@
 
   auto Stat = FS.status("/b/c");
   ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n" << FS.toString();
-  ASSERT_EQ("c", Stat->getName());
+  ASSERT_EQ("/b/c", Stat->getName());
   ASSERT_EQ("/b", *FS.getCurrentWorkingDirectory());
 
   Stat = FS.status("c");
@@ -919,6 +919,39 @@
   ASSERT_TRUE(Stat->isRegularFile());
 }
 
+// Test that the name returned by status() is in the same form as the path that
+// was requested (to match the behavior of RealFileSystem).
+TEST_F(InMemoryFileSystemTest, StatusName) {
+  NormalizedFS.addFile("/a/b/c", 0, MemoryBuffer::getMemBuffer("abc"),
+   /*User=*/None,
+   /*Group=*/None, sys::fs::file_type::regular_file);
+  NormalizedFS.setCurrentWorkingDirectory("/a/b");
+
+  // Access using InMemoryFileSystem::status.
+  auto Stat = NormalizedFS.status("../b/c");
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using InMemoryFileAdaptor::status.
+  auto File = NormalizedFS.openFileForRead("../b/c");
+  ASSERT_FALSE(File.getError()) << File.getError() << "\n"
+<< NormalizedFS.toString();
+  Stat = (*File)->status();
+  ASSERT_FALSE(Stat.getError()) << Stat.getError() << "\n"
+<< NormalizedFS.toString();
+  ASSERT_TRUE(Stat->isRegularFile());
+  ASSERT_EQ("../b/c", Stat->getName());
+
+  // Access using a directory iterator.
+  std::error_code EC;
+  clang::vfs::directory_iterator It = NormalizedFS.dir_begin("../b", EC);
+  clang::vfs::directory_iterator End;
+
+  ASSERT_EQ("../b/c", It->getName());
+}
+
 // NOTE: in the tests below, we use '//root/' as our root directory, since it is
 // a legal *absolute* path on Windows as well as *nix.
 class VFSFromYAMLTest : public ::testing::Test {
Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -479,7 +479,13 @@
   : Stat(std::move(Stat)), Kind(Kind) {}
   virtual ~InMemoryNode() = default;
 
-  const Status () const { return Stat; }
+  /// Return the \p Status for this node. \p RequestedName should be the name
+  /// through which the caller referred to this node. It will override
+  /// \p Status::Name in the return value, to mimic the behavior of \p RealFile.
+  Status getStatus(std::string RequestedName) const {
+return Status::copyWithNewName(Stat, RequestedName);
+  }
+  StringRef getName() const { return Stat.getName(); }
   InMemoryNodeKind getKind() const { return Kind; }
   virtual std::string toString(unsigned Indent) const = 0;
 };
@@ -496,22 +502,30 @@
   llvm::MemoryBuffer *getBuffer() { return Buffer.get(); }
 
   std::string toString(unsigned Indent) const override {
-return (std::string(Indent, ' ') + getStatus().getName() + "\n").str();
+return (std::string(Indent, ' ') + getName() + "\n").str();
   }
 
   static bool classof(const InMemoryNode *N) {
 return N->getKind() == IME_File;
   }
 };
 
-/// Adapt a InMemoryFile for VFS' File interface.
+/// Adapt a InMemoryFile for VFS' File interface.  The goal is to make
+/// \p InMemoryFileAdaptor mimic as much as possible the behavior of
+/// \p RealFile.
 class InMemoryFileAdaptor : public File {
   InMemoryFile 
 
+  /// The name to use when returning a Status for this file.
+  std::string RequestedName;
+
 public:
-  explicit InMemoryFileAdaptor(InMemoryFile ) : Node(Node) {}
+  explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+  : Node(Node), RequestedName(std::move(RequestedName)) {}
 
-  llvm::ErrorOr status() override { 

[PATCH] D48903: [VirtualFileSystem] InMemoryFileSystem::status: Return a Status with the requested name

[Simon Marchi via Phabricator via cfe-commits]

simark marked 6 inline comments as done.
simark added inline comments.



Comment at: lib/Basic/VirtualFileSystem.cpp:516
+  explicit InMemoryFileAdaptor(InMemoryFile , std::string RequestedName)
+  : Node(Node), RequestedName (std::move (RequestedName))
+  {}

ilya-biryukov wrote:
> NIT: The formatting is broken here.
Hmm this is what git-clang-format does (unless this comment refers to a 
previous version where I had not run clang-format).



Comment at: lib/Basic/VirtualFileSystem.cpp:520
+  llvm::ErrorOr status() override {
+Status St = Node.getStatus();
+return Status::copyWithNewName(St, RequestedName);

ilya-biryukov wrote:
> Maybe add a `RequestedName` parameter to the `InMemoryNode` instead to make 
> sure it's not misused?
> It looks like all the clients calling it have to change the name and some are 
> not doing it now, e.g. the directory iterator will use statuses with full 
> paths.
Ok, I tried to do something like that.



Comment at: lib/Basic/VirtualFileSystem.cpp:521
+Status St = Node.getStatus();
+return Status::copyWithNewName(St, RequestedName);
+  }

ilya-biryukov wrote:
> Maybe add a comment that this matches the real file system behavior?
I added a comment to `InMemoryNode::getSatus`, since that's where the 
`copyWithNewName` is done now.



Comment at: lib/Basic/VirtualFileSystem.cpp:724
+  Status St = (*Node)->getStatus();
+  return Status::copyWithNewName(St, Path.str());
+}

ilya-biryukov wrote:
> NIT: we don't need `str()` here
Otherwise I'm getting this:

```
/home/emaisin/src/llvm/tools/clang/lib/Basic/VirtualFileSystem.cpp:1673:9: 
error: no matching function for call to 'copyWithNewName'
S = Status::copyWithNewName(S, Path);
^~~
/home/emaisin/src/llvm/tools/clang/lib/Basic/VirtualFileSystem.cpp:76:16: note: 
candidate function not viable: no known conversion from 'const llvm::Twine' to 
'llvm::StringRef' for 2nd argument
Status Status::copyWithNewName(const Status , StringRef NewName) {
   ^
/home/emaisin/src/llvm/tools/clang/lib/Basic/VirtualFileSystem.cpp:82:16: note: 
candidate function not viable: no known conversion from 'clang::vfs::Status' to 
'const llvm::sys::fs::file_status' for 1st argument
Status Status::copyWithNewName(const file_status , StringRef NewName) {
   ^
```


Repository:
  rC Clang

https://reviews.llvm.org/D48903



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits