from:"Alex Lorenz via Phabricator via cfe\-commits"

[PATCH] D50154: [clangd] capitalize diagnostic messages

2018-08-07 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

In https://reviews.llvm.org/D50154#1191002, @dblaikie wrote:

> What's the motivation for clangd to differ from clang here? (& if the first
>  letter is going to be capitalized, should there be a period at the end? But
>  also the phrasing of most/all diagnostic text isn't in the form of complete
>  sentences, so this might not make sense)

It's mostly for the presentation purposes, to match the needs of our client. I 
first implemented it as an opt-in feature, but the consensus was to capitalize 
the messages all the time. 
I don't think it would make sense to insert the period at the end, because, as 
you said, not all diagnostics are complete sentences

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] D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested)

2018-08-07 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.
arphaman added reviewers: jkorous, sammccall, ilya-biryukov.
Herald added subscribers: dexonsmith, MaskRay, ioeric.

This change extends the 'textDocument/publishDiagnostics' notification sent 
from Clangd to the client. The extension can be enabled using the 
'-fixit-usage=embed-in-diagnostic' argument. When it's enabled, Clangd sends 
out the fixits associated with the appropriate diagnostic in the body of the 
'publicDiagnostics' notification.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50415

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/Diagnostics.h
  clangd/fuzzer/ClangdFuzzer.cpp
  clangd/tool/ClangdMain.cpp
  test/clangd/fixits-embed-in-diagnostic.test

Index: test/clangd/fixits-embed-in-diagnostic.test
===
--- /dev/null
+++ test/clangd/fixits-embed-in-diagnostic.test
@@ -0,0 +1,66 @@
+# RUN: clangd -fixit-usage=embed-in-diagnostic -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"struct Point {}; union Point p;"}}}
+#  CHECK:"method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:"params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"clangd.fixes": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"edit": {
+# CHECK-NEXT:  "changes": {
+# CHECK-NEXT:"file://{{.*}}/foo.c": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"newText": "struct",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 22,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 17,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:]
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"title": "change 'union' to 'struct'"
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"message": "Use of 'Point' with tag type that does not match previous declaration\n\nfoo.c:1:8: note: previous use is here",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 22,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 17,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"severity": 1
+# CHECK-NEXT:  },
+# CHECK-NEXT:  {
+# CHECK-NEXT:"message": "Previous use is here\n\nfoo.c:1:18: error: use of 'Point' with tag type that does not match previous declaration",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 12,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 7,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"severity": 3
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"uri": "file://{{.*}}/foo.c"
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -185,6 +185,21 @@
 "'compile_commands.json' files")),
 llvm::cl::init(FilesystemCompileArgs), llvm::cl::Hidden);
 
+static llvm::cl::opt FixitUsage(
+"fixit-usage",
+llvm::cl::desc("Controls how the diagnostic fixits are used by the client"),
+llvm::cl::values(
+clEnumValN(ClangdDiagnosticOptions::FixitUsageOption::CodeAction,
+   "code-action",
+   "The fixits can be discovered by the client using the "
+   "'textDocument/codeAction' request"),
+clEnumValN(ClangdDiagnosticOptions::FixitUsageOption::EmbedInDiagnostic,
+   "embed-in-diagnostic",
+   "The fixits are sent to the client together with the "
+   "diagnostics using an LSP extension")),
+llvm::cl::init(ClangdDiagnosticOptions::FixitUsageOption::CodeAction),
+llvm::cl::Hidden);
+
 int main(int argc, char *argv[]) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   llvm::cl::SetVersionPrinter([](llvm::raw_ostream &OS) {

[PATCH] D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested)

2018-08-08 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 159760.
arphaman added a comment.

- Client now can request the fixes using a 
'clientCapabilities/textDocument/publishDiagnostics' extension.
- Use 'Fixes' instead of 'Fixits' in code.


https://reviews.llvm.org/D50415

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/Diagnostics.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/fuzzer/ClangdFuzzer.cpp
  clangd/tool/ClangdMain.cpp
  test/clangd/fixits-embed-in-diagnostic.test

Index: test/clangd/fixits-embed-in-diagnostic.test
===
--- /dev/null
+++ test/clangd/fixits-embed-in-diagnostic.test
@@ -0,0 +1,66 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"publishDiagnostics":{"clangdFixSupport":true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"struct Point {}; union Point p;"}}}
+#  CHECK:"method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:"params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"clangd.fixes": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"edit": {
+# CHECK-NEXT:  "changes": {
+# CHECK-NEXT:"file://{{.*}}/foo.c": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"newText": "struct",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 22,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 17,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:]
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"title": "change 'union' to 'struct'"
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"message": "Use of 'Point' with tag type that does not match previous declaration\n\nfoo.c:1:8: note: previous use is here",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 22,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 17,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"severity": 1
+# CHECK-NEXT:  },
+# CHECK-NEXT:  {
+# CHECK-NEXT:"message": "Previous use is here\n\nfoo.c:1:18: error: use of 'Point' with tag type that does not match previous declaration",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 12,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 7,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"severity": 3
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"uri": "file://{{.*}}/foo.c"
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -300,9 +300,11 @@
 CCOpts.IncludeIndicator.NoInsert.clear();
   }
 
+  ClangdDiagnosticOptions DiagOpts;
+
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
-  Out, CCOpts, CompileCommandsDirPath,
+  Out, CCOpts, DiagOpts, CompileCommandsDirPath,
   /*ShouldUseInMemoryCDB=*/CompileArgsFrom == LSPCompileArgs, Opts);
   constexpr int NoShutdownRequestErrorCode = 1;
   llvm::set_thread_name("clangd.main");
Index: clangd/fuzzer/ClangdFuzzer.cpp
===
--- clangd/fuzzer/ClangdFuzzer.cpp
+++ clangd/fuzzer/ClangdFuzzer.cpp
@@ -27,11 +27,12 @@
 clang::clangd::Logger::Error, nullptr);
   clang::clangd::CodeCompleteOptions CCOpts;
   CCOpts.EnableSnippets = false;
+  ClangdDiagnosticOptions DiagOpts;
   clang::clangd::ClangdServer::Options Opts;
 
   // Initialize and run ClangdLSPServer.
-  clang::clangd::ClangdLSPServer LSPServer(Out, CCOpts, llvm::None, false,
-   Opts);
+  clang::clangd::ClangdLSPServer LSPServer(Out, CCOpts, DiagOpts, llvm::None,
+   false, Opts);
   // fmemopen isn't portable, but I think we only run the fuzzer on Linux.
   LSPServer.run(fmemopen(d

[PATCH] D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected

2018-08-08 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

This seems sensible to me.




Comment at: include/clang/Basic/Diagnostic.h:764
+  /// off extra processing that might be wasteful in this case.
+bool areDiagnosticsSuppressedAfterFatalError() const {
+return FatalErrorOccurred && SuppressAfterFatalError;

Looks like this line wasn't formatted with clang-format.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50462



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

[PATCH] D50462: Try building complete AST after a fatal error was emitted if further diagnostics are expected

2018-08-08 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

On a second look I think that there is value separating the concepts of 
'producing diagnostics' after hitting the fatal error (which is 
SuppressAfterFatalError currently does), and trying to build a more complete 
AST.
For example,

- An editor client might not want to show the diagnostics after the fatal error 
has been reached to match the diagnostics observed during build, but it would 
benefit from a more complete AST for other editing features.
- Index-while-building: the compiler shouldn't show the diagnostics after a 
fatal error has been reached, but the indexer would be able to produce better 
indexing data from a more complete AST.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50462



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

[PATCH] D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested)

2018-08-09 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 160007.
arphaman marked an inline comment as done.
arphaman added a comment.

remove parameter.


https://reviews.llvm.org/D50415

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/Diagnostics.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/fixits-embed-in-diagnostic.test

Index: test/clangd/fixits-embed-in-diagnostic.test
===
--- /dev/null
+++ test/clangd/fixits-embed-in-diagnostic.test
@@ -0,0 +1,66 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"publishDiagnostics":{"clangdFixSupport":true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"struct Point {}; union Point p;"}}}
+#  CHECK:"method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:"params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"clangd.fixes": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"edit": {
+# CHECK-NEXT:  "changes": {
+# CHECK-NEXT:"file://{{.*}}/foo.c": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"newText": "struct",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 22,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 17,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:}
+# CHECK-NEXT:  }
+# CHECK-NEXT:]
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"title": "change 'union' to 'struct'"
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"message": "Use of 'Point' with tag type that does not match previous declaration\n\nfoo.c:1:8: note: previous use is here",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 22,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 17,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"severity": 1
+# CHECK-NEXT:  },
+# CHECK-NEXT:  {
+# CHECK-NEXT:"message": "Previous use is here\n\nfoo.c:1:18: error: use of 'Point' with tag type that does not match previous declaration",
+# CHECK-NEXT:"range": {
+# CHECK-NEXT:  "end": {
+# CHECK-NEXT:"character": 12,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  },
+# CHECK-NEXT:  "start": {
+# CHECK-NEXT:"character": 7,
+# CHECK-NEXT:"line": 0
+# CHECK-NEXT:  }
+# CHECK-NEXT:},
+# CHECK-NEXT:"severity": 3
+# CHECK-NEXT:  }
+# CHECK-NEXT:],
+# CHECK-NEXT:"uri": "file://{{.*}}/foo.c"
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":4,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -247,6 +247,18 @@
 };
 bool fromJSON(const llvm::json::Value &, CompletionClientCapabilities &);
 
+struct PublishDiagnosticsClientCapabilities {
+  // Whether the client accepts diagnostics with related information.
+  // NOTE: not used by clangd at the moment.
+  // bool relatedInformation;
+
+  /// Whether the client accepts diagnostics with fixes attached using the
+  /// "clangd.fixes" extension.
+  bool clangdFixSupport = false;
+};
+bool fromJSON(const llvm::json::Value &,
+  PublishDiagnosticsClientCapabilities &);
+
 /// A symbol kind.
 enum class SymbolKind {
   File = 1,
@@ -313,6 +325,9 @@
 struct TextDocumentClientCapabilities {
   /// Capabilities specific to the `textDocument/completion`
   CompletionClientCapabilities completion;
+
+  /// Capabilities specific to the 'textDocument/publishDiagnostics'
+  PublishDiagnosticsClientCapabilities publishDiagnostics;
 };
 bool fromJSON(const llvm::json::Value &, TextDocumentClientCapabilities &);
 
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -178,6 +178,15 @@
   return true;
 }
 
+bool fromJSON(const llvm::json::Value &Params,
+  PublishDiagnosticsClientCapabilities &R) {
+  json::ObjectMapper O(Params);
+  if (!O)
+return false;
+  O.map("clangdFixSupport", R.clangdFixSupport);
+  return true;
+}
+
 bool fromJSON(const json::Value &E, SymbolKind &Out) {

[PATCH] D50415: [clangd] extend the publishDiagnostics response to send back fixits to the client directly as well (if requested)

2018-08-10 Thread Alex Lorenz via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
arphaman marked an inline comment as done.
Closed by commit rCTE339454: [clangd] extend the publishDiagnostics response to 
send back fixits to the (authored by arphaman, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D50415?vs=160007&id=160137#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50415

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

Index: clangd/ClangdLSPServer.h
===
--- clangd/ClangdLSPServer.h
+++ clangd/ClangdLSPServer.h
@@ -155,6 +155,8 @@
   RealFileSystemProvider FSProvider;
   /// Options used for code completion
   clangd::CodeCompleteOptions CCOpts;
+  /// Options used for diagnostics.
+  ClangdDiagnosticOptions DiagOpts;
   /// The supported kinds of the client.
   SymbolKindBitset SupportedSymbolKinds;
 
Index: clangd/Diagnostics.h
===
--- clangd/Diagnostics.h
+++ clangd/Diagnostics.h
@@ -23,6 +23,12 @@
 namespace clang {
 namespace clangd {
 
+struct ClangdDiagnosticOptions {
+  /// If true, Clangd uses an LSP extension to embed the fixes with the
+  /// diagnostics that are sent to the client.
+  bool EmbedFixesInDiagnostics = false;
+};
+
 /// Contains basic information about a diagnostic.
 struct DiagBase {
   std::string Message;
Index: clangd/Protocol.cpp
===
--- clangd/Protocol.cpp
+++ clangd/Protocol.cpp
@@ -178,6 +178,15 @@
   return true;
 }
 
+bool fromJSON(const llvm::json::Value &Params,
+  PublishDiagnosticsClientCapabilities &R) {
+  json::ObjectMapper O(Params);
+  if (!O)
+return false;
+  O.map("clangdFixSupport", R.clangdFixSupport);
+  return true;
+}
+
 bool fromJSON(const json::Value &E, SymbolKind &Out) {
   if (auto T = E.getAsInteger()) {
 if (*T < static_cast(SymbolKind::File) ||
@@ -240,6 +249,7 @@
   if (!O)
 return false;
   O.map("completion", R.completion);
+  O.map("publishDiagnostics", R.publishDiagnostics);
   return true;
 }
 
Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -82,6 +82,8 @@
 
   CCOpts.EnableSnippets =
   Params.capabilities.textDocument.completion.completionItem.snippetSupport;
+  DiagOpts.EmbedFixesInDiagnostics =
+  Params.capabilities.textDocument.publishDiagnostics.clangdFixSupport;
 
   if (Params.capabilities.workspace && Params.capabilities.workspace->symbol &&
   Params.capabilities.workspace->symbol->symbolKind) {
@@ -486,11 +488,25 @@
   DiagnosticToReplacementMap LocalFixIts; // Temporary storage
   for (auto &Diag : Diagnostics) {
 toLSPDiags(Diag, [&](clangd::Diagnostic Diag, llvm::ArrayRef Fixes) {
-  DiagnosticsJSON.push_back(json::Object{
+  json::Object LSPDiag({
   {"range", Diag.range},
   {"severity", Diag.severity},
   {"message", Diag.message},
   });
+  // LSP extension: embed the fixes in the diagnostic.
+  if (DiagOpts.EmbedFixesInDiagnostics && !Fixes.empty()) {
+json::Array ClangdFixes;
+for (const auto &Fix : Fixes) {
+  WorkspaceEdit WE;
+  URIForFile URI{File};
+  WE.changes = {{URI.uri(), std::vector(Fix.Edits.begin(),
+  Fix.Edits.end())}};
+  ClangdFixes.push_back(
+  json::Object{{"edit", toJSON(WE)}, {"title", Fix.Message}});
+}
+LSPDiag["clangd_fixes"] = std::move(ClangdFixes);
+  }
+  DiagnosticsJSON.push_back(std::move(LSPDiag));
 
   auto &FixItsForDiagnostic = LocalFixIts[Diag];
   std::copy(Fixes.begin(), Fixes.end(),
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -247,6 +247,18 @@
 };
 bool fromJSON(const llvm::json::Value &, CompletionClientCapabilities &);
 
+struct PublishDiagnosticsClientCapabilities {
+  // Whether the client accepts diagnostics with related information.
+  // NOTE: not used by clangd at the moment.
+  // bool relatedInformation;
+
+  /// Whether the client accepts diagnostics with fixes attached using the
+  /// "clangd_fixes" extension.
+  bool clangdFixSupport = false;
+};
+bool fromJSON(const llvm::json::Value &,
+  PublishDiagnosticsClientCapabilities &);
+
 /// A symbol kind.
 enum class SymbolKind {
   File = 1,
@@ -313,6 +325,9 @@
 struct TextDocumentClientCapabilities {
   /// Capabilities specific to the `textDocument/completion`
   CompletionClientCapabilities completion;
+
+  /// Capabilities specific to the 'textDocument/publishDiagnostics'
+  PublishDiagnosticsClientCapabilities publishDiagnostics;
 };
 bool fromJSON(const ll

[PATCH] D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category

2018-08-10 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.
arphaman added reviewers: jkorous, ilya-biryukov, sammccall.
Herald added subscribers: dexonsmith, MaskRay, ioeric.

This patch adds a 'category' extension field to the LSP diagnostic that's sent 
by Clangd. This extension is always on by default.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  clangd/Protocol.h
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/compile-commands-path.test
  test/clangd/diagnostics.test
  test/clangd/did-change-configuration-params.test
  test/clangd/execute-command.test
  test/clangd/extra-flags.test
  test/clangd/fixits.test

Index: test/clangd/fixits.test
===
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -6,6 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/extra-flags.test
===
--- test/clangd/extra-flags.test
+++ test/clangd/extra-flags.test
@@ -6,6 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
@@ -28,6 +29,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/execute-command.test
===
--- test/clangd/execute-command.test
+++ test/clangd/execute-command.test
@@ -6,6 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/did-change-configuration-params.test
===
--- test/clangd/did-change-configuration-params.test
+++ test/clangd/did-change-configuration-params.test
@@ -24,6 +24,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/diagnostics.test
===
--- test/clangd/diagnostics.test
+++ test/clangd/diagnostics.test
@@ -6,6 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Return type of 'main' is not 'int'",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/compile-commands-path.test
===
--- test/clangd/compile-commands-path.test
+++ test/clangd/compile-commands-path.test
@@ -23,20 +23,23 @@
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT: "diagnostics": [
 # CHECK-NEXT:   {
+# CHECK-NEXT: "category": "#pragma message Directive",
 # 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: "category": "#pragma message Directive",
 # 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: "category": "#pragma message Directive",
 # 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-ini

[PATCH] D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category

2018-08-13 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 160464.
arphaman marked 2 inline comments as done.
arphaman added a comment.

Address review comments.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  clangd/Protocol.h
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/compile-commands-path.test
  test/clangd/diagnostics.test
  test/clangd/did-change-configuration-params.test
  test/clangd/execute-command.test
  test/clangd/extra-flags.test
  test/clangd/fixits.test

Index: test/clangd/fixits.test
===
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -6,6 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/extra-flags.test
===
--- test/clangd/extra-flags.test
+++ test/clangd/extra-flags.test
@@ -6,6 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
@@ -28,6 +29,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/execute-command.test
===
--- test/clangd/execute-command.test
+++ test/clangd/execute-command.test
@@ -6,6 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/did-change-configuration-params.test
===
--- test/clangd/did-change-configuration-params.test
+++ test/clangd/did-change-configuration-params.test
@@ -24,6 +24,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/diagnostics.test
===
--- test/clangd/diagnostics.test
+++ test/clangd/diagnostics.test
@@ -6,6 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Return type of 'main' is not 'int'",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/compile-commands-path.test
===
--- test/clangd/compile-commands-path.test
+++ test/clangd/compile-commands-path.test
@@ -23,20 +23,23 @@
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT: "diagnostics": [
 # CHECK-NEXT:   {
+# CHECK-NEXT: "category": "#pragma message Directive",
 # 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: "category": "#pragma message Directive",
 # 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: "category": "#pragma message Directive",
 # 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
@@ -23,13 +23,15 @@
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT: "diagnostics": [
 # CHECK-NEXT:   {
+# CHECK-NEXT:

[PATCH] D50641: [clangd][test] Fix exit messages in tests

2018-08-13 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

Thanks for fixing this!

You're right we should try to fix it properly to avoid such mistakes in the 
future. Checking for stderr from Clangd might work, but I don't think it's the 
most optimal solution. What do you think about Clangd exiting with failure on 
malformed JSON input when running in `-lit` mode?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50641



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

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

2018-08-14 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.
arphaman added reviewers: jkorous, klimek, ioeric, vsapsai, ilya-biryukov.
Herald added a subscriber: dexonsmith.

The current implementation of `isPointWithin` uses `isBeforeInTranslationUnit` 
to check if a location is within a range.
The problem is that `isPointWithin` is used to perform is lexical range check, 
i.e. we want to determine if a location in a particular source file is within a 
source range in the same source file.
The use of `isBeforeInTranslationUnit` is not correct for this use-case, as one 
source file can be included multiple times. Given a source range and a location 
that's lexically contained within that source range (e.g. file.h:1:2->1:10 and 
file:h:1:5),  `isBeforeInTranslationUnit` will fail to correctly determine if a 
source range from the first occurrence of the source file contains a location 
from the second occurrence of the same source file, as the start and end of the 
range are both included before the location.
This patch reimplements `isPointWithin` to ensure that buffer offsets are 
compared directly for lexical correctness.

This patch is a pre-requisuite to fix a bug in clang-rename where we are unable 
to rename a structure in a PCH that has a header that's included multiple times 
(follow-up patch will be posted to update clang-rename to use isPointWithin).

rdar://42746284


Repository:
  rC Clang

https://reviews.llvm.org/D50740

Files:
  include/clang/Basic/SourceManager.h
  lib/Basic/SourceManager.cpp
  unittests/Basic/SourceManagerTest.cpp

Index: unittests/Basic/SourceManagerTest.cpp
===
--- unittests/Basic/SourceManagerTest.cpp
+++ unittests/Basic/SourceManagerTest.cpp
@@ -377,6 +377,68 @@
   EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
 }
 
+TEST_F(SourceManagerTest, isPointWithin) {
+  const char *header = "int x;";
+
+  const char *main = "#include \n"
+ "#include \n";
+
+  std::unique_ptr HeaderBuf =
+  llvm::MemoryBuffer::getMemBuffer(header);
+  std::unique_ptr MainBuf =
+  llvm::MemoryBuffer::getMemBuffer(main);
+  SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(MainBuf)));
+
+  const FileEntry *headerFile =
+  FileMgr.getVirtualFile("/test-header.h", HeaderBuf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(headerFile, std::move(HeaderBuf));
+
+  TrivialModuleLoader ModLoader;
+  MemoryBufferCache PCMCache;
+  HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+  Diags, LangOpts, &*Target);
+  Preprocessor PP(std::make_shared(), Diags, LangOpts,
+  SourceMgr, PCMCache, HeaderInfo, ModLoader,
+  /*IILookup =*/nullptr,
+  /*OwnsHeaderSearch =*/false);
+  PP.Initialize(*Target);
+
+  PP.EnterMainSourceFile();
+
+  std::vector Tokens;
+  while (1) {
+Token tok;
+PP.Lex(tok);
+if (tok.is(tok::eof))
+  break;
+Tokens.push_back(tok);
+  }
+
+  // Make sure we got the tokens that we expected.
+  ASSERT_EQ(6U, Tokens.size());
+
+  // Make sure the FileIDs are different.
+  SourceLocation L1 = Tokens[0].getLocation();
+  SourceLocation L2 = Tokens[3].getLocation();
+  ASSERT_NE(L1, L2);
+
+  // The location of the 'int' in the first inclusion is lexically within the
+  // range of the 'int' in the second inclusion.
+  ASSERT_TRUE(SourceMgr.isPointWithin(L1, L2, L2));
+  // The location of the 'int' in the second inclusion is lexically within the
+  // range of the 'int' in the first inclusion.
+  ASSERT_TRUE(SourceMgr.isPointWithin(L2, L1, L1));
+  // The location of the 'x' in the second inclusion is lexically within the
+  // range of the 'int' in the first inclusion and ';' in the second inclusion.
+  ASSERT_TRUE(SourceMgr.isPointWithin(Tokens[4].getLocation(), L1,
+  Tokens[5].getLocation()));
+  // The location of the 'int' in the second inclusion is lexically outside of
+  // the 'x' in the first inclusion and ';' in the second inclusion.
+  ASSERT_FALSE(SourceMgr.isPointWithin(Tokens[3].getLocation(),
+   Tokens[1].getLocation(),
+   Tokens[5].getLocation()));
+}
+
 #endif
 
 } // anonymous namespace
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -2028,6 +2028,35 @@
   return IBTUCacheOverflow;
 }
 
+bool SourceManager::isPointWithin(SourceLocation Loc, SourceLocation Start,
+  SourceLocation End) const {
+  assert(Start.isValid() && End.isValid() && Loc.isValid() &&
+ "Passed invalid source location!");
+  assert(Start.isFileID() && End.isFileID() && Loc.isFileID() &&
+ "Passed non-file source location!");
+
+  std::pair PointLoc = getDecomposedLoc(Loc);
+  std::pair StartLoc = getDecomposedLoc(Start);

[PATCH] D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category

2018-08-14 Thread Alex Lorenz via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE339738: [clangd] add an extension field to LSP to transfer 
the diagnostic's category (authored by arphaman, committed by ).

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50571

Files:
  clangd/ClangdLSPServer.cpp
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  clangd/Protocol.h
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/compile-commands-path.test
  test/clangd/diagnostics.test
  test/clangd/did-change-configuration-params.test
  test/clangd/execute-command.test
  test/clangd/extra-flags.test
  test/clangd/fixits.test

Index: clangd/ClangdLSPServer.cpp
===
--- clangd/ClangdLSPServer.cpp
+++ clangd/ClangdLSPServer.cpp
@@ -506,6 +506,8 @@
 }
 LSPDiag["clangd_fixes"] = std::move(ClangdFixes);
   }
+  if (!Diag.category.empty())
+LSPDiag["category"] = Diag.category;
   DiagnosticsJSON.push_back(std::move(LSPDiag));
 
   auto &FixItsForDiagnostic = LocalFixIts[Diag];
Index: clangd/Diagnostics.cpp
===
--- clangd/Diagnostics.cpp
+++ clangd/Diagnostics.cpp
@@ -226,6 +226,7 @@
 clangd::Diagnostic Res;
 Res.range = D.Range;
 Res.severity = getSeverity(D.Severity);
+Res.category = D.Category;
 return Res;
   };
 
@@ -292,6 +293,9 @@
 D.InsideMainFile = InsideMainFile;
 D.File = Info.getSourceManager().getFilename(Info.getLocation());
 D.Severity = DiagLevel;
+D.Category = DiagnosticIDs::getCategoryNameFromID(
+ DiagnosticIDs::getCategoryNumberForDiag(Info.getID()))
+ .str();
 return D;
   };
 
Index: clangd/Diagnostics.h
===
--- clangd/Diagnostics.h
+++ clangd/Diagnostics.h
@@ -37,6 +37,7 @@
   std::string File;
   clangd::Range Range;
   DiagnosticsEngine::Level Severity = DiagnosticsEngine::Note;
+  std::string Category;
   // Since File is only descriptive, we store a separate flag to distinguish
   // diags from the main file.
   bool InsideMainFile = false;
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -544,6 +544,12 @@
 
   /// The diagnostic's message.
   std::string message;
+
+  /// The diagnostic's category. Can be omitted.
+  /// An LSP extension that's used to send the name of the category over to the
+  /// client. The category typically describes the compilation stage during
+  /// which the issue was produced, e.g. "Semantic Issue" or "Parse Issue".
+  std::string category;
 };
 
 /// A LSP-specific comparator used to find diagnostic in a container like
Index: test/clangd/did-change-configuration-params.test
===
--- test/clangd/did-change-configuration-params.test
+++ test/clangd/did-change-configuration-params.test
@@ -24,6 +24,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/fixits.test
===
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -6,6 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
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
@@ -23,13 +23,15 @@
 # CHECK-NEXT:   "params": {
 # CHECK-NEXT: "diagnostics": [
 # CHECK-NEXT:   {
+# CHECK-NEXT: "category": "#pragma message Directive",
 # 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: "category": "#pragma message Directive",
 # CHECK-NEXT: "message": "MACRO is two",
 ---
 {"jsonrpc":"2.0","id":1,"method":"shutdown"}
Index: test/clangd/compile-commands-path.test
===
--- test/clangd/compile-commands-path.test
+++ test/clangd/compile-commands-path.test
@@ -23,20 +23,23 @@

[PATCH] D50785: [clangd][tests] Add exit(EXIT_FAILURE) in case of JSON parsing failure in TestMode

2018-08-15 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

I think propagating the 'test' yes/no value is not the best way to describe the 
intention of this change.
Our intention is to exit from the LSP server on JSON error. One way to describe 
this intention better is to propagate a boolean 'exitOnJSONError' value.
Furthermore, If you think about the '-llvm-lit' flag itself you'll see that it 
acts an abbreviation for these 3 options: `-input-style=delimited -pretty 
-run-synchronously`. I think that it would be valuable to follow this intention 
and to add a new option (e.g. `-exit-on-json-error`) that will then be set when 
`-llvm-lit` is set.
WDYT?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50785



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

[PATCH] D50801: [rename] Use isPointWithin when looking for a declaration at location

2018-08-15 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.
arphaman added reviewers: jkorous, hokein, ioeric.
Herald added a subscriber: dexonsmith.

This patch is a followup to https://reviews.llvm.org/D50740 .

This patch fixes the issue where clang-refactor local-rename was unable to find 
a declaration in a header file if that header file was included more than once, 
and the location of the point of interest was in a different included instance 
of a file than then declaration itself. The use of `isPointWithin` ensures that 
we're checking if the point is in the source range regardless of the included 
file.


Repository:
  rC Clang

https://reviews.llvm.org/D50801

Files:
  lib/Tooling/Refactoring/Rename/USRFinder.cpp
  test/Refactor/LocalRename/Inputs/MultiHeader.h
  test/Refactor/LocalRename/IsPointWithinDifferentInclude.c


Index: test/Refactor/LocalRename/IsPointWithinDifferentInclude.c
===
--- /dev/null
+++ test/Refactor/LocalRename/IsPointWithinDifferentInclude.c
@@ -0,0 +1,16 @@
+// RUN: clang-refactor local-rename -selection=%S/Inputs/MultiHeader.h:5:9 
-new-name=renamedField %s -- -I %S/Inputs 2>&1 | FileCheck %s
+
+// The MultiHeader file included twice.
+// The first inclusion contains no declarations. However, the selection
+// "MultiHeader:5:9" is mapped to a location in this inclusion of the file.
+#include "MultiHeader.h"
+
+// The second inclusion contains the declarations. The range of the
+// declaration we would like is located in this inclusion.
+#define HAVE_DECLS
+#include "MultiHeader.h"
+
+// Ensure that we still find the declaration even though the location and the
+// declaration's range are located in two different inclusions.
+// CHECK:  struct Foo {
+// CHECK-NEXT:   int renamedField;
Index: test/Refactor/LocalRename/Inputs/MultiHeader.h
===
--- /dev/null
+++ test/Refactor/LocalRename/Inputs/MultiHeader.h
@@ -0,0 +1,9 @@
+#ifdef HAVE_DECLS
+
+struct MyStruct {
+  struct Foo {
+int field;
+  } bar;
+};
+
+#endif
Index: lib/Tooling/Refactoring/Rename/USRFinder.cpp
===
--- lib/Tooling/Refactoring/Rename/USRFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRFinder.cpp
@@ -48,7 +48,8 @@
   SourceLocation Start = Range.getBegin();
   SourceLocation End = Range.getEnd();
   if (!Start.isValid() || !Start.isFileID() || !End.isValid() ||
-  !End.isFileID() || !isPointWithin(Start, End))
+  !End.isFileID() ||
+  !Context.getSourceManager().isPointWithin(Point, Start, End))
 return true;
 }
 Result = ND;
@@ -58,14 +59,6 @@
   const NamedDecl *getNamedDecl() const { return Result; }
 
 private:
-  // Determines if the Point is within Start and End.
-  bool isPointWithin(const SourceLocation Start, const SourceLocation End) {
-// FIXME: Add tests for Point == End.
-return Point == Start || Point == End ||
-   (Context.getSourceManager().isBeforeInTranslationUnit(Start,
- Point) &&
-Context.getSourceManager().isBeforeInTranslationUnit(Point, End));
-  }
 
   const NamedDecl *Result = nullptr;
   const SourceLocation Point; // The location to find the NamedDecl.
@@ -80,14 +73,15 @@
   NamedDeclOccurrenceFindingVisitor Visitor(Point, Context);
 
   // Try to be clever about pruning down the number of top-level declarations 
we
-  // see. If both start and end is either before or after the point we're
-  // looking for the point cannot be inside of this decl. Don't even look at 
it.
+  // see. If the range of the declaration doesn't contain the source location,
+  // then don't even look at it.
   for (auto *CurrDecl : Context.getTranslationUnitDecl()->decls()) {
-SourceLocation StartLoc = CurrDecl->getBeginLoc();
-SourceLocation EndLoc = CurrDecl->getEndLoc();
-if (StartLoc.isValid() && EndLoc.isValid() &&
-SM.isBeforeInTranslationUnit(StartLoc, Point) !=
-SM.isBeforeInTranslationUnit(EndLoc, Point))
+CharSourceRange DeclRange = Lexer::makeFileCharRange(
+CharSourceRange::getTokenRange(CurrDecl->getSourceRange()),
+Context.getSourceManager(), Context.getLangOpts());
+// FIXME: Use early break if the decl is found.
+if (DeclRange.isValid() &&
+SM.isPointWithin(Point, DeclRange.getBegin(), DeclRange.getEnd()))
   Visitor.TraverseDecl(CurrDecl);
   }
 


Index: test/Refactor/LocalRename/IsPointWithinDifferentInclude.c
===
--- /dev/null
+++ test/Refactor/LocalRename/IsPointWithinDifferentInclude.c
@@ -0,0 +1,16 @@
+// RUN: clang-refactor local-rename -selection=%S/Inputs/MultiHeader.h:5:9 -new-name=renamedField %s -- -I %S/Inputs 2>&1 | FileCheck %s
+
+// The MultiHeader file included twice.
+// The first inclusion contains no declarations. However,

[PATCH] D50814: [clangd] transfer the fixits with the notes instead of adding them to the main diagnostic if request by the client

2018-08-15 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.
arphaman added reviewers: ilya-biryukov, jkorous, sammccall.
Herald added subscribers: cfe-commits, dexonsmith, MaskRay, ioeric, javed.absar.

This patch extends Clangd to allow the clients to specify the preference for 
how it wants to consume the fixes on the notes. If `AllowNotesWithFixes` is 
true in the DiagnosticOptions, then Clangd will keep the notes with the fixes 
instead of converting to fixes in the main diagnostic. This extension can be 
enabled by the client using the "clangdNotesWithFixSupport" capability.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50814

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  test/clangd/fixits-embed-in-diagnostic.test
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FindSymbolsTests.cpp
  unittests/clangd/TUSchedulerTests.cpp
  unittests/clangd/XRefsTests.cpp

Index: unittests/clangd/XRefsTests.cpp
===
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -339,7 +339,9 @@
 
   IgnoreDiagnostics DiagConsumer;
   MockFSProvider FS;
-  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+  ClangdDiagnosticOptions DiagOpts;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest(),
+  DiagOpts);
 
   // Fill the filesystem.
   auto FooCpp = testPath("src/foo.cpp");
@@ -958,7 +960,9 @@
   MockFSProvider FS;
   IgnoreDiagnostics DiagConsumer;
   MockCompilationDatabase CDB;
-  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+  ClangdDiagnosticOptions DiagOpts;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest(),
+  DiagOpts);
 
   auto FooCpp = testPath("foo.cpp");
   const char *SourceContents = R"cpp(
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -43,11 +43,12 @@
 };
 
 TEST_F(TUSchedulerTests, MissingFiles) {
+  ClangdDiagnosticOptions DiagOpts;
   TUScheduler S(getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true,
 /*PreambleParsedCallback=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-ASTRetentionPolicy());
+ASTRetentionPolicy(), DiagOpts);
 
   auto Added = testPath("added.cpp");
   Files[Added] = "";
@@ -99,12 +100,13 @@
 // To avoid a racy test, don't allow tasks to actualy run on the worker
 // thread until we've scheduled them all.
 Notification Ready;
+ClangdDiagnosticOptions DiagOpts;
 TUScheduler S(
 getDefaultAsyncThreadsCount(),
 /*StorePreamblesInMemory=*/true,
 /*PreambleParsedCallback=*/nullptr,
 /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-ASTRetentionPolicy());
+ASTRetentionPolicy(), DiagOpts);
 auto Path = testPath("foo.cpp");
 S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
  [&](std::vector) { Ready.wait(); });
@@ -129,11 +131,12 @@
 TEST_F(TUSchedulerTests, Debounce) {
   std::atomic CallbackCount(0);
   {
+ClangdDiagnosticOptions DiagOpts;
 TUScheduler S(getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true,
   /*PreambleParsedCallback=*/nullptr,
   /*UpdateDebounce=*/std::chrono::seconds(1),
-  ASTRetentionPolicy());
+  ASTRetentionPolicy(), DiagOpts);
 // FIXME: we could probably use timeouts lower than 1 second here.
 auto Path = testPath("foo.cpp");
 S.update(Path, getInputs(Path, "auto (debounced)"), WantDiagnostics::Auto,
@@ -161,11 +164,12 @@
 
   // Run TUScheduler and collect some stats.
   {
+ClangdDiagnosticOptions DiagOpts;
 TUScheduler S(getDefaultAsyncThreadsCount(),
   /*StorePreamblesInMemory=*/true,
   /*PreambleParsedCallback=*/nullptr,
   /*UpdateDebounce=*/std::chrono::milliseconds(50),
-  ASTRetentionPolicy());
+  ASTRetentionPolicy(), DiagOpts);
 
 std::vector Files;
 for (int I = 0; I < FilesCount; ++I) {
@@ -259,10 +263,12 @@
   std::atomic BuiltASTCounter(0);
   ASTRetentionPolicy Policy;
   Policy.MaxRetainedASTs = 2;
+  ClangdDiagnosticOptions DiagOpts;
   TUScheduler S(
   /*AsyncThreadsCount=*/1, /*StorePreambleInMemory=*/true,
   PreambleParsedCallback(),
-  /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(), Policy);
+  /*UpdateDebounce=*/std::chrono::steady_clock::duration:

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

2018-08-16 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

In https://reviews.llvm.org/D50740#1202248, @jkorous wrote:

> Hi Alex, nice work!
>
> I am just wondering if it would be beneficial to assert Loc and End are in 
> the same file. It might help to catch bugs.

I don't see the value in that unless I'm misunderstanding something. We already 
check if Loc and End are in the same file, and return false if they're not.

> I also stumbled upon this function but not sure it makes things significantly 
> cleaner here:
>  https://clang.llvm.org/doxygen/SourceLocation_8h_source.html#l00175
> 
> LGTM otherwise.

Repository:
  rC Clang

https://reviews.llvm.org/D50740

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

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

2018-08-16 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman marked 2 inline comments as done.
arphaman added inline comments.



Comment at: lib/Basic/SourceManager.cpp:2035
+ "Passed invalid source location!");
+  assert(Start.isFileID() && End.isFileID() && Loc.isFileID() &&
+ "Passed non-file source location!");

ioeric wrote:
> Why do we disallow locations from macro expansions?
I changed the patch to allow them by working with spelling locs.


Repository:
  rC Clang

https://reviews.llvm.org/D50740



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

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

2018-08-16 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 161132.
arphaman marked an inline comment as done.
arphaman added a comment.

- Use lambda
- Work with spelling locs


Repository:
  rC Clang

https://reviews.llvm.org/D50740

Files:
  include/clang/Basic/SourceManager.h
  lib/Basic/SourceManager.cpp
  unittests/Basic/SourceManagerTest.cpp

Index: unittests/Basic/SourceManagerTest.cpp
===
--- unittests/Basic/SourceManagerTest.cpp
+++ unittests/Basic/SourceManagerTest.cpp
@@ -377,6 +377,123 @@
   EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
 }
 
+TEST_F(SourceManagerTest, isPointWithin) {
+  const char *header = "int x;";
+
+  const char *main = "#include \n"
+ "#include \n";
+
+  std::unique_ptr HeaderBuf =
+  llvm::MemoryBuffer::getMemBuffer(header);
+  std::unique_ptr MainBuf =
+  llvm::MemoryBuffer::getMemBuffer(main);
+  SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(MainBuf)));
+
+  const FileEntry *headerFile =
+  FileMgr.getVirtualFile("/test-header.h", HeaderBuf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(headerFile, std::move(HeaderBuf));
+
+  TrivialModuleLoader ModLoader;
+  MemoryBufferCache PCMCache;
+  HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+  Diags, LangOpts, &*Target);
+  Preprocessor PP(std::make_shared(), Diags, LangOpts,
+  SourceMgr, PCMCache, HeaderInfo, ModLoader,
+  /*IILookup =*/nullptr,
+  /*OwnsHeaderSearch =*/false);
+  PP.Initialize(*Target);
+
+  PP.EnterMainSourceFile();
+
+  std::vector Tokens;
+  while (1) {
+Token tok;
+PP.Lex(tok);
+if (tok.is(tok::eof))
+  break;
+Tokens.push_back(tok);
+  }
+
+  // Make sure we got the tokens that we expected.
+  ASSERT_EQ(6U, Tokens.size());
+
+  // Make sure the FileIDs are different.
+  SourceLocation L1 = Tokens[0].getLocation();
+  SourceLocation L2 = Tokens[3].getLocation();
+  ASSERT_NE(L1, L2);
+
+  // The location of the 'int' in the first inclusion is lexically within the
+  // range of the 'int' in the second inclusion.
+  ASSERT_TRUE(SourceMgr.isPointWithin(L1, L2, L2));
+  // The location of the 'int' in the second inclusion is lexically within the
+  // range of the 'int' in the first inclusion.
+  ASSERT_TRUE(SourceMgr.isPointWithin(L2, L1, L1));
+  // The location of the 'x' in the second inclusion is lexically within the
+  // range of the 'int' in the first inclusion and ';' in the second inclusion.
+  ASSERT_TRUE(SourceMgr.isPointWithin(Tokens[4].getLocation(), L1,
+  Tokens[5].getLocation()));
+  // The location of the 'int' in the second inclusion is lexically outside of
+  // the 'x' in the first inclusion and ';' in the second inclusion.
+  ASSERT_FALSE(SourceMgr.isPointWithin(Tokens[3].getLocation(),
+   Tokens[1].getLocation(),
+   Tokens[5].getLocation()));
+}
+
+TEST_F(SourceManagerTest, isPointWithinMacroSpelling) {
+  const char *main = "#define MYMACRO int x;\n"
+ "MYMACRO\n"
+ "MYMACRO\n";
+
+  std::unique_ptr MainBuf =
+  llvm::MemoryBuffer::getMemBuffer(main);
+  SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(MainBuf)));
+
+  TrivialModuleLoader ModLoader;
+  MemoryBufferCache PCMCache;
+  HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+  Diags, LangOpts, &*Target);
+  Preprocessor PP(std::make_shared(), Diags, LangOpts,
+  SourceMgr, PCMCache, HeaderInfo, ModLoader,
+  /*IILookup =*/nullptr,
+  /*OwnsHeaderSearch =*/false);
+  PP.Initialize(*Target);
+
+  PP.EnterMainSourceFile();
+
+  std::vector Tokens;
+  while (1) {
+Token tok;
+PP.Lex(tok);
+if (tok.is(tok::eof))
+  break;
+Tokens.push_back(tok);
+  }
+
+  // Make sure we got the tokens that we expected.
+  ASSERT_EQ(6U, Tokens.size());
+
+  // Make sure the FileIDs are different.
+  SourceLocation L1 = Tokens[0].getLocation();
+  SourceLocation L2 = Tokens[3].getLocation();
+  ASSERT_NE(L1, L2);
+
+  // The location of the 'int' in the first expansion is lexically within the
+  // range of the 'int' in the second expansion.
+  ASSERT_TRUE(SourceMgr.isPointWithin(L1, L2, L2));
+  // The location of the 'int' in the second expansion is lexically within the
+  // range of the 'int' in the first expansion.
+  ASSERT_TRUE(SourceMgr.isPointWithin(L2, L1, L1));
+  // The location of the 'x' in the second expansion is lexically within the
+  // range of the 'int' in the first expansion and ';' in the second expansion.
+  ASSERT_TRUE(SourceMgr.isPointWithin(Tokens[4].getLocation(), L1,
+  Tokens[5].getLocation()));
+  // The location of the 'int' in the second expansion is lexically outside of
+  // the 'x' in the firs

[PATCH] D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression

2018-08-16 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 161142.
arphaman marked 3 inline comments as done.
arphaman added a comment.

address review comments.


Repository:
  rC Clang

https://reviews.llvm.org/D49462

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/CodeGen/CGObjCMac.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaExprObjC.cpp
  test/CodeGenObjC/forward-declare-protocol-gnu.m
  test/CodeGenObjC/forward-protocol-metadata-symbols.m
  test/CodeGenObjC/hidden-visibility.m
  test/CodeGenObjC/link-errors.m
  test/CodeGenObjC/protocol-comdat.m
  test/CodeGenObjC/protocols-lazy.m
  test/CodeGenObjC/protocols.m
  test/PCH/objc_exprs.h
  test/Parser/objc-cxx-keyword-identifiers.mm
  test/SemaObjC/protocol-expr-neg-1.m

Index: test/SemaObjC/protocol-expr-neg-1.m
===
--- test/SemaObjC/protocol-expr-neg-1.m
+++ test/SemaObjC/protocol-expr-neg-1.m
@@ -12,7 +12,7 @@
 int main()
 {
 	Protocol *proto = @protocol(p1);
-Protocol *fproto = @protocol(fproto); // expected-warning {{@protocol is using a forward protocol declaration of 'fproto'}}
+Protocol *fproto = @protocol(fproto); // expected-error {{@protocol is using a forward protocol declaration of 'fproto'}}
 	Protocol *pp = @protocol(i); // expected-error {{cannot find protocol declaration for 'i'}}
 	Protocol *p1p = @protocol(cl); // expected-error {{cannot find protocol declaration for 'cl'}}
 }
@@ -26,9 +26,9 @@
 @end
 
 int doesConform(id foo) {
-  return [foo conformsToProtocol:@protocol(TestProtocol)]; // expected-warning {{@protocol is using a forward protocol declaration of 'TestProtocol'}}
+  return [foo conformsToProtocol:@protocol(TestProtocol)]; // expected-error {{@protocol is using a forward protocol declaration of 'TestProtocol'}}
 }
 
 int doesConformSuper(id foo) {
-  return [foo conformsToProtocol:@protocol(SuperProtocol)]; // expected-warning {{@protocol is using a forward protocol declaration of 'SuperProtocol'}}
+  return [foo conformsToProtocol:@protocol(SuperProtocol)]; // expected-error {{@protocol is using a forward protocol declaration of 'SuperProtocol'}}
 }
Index: test/Parser/objc-cxx-keyword-identifiers.mm
===
--- test/Parser/objc-cxx-keyword-identifiers.mm
+++ test/Parser/objc-cxx-keyword-identifiers.mm
@@ -18,7 +18,9 @@
 @protocol new // expected-error {{expected identifier; 'new' is a keyword in Objective-C++}}
 @end
 
-@protocol P2, delete; // expected-error {{expected identifier; 'delete' is a keyword in Objective-C++}}
+@protocol P2;
+@protocol delete // expected-error {{expected identifier; 'delete' is a keyword in Objective-C++}}
+@end
 
 @class Foo, try; // expected-error {{expected identifier; 'try' is a keyword in Objective-C++}}
 
Index: test/PCH/objc_exprs.h
===
--- test/PCH/objc_exprs.h
+++ test/PCH/objc_exprs.h
@@ -1,11 +1,13 @@
 
 @protocol foo;
+@protocol foo2
+@end
 @class itf;
 
 // Expressions
 typedef typeof(@"foo" "bar") objc_string;
 typedef typeof(@encode(int)) objc_encode;
-typedef typeof(@protocol(foo)) objc_protocol;
+typedef typeof(@protocol(foo2)) objc_protocol;
 typedef typeof(@selector(noArgs)) objc_selector_noArgs;
 typedef typeof(@selector(oneArg:)) objc_selector_oneArg;
 typedef typeof(@selector(foo:bar:)) objc_selector_twoArg;
Index: test/CodeGenObjC/protocols.m
===
--- test/CodeGenObjC/protocols.m
+++ test/CodeGenObjC/protocols.m
@@ -7,7 +7,8 @@
 -(int) conformsTo: (id) x;
 @end
 
-@protocol P0;
+@protocol P0
+@end
 
 @protocol P1
 +(void) classMethodReq0;
Index: test/CodeGenObjC/protocols-lazy.m
===
--- test/CodeGenObjC/protocols-lazy.m
+++ test/CodeGenObjC/protocols-lazy.m
@@ -18,7 +18,10 @@
 // RUN: grep OBJC_PROTOCOL_P3 %t | count 3
 // RUN: not grep OBJC_PROTOCOL_INSTANCE_METHODS_P3 %t
 @protocol P3;
-void f1() { id x = @protocol(P3); }
+@interface UserP3
+@end
+@implementation UserP3
+@end
 
 // Definition triggered by class reference.
 // RUN: grep OBJC_PROTOCOL_P4 %t | count 3
@@ -31,10 +34,16 @@
 // RUN: grep OBJC_PROTOCOL_P5 %t | count 3
 // RUN: grep OBJC_PROTOCOL_INSTANCE_METHODS_P5 %t | count 3
 @protocol P5;
-void f2() { id x = @protocol(P5); } // This generates a forward
-// reference, which has to be
-// updated on the next line.
-@protocol P5 -im1; @end   
+@interface UserP5 // This generates a forward
+  // reference, which has to be
+  // updated on the next line.
+@end
+@protocol P5 -im1; @end
+@implementation UserP5
+
+- im1 { }
+
+@end
 
 // Protocol reference following definition.
 // RUN: grep OBJC_PROTOCOL_P6 %t | count 4
Index: test/CodeGenObjC/protocol-comdat.m
==

[PATCH] D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression

2018-08-16 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

Sorry for the delay.

In https://reviews.llvm.org/D49462#1166032, @rjmccall wrote:

> Hmm.  I think this is a reasonable change to make to the language.  Have you 
> investigated to see if this causes source-compatibility problems?


Yes, I tested this change on internal code base. There's an impact, but the 
impact is fairly minimal. The biggest issue is actually in some Swift overlay: 
https://github.com/apple/swift/blob/c275826a41aca8268719061636efc12717b8dae1/stdlib/public/SDK/Dispatch/Dispatch.mm#L36




Comment at: test/CodeGenObjC/forward-declare-protocol-gnu.m:6
 
-Protocol *getProtocol(void)
-{
-   return @protocol(P);
-}
+@interface I 
+@end

rjmccall wrote:
> Does this really not require a definition of `P`?  Ugh.  I wonder if that's 
> reasonable to fix, too.
Nope, we don't emit the protocol metadata for it. It might make sense to 
require the definition with the implementation?



Comment at: test/Parser/objc-cxx-keyword-identifiers.mm:22
+@protocol P2;
+@protocol delete // expected-error {{expected identifier; 'delete' is a 
keyword in Objective-C++}}
+@end

rjmccall wrote:
> Why did this test need to change?
We need to declare `delete` because it's used in a `@protocol` expression on 
line 63.


Repository:
  rC Clang

https://reviews.llvm.org/D49462



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

[PATCH] D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression

2018-08-17 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: test/CodeGenObjC/forward-declare-protocol-gnu.m:6
 
-Protocol *getProtocol(void)
-{
-   return @protocol(P);
-}
+@interface I 
+@end

rjmccall wrote:
> arphaman wrote:
> > rjmccall wrote:
> > > Does this really not require a definition of `P`?  Ugh.  I wonder if 
> > > that's reasonable to fix, too.
> > Nope, we don't emit the protocol metadata for it. It might make sense to 
> > require the definition with the implementation?
> Yeah, I think so.  I would argue that there no places where we should be 
> emitting metadata for an incomplete protocol.
Ok, makes sense. 
I will fix it in a follow-up patch then. I don't want to block this change as 
this patch fixes some nasty runtime issues. I will run a separate source 
compatibility assessment for the follow-up.


Repository:
  rC Clang

https://reviews.llvm.org/D49462



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

[PATCH] D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression

2018-08-17 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: lib/CodeGen/CGObjCMac.cpp:6788
+ "emitting protocol metadata without definition");
+  PD = PD->getDefinition();
 

rjmccall wrote:
> What happens in the `@implementation` case (the one that we're not diagnosing 
> yet) when the protocol is a forward declaration?
We emit an `external global` reference to the protocol metadata using 
`GetOrEmitProtocolRef`, so this assertion won't be triggered until we force the 
emission of protocol metadata from implementation as planned in a follow-up 
patch.


Repository:
  rC Clang

https://reviews.llvm.org/D49462



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

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

2018-08-17 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

Hmm, after more analysis I realized that this is not the right solution for the 
rename problem. It would be correct to map one file:line:column location to a 
set of `SourceLocation`s, and to use the old `isPointWithin` on a set of such 
locations. I opened https://reviews.llvm.org/D50926 instead. Sorry for the 
trouble!


Repository:
  rC Clang

https://reviews.llvm.org/D50740



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

[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs

2018-08-17 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.
arphaman added reviewers: jkorous, ioeric.
Herald added a subscriber: dexonsmith.

This patch extracts the code that searches for a file id from `translateFile` 
to `findFileIDsForFile` to allow the users to map from one file entry to 
multiple FileIDs.
Will be used as a basis for a clang-rename fix in 
https://reviews.llvm.org/D50801.


Repository:
  rC Clang

https://reviews.llvm.org/D50926

Files:
  include/clang/Basic/SourceManager.h
  lib/Basic/SourceManager.cpp
  unittests/Basic/SourceManagerTest.cpp

Index: unittests/Basic/SourceManagerTest.cpp
===
--- unittests/Basic/SourceManagerTest.cpp
+++ unittests/Basic/SourceManagerTest.cpp
@@ -377,6 +377,55 @@
   EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
 }
 
+TEST_F(SourceManagerTest, findFileIDsForFile) {
+  const char *header = "int x;";
+
+  const char *main = "#include \n"
+ "#include \n";
+
+  std::unique_ptr HeaderBuf =
+  llvm::MemoryBuffer::getMemBuffer(header);
+  std::unique_ptr MainBuf =
+  llvm::MemoryBuffer::getMemBuffer(main);
+  SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(MainBuf)));
+
+  const FileEntry *headerFile =
+  FileMgr.getVirtualFile("/test-header.h", HeaderBuf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(headerFile, std::move(HeaderBuf));
+
+  TrivialModuleLoader ModLoader;
+  MemoryBufferCache PCMCache;
+  HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+  Diags, LangOpts, &*Target);
+  Preprocessor PP(std::make_shared(), Diags, LangOpts,
+  SourceMgr, PCMCache, HeaderInfo, ModLoader,
+  /*IILookup =*/nullptr,
+  /*OwnsHeaderSearch =*/false);
+  PP.Initialize(*Target);
+
+  PP.EnterMainSourceFile();
+
+  while (1) {
+Token tok;
+PP.Lex(tok);
+if (tok.is(tok::eof))
+  break;
+  }
+
+  llvm::SmallVector Files;
+  SourceMgr.findFileIDsForFile(headerFile, [&](FileID F) -> bool {
+Files.push_back(F);
+return false;
+  });
+
+  ASSERT_EQ(2U, Files.size());
+  ASSERT_NE(Files[0], Files[1]);
+  SourceLocation Loc1 = SourceMgr.translateLineCol(Files[0], 1, 1);
+  SourceLocation Loc2 = SourceMgr.translateLineCol(Files[1], 1, 1);
+  ASSERT_NE(Loc1, Loc2);
+  ASSERT_TRUE(SourceMgr.isBeforeInTranslationUnit(Loc1, Loc2));
+}
+
 #endif
 
 } // anonymous namespace
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1602,6 +1602,37 @@
   return translateLineCol(FirstFID, Line, Col);
 }
 
+bool SourceManager::findFileIDsForFile(
+const FileEntry *SourceFile,
+llvm::function_ref Callback) const {
+  assert(SourceFile && "Null source file!");
+
+  // Look through all of the local source locations.
+  for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
+bool Invalid = false;
+const SLocEntry &SLoc = getLocalSLocEntry(I, &Invalid);
+if (Invalid)
+  return false;
+
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+  if (Callback(FileID::get(I)))
+return true;
+}
+  }
+
+  // If that still didn't help, try the modules.
+  for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
+const SLocEntry &SLoc = getLoadedSLocEntry(I);
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+  if (Callback(FileID::get(-int(I) - 2)))
+return true;
+}
+  }
+  return false;
+}
+
 /// Get the FileID for the given file.
 ///
 /// If the source file is included multiple times, the FileID will be the
@@ -1650,35 +1681,16 @@
 }
   }
 
-  if (FirstFID.isInvalid()) {
-// The location we're looking for isn't in the main file; look
-// through all of the local source locations.
-for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
-  bool Invalid = false;
-  const SLocEntry &SLoc = getLocalSLocEntry(I, &Invalid);
-  if (Invalid)
-return FileID();
+  if (FirstFID.isValid())
+return FirstFID;
 
-  if (SLoc.isFile() &&
-  SLoc.getFile().getContentCache() &&
-  SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
-FirstFID = FileID::get(I);
-break;
-  }
-}
-// If that still didn't help, try the modules.
-if (FirstFID.isInvalid()) {
-  for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
-const SLocEntry &SLoc = getLoadedSLocEntry(I);
-if (SLoc.isFile() &&
-SLoc.getFile().getContentCache() &&
-SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
-  FirstFID = FileID::get(-int(I) - 2);
-  break;
-}
-  }
-}
-  }
+  // The location we're

[PATCH] D50814: [clangd] transfer the fixits with the notes instead of adding them to the main diagnostic if request by the client

2018-08-17 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

Our client has a presentation layer with a Clang-based hierarchical model for 
diagnostics, so we need the original notes. This is a hard requirement in that 
sense.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50814



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

[PATCH] D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression

2018-08-17 Thread Alex Lorenz via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340102: [ObjC] Error out when using forward-declared 
protocol in a @protocol (authored by arphaman, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49462?vs=161142&id=161347#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49462

Files:
  cfe/trunk/include/clang/Basic/DiagnosticGroups.td
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/CodeGen/CGObjCMac.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/lib/Sema/SemaExprObjC.cpp
  cfe/trunk/test/CodeGenObjC/forward-declare-protocol-gnu.m
  cfe/trunk/test/CodeGenObjC/forward-protocol-metadata-symbols.m
  cfe/trunk/test/CodeGenObjC/hidden-visibility.m
  cfe/trunk/test/CodeGenObjC/link-errors.m
  cfe/trunk/test/CodeGenObjC/protocol-comdat.m
  cfe/trunk/test/CodeGenObjC/protocols-lazy.m
  cfe/trunk/test/CodeGenObjC/protocols.m
  cfe/trunk/test/PCH/objc_exprs.h
  cfe/trunk/test/Parser/objc-cxx-keyword-identifiers.mm
  cfe/trunk/test/SemaObjC/protocol-expr-neg-1.m

Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -867,8 +867,8 @@
   "protocol has circular dependency">;
 def err_undeclared_protocol : Error<"cannot find protocol declaration for %0">;
 def warn_undef_protocolref : Warning<"cannot find protocol definition for %0">;
-def warn_atprotocol_protocol : Warning<
-  "@protocol is using a forward protocol declaration of %0">, InGroup;
+def err_atprotocol_protocol : Error<
+  "@protocol is using a forward protocol declaration of %0">;
 def warn_readonly_property : Warning<
   "attribute 'readonly' of property %0 restricts attribute "
   "'readwrite' of property inherited from %1">,
Index: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td
@@ -620,7 +620,8 @@
 def SelectorTypeMismatch : DiagGroup<"selector-type-mismatch">;
 def Selector : DiagGroup<"selector", [SelectorTypeMismatch]>;
 def Protocol : DiagGroup<"protocol">;
-def AtProtocol : DiagGroup<"at-protocol">;
+// No longer in use, preserve for backwards compatibility.
+def : DiagGroup<"at-protocol">;
 def PropertyAccessDotSyntax: DiagGroup<"property-access-dot-syntax">;
 def PropertyAttr : DiagGroup<"property-attribute-mismatch">;
 def SuperSubClassMismatch : DiagGroup<"super-class-method-mismatch">;
Index: cfe/trunk/test/Parser/objc-cxx-keyword-identifiers.mm
===
--- cfe/trunk/test/Parser/objc-cxx-keyword-identifiers.mm
+++ cfe/trunk/test/Parser/objc-cxx-keyword-identifiers.mm
@@ -18,7 +18,9 @@
 @protocol new // expected-error {{expected identifier; 'new' is a keyword in Objective-C++}}
 @end
 
-@protocol P2, delete; // expected-error {{expected identifier; 'delete' is a keyword in Objective-C++}}
+@protocol P2;
+@protocol delete // expected-error {{expected identifier; 'delete' is a keyword in Objective-C++}}
+@end
 
 @class Foo, try; // expected-error {{expected identifier; 'try' is a keyword in Objective-C++}}
 
Index: cfe/trunk/test/SemaObjC/protocol-expr-neg-1.m
===
--- cfe/trunk/test/SemaObjC/protocol-expr-neg-1.m
+++ cfe/trunk/test/SemaObjC/protocol-expr-neg-1.m
@@ -12,7 +12,7 @@
 int main()
 {
 	Protocol *proto = @protocol(p1);
-Protocol *fproto = @protocol(fproto); // expected-warning {{@protocol is using a forward protocol declaration of 'fproto'}}
+Protocol *fproto = @protocol(fproto); // expected-error {{@protocol is using a forward protocol declaration of 'fproto'}}
 	Protocol *pp = @protocol(i); // expected-error {{cannot find protocol declaration for 'i'}}
 	Protocol *p1p = @protocol(cl); // expected-error {{cannot find protocol declaration for 'cl'}}
 }
@@ -26,9 +26,9 @@
 @end
 
 int doesConform(id foo) {
-  return [foo conformsToProtocol:@protocol(TestProtocol)]; // expected-warning {{@protocol is using a forward protocol declaration of 'TestProtocol'}}
+  return [foo conformsToProtocol:@protocol(TestProtocol)]; // expected-error {{@protocol is using a forward protocol declaration of 'TestProtocol'}}
 }
 
 int doesConformSuper(id foo) {
-  return [foo conformsToProtocol:@protocol(SuperProtocol)]; // expected-warning {{@protocol is using a forward protocol declaration of 'SuperProtocol'}}
+  return [foo conformsToProtocol:@protocol(SuperProtocol)]; // expected-error {{@protocol is using a forward protocol declaration of 'SuperProtocol'}}
 }
Index: cfe/trunk/test/PCH/objc_exprs.h
===
--- cfe/trunk/test/PCH/objc_exprs.h
+++ cfe/trunk/test/PC

[PATCH] D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category

2018-08-20 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

In https://reviews.llvm.org/D50571#1205650, @joaotavora wrote:

> > LGTM. Let's watch out for possible breakages in any of the clients, though. 
> > I've checked VSCode and it seems to be fine with the added fields.
>
> This isn't in the spec and broke the LSP client eglot 
> (https://github.com/joaotavora/eglot/pull/81). Why don't you put this in the 
> "source" field, or concat it to the "message" field.  Who can even use this 
> information if it's not in the spec? Are clients supposed to code against 
> every LSP server's whim?


Thanks for the feedback. I'll make a patch that turns this off by default so 
that clients can opt-in into it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50571



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

[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs

2018-08-20 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: include/clang/Basic/SourceManager.h:1539
+  /// \returns true if the callback returned true, false otherwise.
+  bool findFileIDsForFile(const FileEntry *SourceFile,
+  llvm::function_ref Callback) const;

ioeric wrote:
> Callback pattern seems uncommon in LLVM/Clang. I'd suggest making this return 
> a set of `FileID`s and put the callback-based function as a helper (shared by 
> this and `translateFile`)in the implementation.
I created a helper, but unfortunately it needs to be a member of SourceManager 
as FileID::get is private.


Repository:
  rC Clang

https://reviews.llvm.org/D50926



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

[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs

2018-08-20 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 161504.
arphaman marked 2 inline comments as done.
arphaman added a comment.

Address review comments.


Repository:
  rC Clang

https://reviews.llvm.org/D50926

Files:
  include/clang/Basic/SourceManager.h
  lib/Basic/SourceManager.cpp
  unittests/Basic/SourceManagerTest.cpp

Index: unittests/Basic/SourceManagerTest.cpp
===
--- unittests/Basic/SourceManagerTest.cpp
+++ unittests/Basic/SourceManagerTest.cpp
@@ -377,6 +377,51 @@
   EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
 }
 
+TEST_F(SourceManagerTest, findFileIDsForFile) {
+  const char *header = "int x;";
+
+  const char *main = "#include \n"
+ "#include \n";
+
+  std::unique_ptr HeaderBuf =
+  llvm::MemoryBuffer::getMemBuffer(header);
+  std::unique_ptr MainBuf =
+  llvm::MemoryBuffer::getMemBuffer(main);
+  SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(MainBuf)));
+
+  const FileEntry *headerFile =
+  FileMgr.getVirtualFile("/test-header.h", HeaderBuf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(headerFile, std::move(HeaderBuf));
+
+  TrivialModuleLoader ModLoader;
+  MemoryBufferCache PCMCache;
+  HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+  Diags, LangOpts, &*Target);
+  Preprocessor PP(std::make_shared(), Diags, LangOpts,
+  SourceMgr, PCMCache, HeaderInfo, ModLoader,
+  /*IILookup =*/nullptr,
+  /*OwnsHeaderSearch =*/false);
+  PP.Initialize(*Target);
+
+  PP.EnterMainSourceFile();
+
+  while (1) {
+Token tok;
+PP.Lex(tok);
+if (tok.is(tok::eof))
+  break;
+  }
+
+  llvm::SmallVector Files = SourceMgr.findFileIDsForFile(headerFile);
+
+  ASSERT_EQ(2U, Files.size());
+  ASSERT_NE(Files[0], Files[1]);
+  SourceLocation Loc1 = SourceMgr.translateLineCol(Files[0], 1, 1);
+  SourceLocation Loc2 = SourceMgr.translateLineCol(Files[1], 1, 1);
+  ASSERT_NE(Loc1, Loc2);
+  ASSERT_TRUE(SourceMgr.isBeforeInTranslationUnit(Loc1, Loc2));
+}
+
 #endif
 
 } // anonymous namespace
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1602,6 +1602,47 @@
   return translateLineCol(FirstFID, Line, Col);
 }
 
+bool SourceManager::findFileIDsForFile(
+const FileEntry *SourceFile,
+llvm::function_ref Callback) const {
+  assert(SourceFile && "Null source file!");
+
+  // Look through all of the local source locations.
+  for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
+bool Invalid = false;
+const SLocEntry &SLoc = getLocalSLocEntry(I, &Invalid);
+if (Invalid)
+  return false;
+
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+  if (Callback(FileID::get(I)))
+return true;
+}
+  }
+
+  // If that still didn't help, try the modules.
+  for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
+const SLocEntry &SLoc = getLoadedSLocEntry(I);
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+  if (Callback(FileID::get(-int(I) - 2)))
+return true;
+}
+  }
+  return false;
+}
+
+llvm::SmallVector
+SourceManager::findFileIDsForFile(const FileEntry *SourceFile) const {
+  llvm::SmallVector Result;
+  findFileIDsForFile(SourceFile, [&](FileID F) {
+Result.push_back(F);
+return false;
+  });
+  return Result;
+}
+
 /// Get the FileID for the given file.
 ///
 /// If the source file is included multiple times, the FileID will be the
@@ -1650,35 +1691,16 @@
 }
   }
 
-  if (FirstFID.isInvalid()) {
-// The location we're looking for isn't in the main file; look
-// through all of the local source locations.
-for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
-  bool Invalid = false;
-  const SLocEntry &SLoc = getLocalSLocEntry(I, &Invalid);
-  if (Invalid)
-return FileID();
+  if (FirstFID.isValid())
+return FirstFID;
 
-  if (SLoc.isFile() &&
-  SLoc.getFile().getContentCache() &&
-  SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
-FirstFID = FileID::get(I);
-break;
-  }
-}
-// If that still didn't help, try the modules.
-if (FirstFID.isInvalid()) {
-  for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
-const SLocEntry &SLoc = getLoadedSLocEntry(I);
-if (SLoc.isFile() &&
-SLoc.getFile().getContentCache() &&
-SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
-  FirstFID = FileID::get(-int(I) - 2);
-  break;
-}
-  }
-}
-  }
+  // The location we're looking for isn't in the main file; look
+  // through all

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

2018-08-21 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: clangd/Threading.cpp:76
+
+  if (::pthread_attr_setstacksize(&Attr, 8 * 1024 * 1024) != 0)
+return;

please use clang::DesiredStackSize instead.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50993



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

[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs

2018-08-21 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 161847.
arphaman marked an inline comment as done.
arphaman added a comment.

Address review comments


Repository:
  rC Clang

https://reviews.llvm.org/D50926

Files:
  include/clang/Basic/SourceManager.h
  lib/Basic/SourceManager.cpp
  unittests/Basic/SourceManagerTest.cpp

Index: unittests/Basic/SourceManagerTest.cpp
===
--- unittests/Basic/SourceManagerTest.cpp
+++ unittests/Basic/SourceManagerTest.cpp
@@ -377,6 +377,60 @@
   EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
 }
 
+TEST_F(SourceManagerTest, findFileIDsForFile) {
+  const char *header = "int x;";
+
+  const char *main = "#include \n"
+ "#include \n";
+
+  std::unique_ptr HeaderBuf =
+  llvm::MemoryBuffer::getMemBuffer(header);
+  std::unique_ptr MainBuf =
+  llvm::MemoryBuffer::getMemBuffer(main);
+
+  const FileEntry *HeaderFile =
+  FileMgr.getVirtualFile("/test-header.h", HeaderBuf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(HeaderFile, std::move(HeaderBuf));
+  const FileEntry *MainFile =
+  FileMgr.getVirtualFile("main.cpp", MainBuf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(MainFile, std::move(MainBuf));
+  SourceMgr.setMainFileID(
+  SourceMgr.createFileID(MainFile, SourceLocation(), SrcMgr::C_User));
+
+  TrivialModuleLoader ModLoader;
+  MemoryBufferCache PCMCache;
+  HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+  Diags, LangOpts, &*Target);
+  Preprocessor PP(std::make_shared(), Diags, LangOpts,
+  SourceMgr, PCMCache, HeaderInfo, ModLoader,
+  /*IILookup =*/nullptr,
+  /*OwnsHeaderSearch =*/false);
+  PP.Initialize(*Target);
+
+  PP.EnterMainSourceFile();
+
+  while (1) {
+Token tok;
+PP.Lex(tok);
+if (tok.is(tok::eof))
+  break;
+  }
+
+  llvm::SmallVector MainFileID =
+  SourceMgr.findFileIDsForFile(MainFile);
+  ASSERT_EQ(1U, MainFileID.size());
+  ASSERT_EQ(MainFileID[0], SourceMgr.getMainFileID());
+
+  llvm::SmallVector Files = SourceMgr.findFileIDsForFile(HeaderFile);
+
+  ASSERT_EQ(2U, Files.size());
+  ASSERT_NE(Files[0], Files[1]);
+  SourceLocation Loc1 = SourceMgr.translateLineCol(Files[0], 1, 1);
+  SourceLocation Loc2 = SourceMgr.translateLineCol(Files[1], 1, 1);
+  ASSERT_NE(Loc1, Loc2);
+  ASSERT_TRUE(SourceMgr.isBeforeInTranslationUnit(Loc1, Loc2));
+}
+
 #endif
 
 } // anonymous namespace
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1602,6 +1602,47 @@
   return translateLineCol(FirstFID, Line, Col);
 }
 
+bool SourceManager::findFileIDsForFile(
+const FileEntry *SourceFile,
+llvm::function_ref Callback) const {
+  assert(SourceFile && "Null source file!");
+
+  // Look through all of the local source locations.
+  for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
+bool Invalid = false;
+const SLocEntry &SLoc = getLocalSLocEntry(I, &Invalid);
+if (Invalid)
+  return false;
+
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+  if (Callback(FileID::get(I)))
+return true;
+}
+  }
+
+  // If that still didn't help, try the modules.
+  for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
+const SLocEntry &SLoc = getLoadedSLocEntry(I);
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+  if (Callback(FileID::get(-int(I) - 2)))
+return true;
+}
+  }
+  return false;
+}
+
+llvm::SmallVector
+SourceManager::findFileIDsForFile(const FileEntry *SourceFile) const {
+  llvm::SmallVector Result;
+  findFileIDsForFile(SourceFile, [&](FileID F) {
+Result.push_back(F);
+return false;
+  });
+  return Result;
+}
+
 /// Get the FileID for the given file.
 ///
 /// If the source file is included multiple times, the FileID will be the
@@ -1650,35 +1691,16 @@
 }
   }
 
-  if (FirstFID.isInvalid()) {
-// The location we're looking for isn't in the main file; look
-// through all of the local source locations.
-for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
-  bool Invalid = false;
-  const SLocEntry &SLoc = getLocalSLocEntry(I, &Invalid);
-  if (Invalid)
-return FileID();
+  if (FirstFID.isValid())
+return FirstFID;
 
-  if (SLoc.isFile() &&
-  SLoc.getFile().getContentCache() &&
-  SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
-FirstFID = FileID::get(I);
-break;
-  }
-}
-// If that still didn't help, try the modules.
-if (FirstFID.isInvalid()) {
-  for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
-const

[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs

2018-08-21 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: lib/Basic/SourceManager.cpp:1705
 
   // If we haven't found what we want yet, try again, but this time stat()
   // each of the files in case the files have changed since we originally

ioeric wrote:
> Do we also need this in `findFileIDsForFile`?
I don't really need this for my use case.


Repository:
  rC Clang

https://reviews.llvm.org/D50926



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

[PATCH] D50814: [clangd] transfer the fixits with the notes instead of adding them to the main diagnostic if request by the client

2018-08-21 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

In https://reviews.llvm.org/D50814#1207219, @ilya-biryukov wrote:

> Just to make sure I fully understand the use-case: could you elaborate a 
> little more? Do you need to get exactly the same set of notes that clang 
> provides?

Yep. We are replacing libclang in a client that currently consumes diagnostics 
as provided by clang. We'd like to avoid regressions in the client, so we are 
looking to match the diagnostics provided by clangd, at least for now.
It might be possible to recreate the original hierarchy using the diagnostics 
that are currently provided by clangd using some heroics on the client-side. 
However, we'd like to avoid them for now and get the data from Clangd directly 
:)

> Our current model seems to follow the clang's model, but it removes some 
> notes and shows them as fix-its for the main diagnostic instead.
>  (We think it should provide a better user experience, because those notes do 
> look like a way to present a fix-it to the user when you have only text-based 
> output and they seem redundant when one has an editor-based UI with fix-its).

The current Clangd model certainly works well for newer clients who are built 
with LSP in mind from the get go. Sending diagnostics in a format similar to 
Clang's originals diagnostics would be helpful to clients that are 
transitioning though :)

> Not opposed to changing the model to meet other needs, but want to make sure 
> what we're currently doing in clangd makes sense and understand your use-case 
> better.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50814

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

[PATCH] D51077: [clangd] send diagnostic categories only when 'categorySupport' capability was given by the client

2018-08-21 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.
arphaman added a reviewer: ilya-biryukov.
Herald added subscribers: kadircet, dexonsmith, jkorous, MaskRay, ioeric.

After https://reviews.llvm.org/D50571 Clangd started sending categories with 
each diagnostic, but that broke the eglot client. This patch puts the 
categories behind a capability to fix that breakage.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51077

Files:
  clangd/ClangdLSPServer.cpp
  clangd/Diagnostics.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/compile-commands-path.test
  test/clangd/diagnostic-category.test
  test/clangd/diagnostics.test
  test/clangd/did-change-configuration-params.test
  test/clangd/execute-command.test
  test/clangd/extra-flags.test
  test/clangd/fixits-embed-in-diagnostic.test
  test/clangd/fixits.test

Index: test/clangd/fixits.test
===
--- test/clangd/fixits.test
+++ test/clangd/fixits.test
@@ -6,7 +6,6 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/fixits-embed-in-diagnostic.test
===
--- test/clangd/fixits-embed-in-diagnostic.test
+++ test/clangd/fixits-embed-in-diagnostic.test
@@ -6,7 +6,6 @@
 # CHECK-NEXT:"params": {
 # CHECK-NEXT: "diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"clangd_fixes": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"edit": {
Index: test/clangd/extra-flags.test
===
--- test/clangd/extra-flags.test
+++ test/clangd/extra-flags.test
@@ -6,7 +6,6 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
@@ -29,7 +28,6 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/execute-command.test
===
--- test/clangd/execute-command.test
+++ test/clangd/execute-command.test
@@ -6,7 +6,6 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/did-change-configuration-params.test
===
--- test/clangd/did-change-configuration-params.test
+++ test/clangd/did-change-configuration-params.test
@@ -24,7 +24,6 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/diagnostics.test
===
--- test/clangd/diagnostics.test
+++ test/clangd/diagnostics.test
@@ -6,7 +6,6 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Return type of 'main' is not 'int'",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/diagnostic-category.test
===
--- /dev/null
+++ test/clangd/diagnostic-category.test
@@ -0,0 +1,43 @@
+# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"textDocument":{"publishDiagnostics":{"categorySupport":true}}},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"struct Point {}; union Point p;"}}}
+#  CHECK:"method": "textDocument/publishDiagnostics",
+# CHECK-NEXT:"params": {
+# CHECK-NEXT: "diagnostics": [
+# CHECK-NEXT:  {
+# CHECK-NEXT:"category": "Semantic Issue",
+# CHECK-NEXT:"message": "Use of 'Point' with tag type that does not mat

[PATCH] D51077: [clangd] send diagnostic categories only when 'categorySupport' capability was given by the client

2018-08-22 Thread Alex Lorenz via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL340449: [clangd] send diagnostic categories only when 
'categorySupport' (authored by arphaman, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D51077?vs=161860&id=162042#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D51077

Files:
  clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
  clang-tools-extra/trunk/clangd/Diagnostics.h
  clang-tools-extra/trunk/clangd/Protocol.cpp
  clang-tools-extra/trunk/clangd/Protocol.h
  clang-tools-extra/trunk/test/clangd/compile-commands-path-in-initialize.test
  clang-tools-extra/trunk/test/clangd/compile-commands-path.test
  clang-tools-extra/trunk/test/clangd/diagnostic-category.test
  clang-tools-extra/trunk/test/clangd/diagnostics.test
  clang-tools-extra/trunk/test/clangd/did-change-configuration-params.test
  clang-tools-extra/trunk/test/clangd/execute-command.test
  clang-tools-extra/trunk/test/clangd/extra-flags.test
  clang-tools-extra/trunk/test/clangd/fixits-embed-in-diagnostic.test
  clang-tools-extra/trunk/test/clangd/fixits.test

Index: clang-tools-extra/trunk/clangd/Protocol.h
===
--- clang-tools-extra/trunk/clangd/Protocol.h
+++ clang-tools-extra/trunk/clangd/Protocol.h
@@ -255,6 +255,10 @@
   /// Whether the client accepts diagnostics with fixes attached using the
   /// "clangd_fixes" extension.
   bool clangdFixSupport = false;
+
+  /// Whether the client accepts diagnostics with category attached to it
+  /// using the "category" extension.
+  bool categorySupport = false;
 };
 bool fromJSON(const llvm::json::Value &,
   PublishDiagnosticsClientCapabilities &);
Index: clang-tools-extra/trunk/clangd/Diagnostics.h
===
--- clang-tools-extra/trunk/clangd/Diagnostics.h
+++ clang-tools-extra/trunk/clangd/Diagnostics.h
@@ -27,6 +27,12 @@
   /// If true, Clangd uses an LSP extension to embed the fixes with the
   /// diagnostics that are sent to the client.
   bool EmbedFixesInDiagnostics = false;
+
+  /// If true, Clangd uses an LSP extension to send the diagnostic's
+  /// category to the client. The category typically describes the compilation
+  /// stage during which the issue was produced, e.g. "Semantic Issue" or "Parse
+  /// Issue".
+  bool SendDiagnosticCategory = false;
 };
 
 /// Contains basic information about a diagnostic.
Index: clang-tools-extra/trunk/clangd/Protocol.cpp
===
--- clang-tools-extra/trunk/clangd/Protocol.cpp
+++ clang-tools-extra/trunk/clangd/Protocol.cpp
@@ -184,6 +184,7 @@
   if (!O)
 return false;
   O.map("clangdFixSupport", R.clangdFixSupport);
+  O.map("categorySupport", R.categorySupport);
   return true;
 }
 
Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
@@ -84,6 +84,8 @@
   Params.capabilities.textDocument.completion.completionItem.snippetSupport;
   DiagOpts.EmbedFixesInDiagnostics =
   Params.capabilities.textDocument.publishDiagnostics.clangdFixSupport;
+  DiagOpts.SendDiagnosticCategory =
+  Params.capabilities.textDocument.publishDiagnostics.categorySupport;
 
   if (Params.capabilities.workspace && Params.capabilities.workspace->symbol &&
   Params.capabilities.workspace->symbol->symbolKind) {
@@ -506,7 +508,7 @@
 }
 LSPDiag["clangd_fixes"] = std::move(ClangdFixes);
   }
-  if (!Diag.category.empty())
+  if (DiagOpts.SendDiagnosticCategory && !Diag.category.empty())
 LSPDiag["category"] = Diag.category;
   DiagnosticsJSON.push_back(std::move(LSPDiag));
 
Index: clang-tools-extra/trunk/test/clangd/did-change-configuration-params.test
===
--- clang-tools-extra/trunk/test/clangd/did-change-configuration-params.test
+++ clang-tools-extra/trunk/test/clangd/did-change-configuration-params.test
@@ -24,7 +24,6 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Variable 'i' is uninitialized when used here",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: clang-tools-extra/trunk/test/clangd/fixits.test
===
--- clang-tools-extra/trunk/test/clangd/fixits.test
+++ clang-tools-extra/trunk/test/clangd/fixits.test
@@ -6,7 +6,6 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"category": "Semantic Issue",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without p

[PATCH] D50571: [clangd] add an extension field to LSP to transfer the diagnostic's category

2018-08-22 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

In https://reviews.llvm.org/D50571#1208635, @joaotavora wrote:

> In https://reviews.llvm.org/D50571#1206020, @arphaman wrote:
>
> > In https://reviews.llvm.org/D50571#1205650, @joaotavora wrote:
> >
> > > > LGTM. Let's watch out for possible breakages in any of the clients, 
> > > > though. I've checked VSCode and it seems to be fine with the added 
> > > > fields.
> > >
> > > This isn't in the spec and broke the LSP client eglot 
> > > (https://github.com/joaotavora/eglot/pull/81). Why don't you put this in 
> > > the "source" field, or concat it to the "message" field.  Who can even 
> > > use this information if it's not in the spec? Are clients supposed to 
> > > code against every LSP server's whim?
> >
> >
> > Thanks for the feedback. I'll make a patch that turns this off by default 
> > so that clients can opt-in into it.
>
>
> Thank you very much, and sorry if I came across a bit hostile.  What is this 
> category field good for?


NP! Fixed in r340449.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50571



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

[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs

2018-08-22 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 162106.
arphaman added a comment.

Address review comments.


https://reviews.llvm.org/D50926

Files:
  include/clang/Basic/SourceManager.h
  lib/Basic/SourceManager.cpp
  unittests/Basic/SourceManagerTest.cpp

Index: unittests/Basic/SourceManagerTest.cpp
===
--- unittests/Basic/SourceManagerTest.cpp
+++ unittests/Basic/SourceManagerTest.cpp
@@ -377,6 +377,60 @@
   EXPECT_TRUE(SourceMgr.isBeforeInTranslationUnit(Macros[10].Loc, Macros[11].Loc));
 }
 
+TEST_F(SourceManagerTest, findFileIDsForFile) {
+  const char *header = "int x;";
+
+  const char *main = "#include \n"
+ "#include \n";
+
+  std::unique_ptr HeaderBuf =
+  llvm::MemoryBuffer::getMemBuffer(header);
+  std::unique_ptr MainBuf =
+  llvm::MemoryBuffer::getMemBuffer(main);
+
+  const FileEntry *HeaderFile =
+  FileMgr.getVirtualFile("/test-header.h", HeaderBuf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(HeaderFile, std::move(HeaderBuf));
+  const FileEntry *MainFile =
+  FileMgr.getVirtualFile("main.cpp", MainBuf->getBufferSize(), 0);
+  SourceMgr.overrideFileContents(MainFile, std::move(MainBuf));
+  SourceMgr.setMainFileID(
+  SourceMgr.createFileID(MainFile, SourceLocation(), SrcMgr::C_User));
+
+  TrivialModuleLoader ModLoader;
+  MemoryBufferCache PCMCache;
+  HeaderSearch HeaderInfo(std::make_shared(), SourceMgr,
+  Diags, LangOpts, &*Target);
+  Preprocessor PP(std::make_shared(), Diags, LangOpts,
+  SourceMgr, PCMCache, HeaderInfo, ModLoader,
+  /*IILookup =*/nullptr,
+  /*OwnsHeaderSearch =*/false);
+  PP.Initialize(*Target);
+
+  PP.EnterMainSourceFile();
+
+  while (1) {
+Token tok;
+PP.Lex(tok);
+if (tok.is(tok::eof))
+  break;
+  }
+
+  llvm::SmallVector MainFileID =
+  SourceMgr.findFileIDsForFile(MainFile);
+  ASSERT_EQ(1U, MainFileID.size());
+  ASSERT_EQ(MainFileID[0], SourceMgr.getMainFileID());
+
+  llvm::SmallVector Files = SourceMgr.findFileIDsForFile(HeaderFile);
+
+  ASSERT_EQ(2U, Files.size());
+  ASSERT_NE(Files[0], Files[1]);
+  SourceLocation Loc1 = SourceMgr.translateLineCol(Files[0], 1, 1);
+  SourceLocation Loc2 = SourceMgr.translateLineCol(Files[1], 1, 1);
+  ASSERT_NE(Loc1, Loc2);
+  ASSERT_TRUE(SourceMgr.isBeforeInTranslationUnit(Loc1, Loc2));
+}
+
 #endif
 
 } // anonymous namespace
Index: lib/Basic/SourceManager.cpp
===
--- lib/Basic/SourceManager.cpp
+++ lib/Basic/SourceManager.cpp
@@ -1602,6 +1602,70 @@
   return translateLineCol(FirstFID, Line, Col);
 }
 
+bool SourceManager::findFileIDsForFile(
+const FileEntry *SourceFile, bool LookForFilesystemUpdates,
+llvm::function_ref Callback) const {
+  assert(SourceFile && "Null source file!");
+
+  Optional SourceFileUID;
+  Optional SourceFileName;
+
+  // Look through all of the local source locations.
+  for (unsigned I = 0, N = local_sloc_entry_size(); I != N; ++I) {
+bool Invalid = false;
+const SLocEntry &SLoc = getLocalSLocEntry(I, &Invalid);
+if (Invalid)
+  return false;
+if (!SLoc.isFile())
+  continue;
+const ContentCache *FileContentCache = SLoc.getFile().getContentCache();
+if (!FileContentCache || !FileContentCache->OrigEntry)
+  continue;
+
+if (FileContentCache->OrigEntry == SourceFile) {
+  if (Callback(FileID::get(I)))
+return true;
+} else if (LookForFilesystemUpdates &&
+   (SourceFileName || (SourceFileName = llvm::sys::path::filename(
+   SourceFile->getName( &&
+   *SourceFileName == llvm::sys::path::filename(
+  FileContentCache->OrigEntry->getName()) &&
+   (SourceFileUID ||
+(SourceFileUID = getActualFileUID(SourceFile {
+  if (Optional EntryUID =
+  getActualFileUID(FileContentCache->OrigEntry)) {
+if (*SourceFileUID == *EntryUID) {
+  if (Callback(FileID::get(I)))
+return true;
+  SourceFile = FileContentCache->OrigEntry;
+}
+  }
+}
+  }
+
+  // If that still didn't help, try the modules.
+  for (unsigned I = 0, N = loaded_sloc_entry_size(); I != N; ++I) {
+const SLocEntry &SLoc = getLoadedSLocEntry(I);
+if (SLoc.isFile() && SLoc.getFile().getContentCache() &&
+SLoc.getFile().getContentCache()->OrigEntry == SourceFile) {
+  if (Callback(FileID::get(-int(I) - 2)))
+return true;
+}
+  }
+  return false;
+}
+
+llvm::SmallVector
+SourceManager::findFileIDsForFile(const FileEntry *SourceFile) const {
+  llvm::SmallVector Result;
+  findFileIDsForFile(SourceFile, /*LookForFilesystemUpdates=*/true,
+ [&](FileID F) {
+   Result.push_back(F);
+   return false;
+ });
+  ret

[PATCH] D50926: [SourceManager] Extract 'findFileIDsForFile' from 'translateFile' to allow mapping from one file entry to multiple FileIDs

2018-08-22 Thread Alex Lorenz via Phabricator via cfe-commits

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



Comment at: lib/Basic/SourceManager.cpp:1705
 
   // If we haven't found what we want yet, try again, but this time stat()
   // each of the files in case the files have changed since we originally

ioeric wrote:
> arphaman wrote:
> > ioeric wrote:
> > > Do we also need this in `findFileIDsForFile`?
> > I don't really need this for my use case.
> But it's not clear from the interface AFAICT. We should either handle this 
> case (maybe controlled with a flag), or make it clear in the API (with a 
> different name or documentation).
Hmm, I think that would be better. I pulled that code into the 
`findFileIDsForFile` helper function, so we can call it in two modes now. It's 
probably good to do that check just in case in the new API too, so it does that 
check as well.


https://reviews.llvm.org/D50926



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

[PATCH] D47280: [Sema][ObjC] Do not DiagnoseUseOfDecl in LookupMemberExpr

2018-05-23 Thread Alex Lorenz via Phabricator via cfe-commits

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

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D47280



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

[PATCH] D47229: Make atomic non-member functions as nonnull

2018-05-23 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:3497
+else if (Form == Copy || Form == Xchg) {
+  if (!IsC11 && !IsN)
+// The value pointer is always dereferenced, a nullptr is 
undefined.

Nit: might make more sense to check if `ByValType` is a pointer here instead of 
duplicating the `if` condition from above.


Repository:
  rC Clang

https://reviews.llvm.org/D47229



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

[PATCH] D47341: [Sema] Disable creating new delayed typos while correcting existing.

2018-05-24 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: clang/test/SemaCXX/typo-correction-delayed.cpp:146
+  // expected-error@+1 {{use of undeclared identifier 'variableX'}}
   int x = variableX.getX();
 }

Loosing typo correction for 'getX' is fine, however, I think we still want to 
typo-correct 'variableX' to 'variable' here.


https://reviews.llvm.org/D47341



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

[PATCH] D46918: [Coverage] Discard the last uncompleted deferred region in a decl

2018-05-25 Thread Alex Lorenz via Phabricator via cfe-commits

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

LGTM


https://reviews.llvm.org/D46918



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

[PATCH] D47225: Add nonnull; use it for atomics

2018-05-25 Thread Alex Lorenz via Phabricator via cfe-commits

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

LGTM

(FYI, Please use a weekly frequency for Pings).


Repository:
  rCXX libc++

https://reviews.llvm.org/D47225



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

[PATCH] D37856: [refactor] add support for refactoring options

2017-09-29 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 117124.
arphaman marked 10 inline comments as done.
arphaman added a comment.

Address review comments


Repository:
  rL LLVM

https://reviews.llvm.org/D37856

Files:
  include/clang/Tooling/Refactoring/RefactoringActionRule.h
  include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
  include/clang/Tooling/Refactoring/RefactoringOption.h
  include/clang/Tooling/Refactoring/RefactoringOptionVisitor.h
  include/clang/Tooling/Refactoring/RefactoringOptions.h
  include/clang/Tooling/Refactoring/Rename/RenamingAction.h
  lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  test/Refactor/LocalRename/Field.cpp
  test/Refactor/tool-test-support.c
  tools/clang-refactor/ClangRefactor.cpp

Index: tools/clang-refactor/ClangRefactor.cpp
===
--- tools/clang-refactor/ClangRefactor.cpp
+++ tools/clang-refactor/ClangRefactor.cpp
@@ -18,6 +18,7 @@
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/Refactoring/RefactoringAction.h"
+#include "clang/Tooling/Refactoring/RefactoringOptions.h"
 #include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/CommandLine.h"
@@ -32,10 +33,10 @@
 
 namespace opts {
 
-static cl::OptionCategory CommonRefactorOptions("Common refactoring options");
+static cl::OptionCategory CommonRefactorOptions("Refactoring options");
 
 static cl::opt Verbose("v", cl::desc("Use verbose output"),
- cl::cat(CommonRefactorOptions),
+ cl::cat(cl::GeneralCategory),
  cl::sub(*cl::AllSubCommands));
 } // end namespace opts
 
@@ -116,6 +117,98 @@
   return nullptr;
 }
 
+/// A container that stores the command-line options used by a single
+/// refactoring option.
+class RefactoringActionCommandLineOptions {
+public:
+  template 
+  void addOption(const RefactoringOption &Option,
+ std::unique_ptr> CLOption);
+
+  const cl::opt &
+  getStringOption(const RefactoringOption &Opt) const {
+auto It = StringOptions.find(&Opt);
+return *It->second;
+  }
+
+private:
+  llvm::DenseMap>>
+  StringOptions;
+};
+
+template <>
+void RefactoringActionCommandLineOptions::addOption(
+const RefactoringOption &Option,
+std::unique_ptr> CLOption) {
+  StringOptions[&Option] = std::move(CLOption);
+}
+
+/// Passes the command-line option values to the options used by a single
+/// refactoring action rule.
+class CommandLineRefactoringOptionConsumer final
+: public RefactoringOptionVisitor {
+public:
+  CommandLineRefactoringOptionConsumer(
+  const RefactoringActionCommandLineOptions &Options)
+  : Options(Options) {}
+
+  void visit(const RefactoringOption &Opt,
+ Optional &Value) override {
+const cl::opt &CLOpt = Options.getStringOption(Opt);
+if (!CLOpt.getValue().empty()) {
+  Value = CLOpt.getValue();
+  return;
+}
+Value = None;
+if (Opt.isRequired())
+  MissingRequiredOptions.push_back(&Opt);
+  }
+
+  ArrayRef getMissingRequiredOptions() const {
+return MissingRequiredOptions;
+  }
+
+private:
+  llvm::SmallVector MissingRequiredOptions;
+  const RefactoringActionCommandLineOptions &Options;
+};
+
+/// Creates the refactoring options used by all the rules in a single
+/// refactoring action.
+class CommandLineRefactoringOptionCreator final
+: public RefactoringOptionVisitor {
+public:
+  CommandLineRefactoringOptionCreator(
+  cl::OptionCategory &Category, cl::SubCommand &Subcommand,
+  RefactoringActionCommandLineOptions &Options)
+  : Category(Category), Subcommand(Subcommand), Options(Options) {}
+
+  void visit(const RefactoringOption &Opt, Optional &) override {
+if (Visited.insert(&Opt).second)
+  Options.addOption(Opt, create(Opt));
+  }
+
+private:
+  template 
+  std::unique_ptr> create(const RefactoringOption &Opt) {
+if (!OptionNames.insert(Opt.getName()).second)
+  llvm::report_fatal_error("Multiple identical refactoring options "
+   "specified for one refactoring action");
+// FIXME: cl::Required can be specified when this option is present
+// in all rules in an action.
+return llvm::make_unique>(
+Opt.getName(), cl::desc(Opt.getDescription()), cl::Optional,
+cl::cat(Category), cl::sub(Subcommand));
+  }
+
+  llvm::SmallPtrSet Visited;
+  llvm::StringSet<> OptionNames;
+  cl::OptionCategory &Category;
+  cl::SubCommand &Subcommand;
+  RefactoringActionCommandLineOptions &Options;
+};
+
 /// A subcommand that corresponds to individual refactoring action.
 class RefactoringActionSubcommand : public cl::SubCommand {
 public:
@@ -138,6 +231,12 @@
"::)"),
   cl::cat(Category), cl::sub(*this));
 }
+// Create

[PATCH] D37856: [refactor] add support for refactoring options

2017-09-29 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:78
+  std::vector>
+  getRefactoringOptions() const final override {
+return {Opt};

ioeric wrote:
> Why return a vector instead of a single value if there is only one element?
To support more complex requirements that need multiple options.



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:83
+  Expected().getValue())>
+  evaluate(RefactoringRuleContext &) const {
+return Opt->getValue();

ioeric wrote:
> This could use some comment, e.g. what is `getValue` expected to do?
I've simplified this by using a typealias in the option itself.



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:88
+private:
+  std::shared_ptr Opt;
+};

ioeric wrote:
> Please explain why `shared_ptr` is needed here.
The same option can be used by multiple rules, hence options can be owned by 
multiple requirements in  different rules at once. I documented this.



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:68
+/// Scans the tuple and returns a valid \c Error if any of the values are
+/// invalid.
+template 

ioeric wrote:
> Please elaborate on what's valid or invalid.
Oops, copy-pasted comment. I fixed it up.



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h:74
+  struct OptionGatherer {
+std::vector> &Options;
+

ioeric wrote:
> Why `shared_ptr`? Would `unique_ptr` work?
I refactored this to change the redundant ownership here.



Comment at: tools/clang-refactor/ClangRefactor.cpp:372
   if (Rule->hasSelectionRequirement()) {
 HasSelection = true;
+if (!Subcommand.getSelection()) {

ioeric wrote:
> Wondering if it is sensible to make `selection` also a general option so that 
> we don't need to special-case selections.
I think right now it's tricky because of the testing support. However, I think 
that it would be better to use a similar mechanism for selection option as 
well. I'll think about how we can reconcile the two approaches and will 
hopefully come up with a patch for it.


Repository:
  rL LLVM

https://reviews.llvm.org/D37856



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

[PATCH] D37976: [docs][refactor] add refactoring engine design documentation

2017-09-29 Thread Alex Lorenz via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
arphaman marked 2 inline comments as done.
Closed by commit rL314509: [docs][refactor] Add refactoring engine design 
documentation (authored by arphaman).

Changed prior to commit:
  https://reviews.llvm.org/D37976?vs=115638&id=117127#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D37976

Files:
  cfe/trunk/docs/RefactoringEngine.rst
  cfe/trunk/docs/index.rst

Index: cfe/trunk/docs/RefactoringEngine.rst
===
--- cfe/trunk/docs/RefactoringEngine.rst
+++ cfe/trunk/docs/RefactoringEngine.rst
@@ -0,0 +1,253 @@
+==
+Clang's refactoring engine
+==
+
+This document describes the design of Clang's refactoring engine and provides
+a couple of examples that show how various primitives in the refactoring API
+can be used to implement different refactoring actions. The :doc:`LibTooling`
+library provides several other APIs that are used when developing a
+refactoring action.
+
+Refactoring engine can be used to implement local refactorings that are
+initiated using a selection in an editor or an IDE. You can combine
+:doc:`AST matchers` and the refactoring engine to implement
+refactorings that don't lend themselves well to source selection and/or have to
+query ASTs for some particular nodes.
+
+We assume basic knowledge about the Clang AST. See the :doc:`Introduction
+to the Clang AST ` if you want to learn more
+about how the AST is structured.
+
+..  FIXME: create new refactoring action tutorial and link to the tutorial
+
+Introduction
+
+
+Clang's refactoring engine defines a set refactoring actions that implement
+a number of different source transformations. The ``clang-refactor``
+command-line tool can be used to perform these refactorings. Certain
+refactorings are also available in other clients like text editors and IDEs.
+
+A refactoring action is a class that defines a list of related refactoring
+operations (rules). These rules are grouped under a common umbrella - a single
+``clang-refactor`` command. In addition to rules, the refactoring action
+provides the action's command name and description to ``clang-refactor``.
+Each action must implement the ``RefactoringAction`` interface. Here's an
+outline of a ``local-rename`` action:
+
+.. code-block:: c++
+
+  class LocalRename final : public RefactoringAction {
+  public:
+StringRef getCommand() const override { return "local-rename"; }
+
+   StringRef getDescription() const override {
+  return "Finds and renames symbols in code with no indexer support";
+}
+
+RefactoringActionRules createActionRules() const override {
+  ...
+}
+  };
+
+Refactoring Action Rules
+
+
+An individual refactoring action is responsible for creating the set of
+grouped refactoring action rules that represent one refactoring operation.
+Although the rules in one action may have a number of different implementations,
+they should strive to produce a similar result. It should be easy for users to
+identify which refactoring action produced the result regardless of which
+refactoring action rule was used.
+
+The distinction between actions and rules enables the creation of actions
+that define a set of different rules that produce similar results. For example,
+the "add missing switch cases" refactoring operation typically adds missing
+cases to one switch at a time. However, it could be useful to have a
+refactoring that works on all switches that operate on a particular enum, as
+one could then automatically update all of them after adding a new enum
+constant. To achieve that, we can create two different rules that will use one
+``clang-refactor`` subcommand. The first rule will describe a local operation
+that's initiated when the user selects a single switch. The second rule will
+describe a global operation that works across translation units and is initiated
+when the user provides the name of the enum to clang-refactor (or the user could
+select the enum declaration instead). The clang-refactor tool will then analyze
+the selection and other options passed to the refactoring action, and will pick
+the most appropriate rule for the given selection and other options.
+
+Rule Types
+^^
+
+Clang's refactoring engine supports several different refactoring rules:
+
+- ``SourceChangeRefactoringRule`` produces source replacements that are applied
+  to the source files. Subclasses that choose to implement this rule have to
+  implement the ``createSourceReplacements`` member function. This type of
+  rule is typically used to implement local refactorings that transform the
+  source in one translation unit only.
+
+- ``FindSymbolOccurrencesRefactoringRule`` produces a "partial" refactoring
+  result: a set of occurrences that refer to a particular symbol. This type
+  of rule is typically used to implement an interactive re

[PATCH] D38402: [clang-refactor] Apply source replacements

2017-09-29 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.

This patch actually brings clang-refactor to a usable state as it can now apply 
the refactoring changes to the source files.
The `-selection` option is now also fully supported.


Repository:
  rL LLVM

https://reviews.llvm.org/D38402

Files:
  include/clang/Frontend/CommandLineSourceLoc.h
  test/Refactor/tool-apply-replacements.cpp
  test/Refactor/tool-selection-option.c
  tools/clang-refactor/ClangRefactor.cpp

Index: tools/clang-refactor/ClangRefactor.cpp
===
--- tools/clang-refactor/ClangRefactor.cpp
+++ tools/clang-refactor/ClangRefactor.cpp
@@ -14,6 +14,7 @@
 //===--===//
 
 #include "TestSupport.h"
+#include "clang/Frontend/CommandLineSourceLoc.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Refactoring.h"
@@ -54,7 +55,7 @@
 
   /// Prints any additional state associated with the selection argument to
   /// the given output stream.
-  virtual void print(raw_ostream &OS) = 0;
+  virtual void print(raw_ostream &OS) {}
 
   /// Returns a replacement refactoring result consumer (if any) that should
   /// consume the results of a refactoring operation.
@@ -99,6 +100,41 @@
   TestSelectionRangesInFile TestSelections;
 };
 
+/// Stores the parsed -selection=filename:line:column[-line:column] option.
+class SourceRangeSelectionArgument final : public SourceSelectionArgument {
+public:
+  SourceRangeSelectionArgument(ParsedSourceRange Range)
+  : Range(std::move(Range)) {}
+
+  bool forAllRanges(const SourceManager &SM,
+llvm::function_ref Callback) override {
+const FileEntry *FE = SM.getFileManager().getFile(Range.FileName);
+FileID FID = FE ? SM.translateFile(FE) : FileID();
+if (!FE || FID.isInvalid()) {
+  llvm::errs() << "error: -selection=" << Range.FileName
+   << ":... : given file is not in the target TU\n";
+  return true;
+}
+
+SourceLocation Start = SM.getMacroArgExpandedLocation(
+SM.translateLineCol(FID, Range.Begin.first, Range.Begin.second));
+SourceLocation End = SM.getMacroArgExpandedLocation(
+SM.translateLineCol(FID, Range.End.first, Range.End.second));
+if (Start.isInvalid() || End.isInvalid()) {
+  llvm::errs() << "error: -selection=" << Range.FileName << ':'
+   << Range.Begin.first << ':' << Range.Begin.second << '-'
+   << Range.End.first << ':' << Range.End.second
+   << " : invalid source location\n";
+  return true;
+}
+Callback(SourceRange(Start, End));
+return false;
+  }
+
+private:
+  ParsedSourceRange Range;
+};
+
 std::unique_ptr
 SourceSelectionArgument::fromString(StringRef Value) {
   if (Value.startswith("test:")) {
@@ -110,10 +146,12 @@
 return llvm::make_unique(
 std::move(*ParsedTestSelection));
   }
-  // FIXME: Support true selection ranges.
+  Optional Range = ParsedSourceRange::fromString(Value);
+  if (Range)
+return llvm::make_unique(std::move(*Range));
   llvm::errs() << "error: '-selection' option must be specified using "
   ":: or "
-  "::-: format";
+  "::-: format\n";
   return nullptr;
 }
 
@@ -278,7 +316,18 @@
 llvm::errs() << llvm::toString(std::move(Err)) << "\n";
   }
 
-  // FIXME: Consume atomic changes and apply them to files.
+  void handle(AtomicChanges SourceReplacements) {
+AllSourceReplacements.insert(AllSourceReplacements.begin(),
+ SourceReplacements.begin(),
+ SourceReplacements.end());
+  }
+
+  const AtomicChanges &getAllSourceReplacements() const {
+return AllSourceReplacements;
+  }
+
+private:
+  AtomicChanges AllSourceReplacements;
 };
 
 class ClangRefactorTool {
@@ -358,6 +407,38 @@
 }
   }
 
+  bool applySourceReplacements(const AtomicChanges &Replacements) {
+std::set Files;
+for (const auto &Change : Replacements)
+  Files.insert(Change.getFilePath());
+// FIXME: Add automatic formatting support as well.
+tooling::ApplyChangesSpec Spec;
+Spec.Cleanup = false;
+for (const auto &File : Files) {
+  llvm::ErrorOr> BufferErr =
+  llvm::MemoryBuffer::getFile(File);
+  if (!BufferErr) {
+llvm::errs() << "error: failed to open" << File << " for rewriting\n";
+return true;
+  }
+  auto Result = tooling::applyAtomicChanges(File, (*BufferErr)->getBuffer(),
+Replacements, Spec);
+  if (!Result) {
+llvm::errs() << toString(Result.takeError());
+return true;
+  }
+
+  std::error_code EC;
+  llvm::raw_fd_ostream OS(File, EC, llvm::sys::fs::F_Text);
+  if (EC) {
+llvm::errs() << EC.message() << "\n";
+return true;
+  }
+  OS << *Result;
+

[PATCH] D37681: [refactor] Simplify the interface and remove some template magic

2017-10-02 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

I will commit this today. @klimek, let me know if there any issues in 
post-commit review.


Repository:
  rL LLVM

https://reviews.llvm.org/D37681



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

[PATCH] D37681: [refactor] Simplify the interface and remove some template magic

2017-10-02 Thread Alex Lorenz via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL314704: [refactor] Simplify the refactoring interface 
(authored by arphaman).

Changed prior to commit:
  https://reviews.llvm.org/D37681?vs=115211&id=117403#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D37681

Files:
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h
  
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirementsInternal.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRules.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringResultConsumer.h
  cfe/trunk/include/clang/Tooling/Refactoring/SourceSelectionConstraints.h
  cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
  cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  cfe/trunk/lib/Tooling/Refactoring/SourceSelectionConstraints.cpp
  cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp

Index: cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp
===
--- cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp
+++ cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp
@@ -18,7 +18,6 @@
 
 using namespace clang;
 using namespace tooling;
-using namespace refactoring_action_rules;
 
 namespace {
 
@@ -56,29 +55,39 @@
 }
 
 TEST_F(RefactoringActionRulesTest, MyFirstRefactoringRule) {
-  auto ReplaceAWithB =
-  [](const RefactoringRuleContext &,
- std::pair Selection)
-  -> Expected {
-const SourceManager &SM = Selection.first.getSources();
-SourceLocation Loc = Selection.first.getRange().getBegin().getLocWithOffset(
-Selection.second);
-AtomicChange Change(SM, Loc);
-llvm::Error E = Change.replace(SM, Loc, 1, "b");
-if (E)
-  return std::move(E);
-return AtomicChanges{Change};
-  };
-  class SelectionRequirement : public selection::Requirement {
-  public:
-std::pair
-evaluateSelection(const RefactoringRuleContext &,
-  selection::SourceSelectionRange Selection) const {
-  return std::make_pair(Selection, 20);
+  class ReplaceAWithB : public SourceChangeRefactoringRule {
+std::pair Selection;
+
+  public:
+ReplaceAWithB(std::pair Selection)
+: Selection(Selection) {}
+
+Expected
+createSourceReplacements(RefactoringRuleContext &Context) {
+  const SourceManager &SM = Context.getSources();
+  SourceLocation Loc =
+  Selection.first.getBegin().getLocWithOffset(Selection.second);
+  AtomicChange Change(SM, Loc);
+  llvm::Error E = Change.replace(SM, Loc, 1, "b");
+  if (E)
+return std::move(E);
+  return AtomicChanges{Change};
+}
+  };
+
+  class SelectionRequirement : public SourceRangeSelectionRequirement {
+  public:
+Expected>
+evaluate(RefactoringRuleContext &Context) const {
+  Expected R =
+  SourceRangeSelectionRequirement::evaluate(Context);
+  if (!R)
+return R.takeError();
+  return std::make_pair(*R, 20);
 }
   };
-  auto Rule = createRefactoringRule(ReplaceAWithB,
-requiredSelection(SelectionRequirement()));
+  auto Rule =
+  createRefactoringActionRule(SelectionRequirement());
 
   // When the requirements are satisifed, the rule's function must be invoked.
   {
@@ -123,54 +132,24 @@
 llvm::handleAllErrors(
 ErrorOrResult.takeError(),
 [&](llvm::StringError &Error) { Message = Error.getMessage(); });
-EXPECT_EQ(Message, "refactoring action can't be initiated with the "
-   "specified selection range");
+EXPECT_EQ(Message,
+  "refactoring action can't be initiated without a selection");
   }
 }
 
 TEST_F(RefactoringActionRulesTest, ReturnError) {
-  Expected (*Func)(const RefactoringRuleContext &,
-  selection::SourceSelectionRange) =
-  [](const RefactoringRuleContext &,
- selection::SourceSelectionRange) -> Expected {
-return llvm::make_error(
-"Error", llvm::make_error_code(llvm::errc::invalid_argument));
-  };
-  auto Rule = createRefactoringRule(
-  Func, requiredSelection(
-selection::identity()));
-
-  RefactoringRuleContext RefContext(Context.Sources);
-  SourceLocation Cursor =
-  Context.Sources.getLocForStartOfFile(Context.Sources.getMainFileID());
-  RefContext.setSelectionRange({Cursor, Cursor});
-  Expected Result = createReplacements(Rule, RefContext);
-
-  ASSERT_TRUE(!Result);
-  std::string Message;
-  llvm::handleAllErrors(Result.takeError(), [&](llvm::StringError &Error) {
-Message = Error.getMessage();
-  });
-  EXPECT_EQ(Message, "Error");
-}
-
-TEST_F(RefactoringActionRulesTest, ReturnInitiationDiagnostic) {
-  RefactoringRuleContext

[PATCH] D38618: Do not add a colon chunk to the code completion of class inheritance access modifiers

2017-10-06 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: include/clang/Sema/Scope.h:129
+/// We are between inheritance colon and the real class/struct definition 
scope
+ClassInheritanceScope = 0x40,
   };

Could you please rebase this patch? There's another scope flag that uses this 
value already.


https://reviews.llvm.org/D38618



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

[PATCH] D38538: Avoid printing some redundant name qualifiers in completion

2017-10-06 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: test/CodeCompletion/call.cpp:22
   // CHECK-CC1: COMPLETION: Pattern : dynamic_cast<<#type#>>(<#expression#>)
-  // CHECK-CC1: f(N::Y y, <#int ZZ#>)
+  // CHECK-CC1: f(Y y, <#int ZZ#>)
   // CHECK-CC1-NEXT: f(int i, <#int j#>, int k)

Could we also test a similar class to `Y` that's not typedefed, so you'd see 
`f(N::Y)`?



Comment at: test/CodeCompletion/enum-switch-case-qualified.cpp:25
 // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:23:8 %s -o - | 
FileCheck -check-prefix=CHECK-CC1 %s
-// CHECK-CC1: Blue : [#M::N::C::Color#]N::C::Blue
-// CHECK-CC1-NEXT: Green : [#M::N::C::Color#]N::C::Green
-// CHECK-CC1-NEXT: Indigo : [#M::N::C::Color#]N::C::Indigo
-// CHECK-CC1-NEXT: Orange : [#M::N::C::Color#]N::C::Orange
-// CHECK-CC1-NEXT: Red : [#M::N::C::Color#]N::C::Red
-// CHECK-CC1-NEXT: Violet : [#M::N::C::Color#]N::C::Violet
-// CHECK-CC1: Yellow : [#M::N::C::Color#]N::C::Yellow
+// CHECK-CC1: Blue : [#Color#]N::C::Blue
+// CHECK-CC1-NEXT: Green : [#Color#]N::C::Green

ilya-biryukov wrote:
> This may be a somewhat unwanted part of this change.
> Enum type is now written without qualifier here. I would argue that's ok, 
> since the actual enum values are always properly qualified (they have to be, 
> as they are actually inserted by completion) and those qualifiers provide all 
> the necessary context for the user.
I'm not 100% comfortable with making this kind of change right now. I'll try to 
investigate what's best for our users.


https://reviews.llvm.org/D38538



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

[PATCH] D38402: [clang-refactor] Apply source replacements

2017-10-06 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: test/Refactor/tool-apply-replacements.cpp:6
+
+class RenameMe { // CHECK: class {
+};

ioeric wrote:
> Why is the result `class {`?
Sorry, the test was broken. Fixed!


Repository:
  rL LLVM

https://reviews.llvm.org/D38402



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

[PATCH] D38402: [clang-refactor] Apply source replacements

2017-10-06 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 118042.
arphaman marked 5 inline comments as done.
arphaman added a comment.

Address review comments


Repository:
  rL LLVM

https://reviews.llvm.org/D38402

Files:
  include/clang/Frontend/CommandLineSourceLoc.h
  test/Refactor/tool-apply-replacements.cpp
  test/Refactor/tool-selection-option.c
  tools/clang-refactor/ClangRefactor.cpp

Index: tools/clang-refactor/ClangRefactor.cpp
===
--- tools/clang-refactor/ClangRefactor.cpp
+++ tools/clang-refactor/ClangRefactor.cpp
@@ -14,6 +14,7 @@
 //===--===//
 
 #include "TestSupport.h"
+#include "clang/Frontend/CommandLineSourceLoc.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Refactoring.h"
@@ -54,7 +55,7 @@
 
   /// Prints any additional state associated with the selection argument to
   /// the given output stream.
-  virtual void print(raw_ostream &OS) = 0;
+  virtual void print(raw_ostream &OS) {}
 
   /// Returns a replacement refactoring result consumer (if any) that should
   /// consume the results of a refactoring operation.
@@ -99,6 +100,41 @@
   TestSelectionRangesInFile TestSelections;
 };
 
+/// Stores the parsed -selection=filename:line:column[-line:column] option.
+class SourceRangeSelectionArgument final : public SourceSelectionArgument {
+public:
+  SourceRangeSelectionArgument(ParsedSourceRange Range)
+  : Range(std::move(Range)) {}
+
+  bool forAllRanges(const SourceManager &SM,
+llvm::function_ref Callback) override {
+const FileEntry *FE = SM.getFileManager().getFile(Range.FileName);
+FileID FID = FE ? SM.translateFile(FE) : FileID();
+if (!FE || FID.isInvalid()) {
+  llvm::errs() << "error: -selection=" << Range.FileName
+   << ":... : given file is not in the target TU\n";
+  return true;
+}
+
+SourceLocation Start = SM.getMacroArgExpandedLocation(
+SM.translateLineCol(FID, Range.Begin.first, Range.Begin.second));
+SourceLocation End = SM.getMacroArgExpandedLocation(
+SM.translateLineCol(FID, Range.End.first, Range.End.second));
+if (Start.isInvalid() || End.isInvalid()) {
+  llvm::errs() << "error: -selection=" << Range.FileName << ':'
+   << Range.Begin.first << ':' << Range.Begin.second << '-'
+   << Range.End.first << ':' << Range.End.second
+   << " : invalid source location\n";
+  return true;
+}
+Callback(SourceRange(Start, End));
+return false;
+  }
+
+private:
+  ParsedSourceRange Range;
+};
+
 std::unique_ptr
 SourceSelectionArgument::fromString(StringRef Value) {
   if (Value.startswith("test:")) {
@@ -110,10 +146,12 @@
 return llvm::make_unique(
 std::move(*ParsedTestSelection));
   }
-  // FIXME: Support true selection ranges.
+  Optional Range = ParsedSourceRange::fromString(Value);
+  if (Range)
+return llvm::make_unique(std::move(*Range));
   llvm::errs() << "error: '-selection' option must be specified using "
   ":: or "
-  "::-: format";
+  "::-: format\n";
   return nullptr;
 }
 
@@ -272,7 +310,14 @@
 llvm::errs() << llvm::toString(std::move(Err)) << "\n";
   }
 
-  // FIXME: Consume atomic changes and apply them to files.
+  void handle(AtomicChanges Changes) {
+SourceChanges.insert(SourceChanges.begin(), Changes.begin(), Changes.end());
+  }
+
+  const AtomicChanges &getSourceChanges() const { return SourceChanges; }
+
+private:
+  AtomicChanges SourceChanges;
 };
 
 class ClangRefactorTool {
@@ -352,6 +397,39 @@
 }
   }
 
+  bool applySourceChanges(const AtomicChanges &Replacements) {
+std::set Files;
+for (const auto &Change : Replacements)
+  Files.insert(Change.getFilePath());
+// FIXME: Add automatic formatting support as well.
+tooling::ApplyChangesSpec Spec;
+// FIXME: We should probably cleanup the result by default as well.
+Spec.Cleanup = false;
+for (const auto &File : Files) {
+  llvm::ErrorOr> BufferErr =
+  llvm::MemoryBuffer::getFile(File);
+  if (!BufferErr) {
+llvm::errs() << "error: failed to open" << File << " for rewriting\n";
+return true;
+  }
+  auto Result = tooling::applyAtomicChanges(File, (*BufferErr)->getBuffer(),
+Replacements, Spec);
+  if (!Result) {
+llvm::errs() << toString(Result.takeError());
+return true;
+  }
+
+  std::error_code EC;
+  llvm::raw_fd_ostream OS(File, EC, llvm::sys::fs::F_Text);
+  if (EC) {
+llvm::errs() << EC.message() << "\n";
+return true;
+  }
+  OS << *Result;
+}
+return false;
+  }
+
   bool invokeAction(RefactoringActionSubcommand &Subcommand,
 const CompilationDatabase &DB,

[PATCH] D37856: [refactor] add support for refactoring options

2017-10-06 Thread Alex Lorenz via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
arphaman marked 3 inline comments as done.
Closed by commit rL315087: [refactor] add support for refactoring options 
(authored by arphaman).

Changed prior to commit:
  https://reviews.llvm.org/D37856?vs=117124&id=118044#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D37856

Files:
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h
  
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringOption.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringOptionVisitor.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringOptions.h
  cfe/trunk/include/clang/Tooling/Refactoring/Rename/RenamingAction.h
  cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  cfe/trunk/test/Refactor/LocalRename/Field.cpp
  cfe/trunk/test/Refactor/tool-test-support.c
  cfe/trunk/tools/clang-refactor/ClangRefactor.cpp

Index: cfe/trunk/tools/clang-refactor/ClangRefactor.cpp
===
--- cfe/trunk/tools/clang-refactor/ClangRefactor.cpp
+++ cfe/trunk/tools/clang-refactor/ClangRefactor.cpp
@@ -18,6 +18,7 @@
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/Refactoring/RefactoringAction.h"
+#include "clang/Tooling/Refactoring/RefactoringOptions.h"
 #include "clang/Tooling/Refactoring/Rename/RenamingAction.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/CommandLine.h"
@@ -32,10 +33,10 @@
 
 namespace opts {
 
-static cl::OptionCategory CommonRefactorOptions("Common refactoring options");
+static cl::OptionCategory CommonRefactorOptions("Refactoring options");
 
 static cl::opt Verbose("v", cl::desc("Use verbose output"),
- cl::cat(CommonRefactorOptions),
+ cl::cat(cl::GeneralCategory),
  cl::sub(*cl::AllSubCommands));
 } // end namespace opts
 
@@ -116,6 +117,92 @@
   return nullptr;
 }
 
+/// A container that stores the command-line options used by a single
+/// refactoring option.
+class RefactoringActionCommandLineOptions {
+public:
+  void addStringOption(const RefactoringOption &Option,
+   std::unique_ptr> CLOption) {
+StringOptions[&Option] = std::move(CLOption);
+  }
+
+  const cl::opt &
+  getStringOption(const RefactoringOption &Opt) const {
+auto It = StringOptions.find(&Opt);
+return *It->second;
+  }
+
+private:
+  llvm::DenseMap>>
+  StringOptions;
+};
+
+/// Passes the command-line option values to the options used by a single
+/// refactoring action rule.
+class CommandLineRefactoringOptionVisitor final
+: public RefactoringOptionVisitor {
+public:
+  CommandLineRefactoringOptionVisitor(
+  const RefactoringActionCommandLineOptions &Options)
+  : Options(Options) {}
+
+  void visit(const RefactoringOption &Opt,
+ Optional &Value) override {
+const cl::opt &CLOpt = Options.getStringOption(Opt);
+if (!CLOpt.getValue().empty()) {
+  Value = CLOpt.getValue();
+  return;
+}
+Value = None;
+if (Opt.isRequired())
+  MissingRequiredOptions.push_back(&Opt);
+  }
+
+  ArrayRef getMissingRequiredOptions() const {
+return MissingRequiredOptions;
+  }
+
+private:
+  llvm::SmallVector MissingRequiredOptions;
+  const RefactoringActionCommandLineOptions &Options;
+};
+
+/// Creates the refactoring options used by all the rules in a single
+/// refactoring action.
+class CommandLineRefactoringOptionCreator final
+: public RefactoringOptionVisitor {
+public:
+  CommandLineRefactoringOptionCreator(
+  cl::OptionCategory &Category, cl::SubCommand &Subcommand,
+  RefactoringActionCommandLineOptions &Options)
+  : Category(Category), Subcommand(Subcommand), Options(Options) {}
+
+  void visit(const RefactoringOption &Opt, Optional &) override {
+if (Visited.insert(&Opt).second)
+  Options.addStringOption(Opt, create(Opt));
+  }
+
+private:
+  template 
+  std::unique_ptr> create(const RefactoringOption &Opt) {
+if (!OptionNames.insert(Opt.getName()).second)
+  llvm::report_fatal_error("Multiple identical refactoring options "
+   "specified for one refactoring action");
+// FIXME: cl::Required can be specified when this option is present
+// in all rules in an action.
+return llvm::make_unique>(
+Opt.getName(), cl::desc(Opt.getDescription()), cl::Optional,
+cl::cat(Category), cl::sub(Subcommand));
+  }
+
+  llvm::SmallPtrSet Visited;
+  llvm::StringSet<> OptionNames;
+  cl::OptionCategory &Category;
+  cl::SubCommand &Subcommand;
+  RefactoringActionCommandLineOptions &Options;
+};
+
 /// A subcommand that corresponds to individual refactoring action.
 class RefactoringActio

[PATCH] D37856: [refactor] add support for refactoring options

2017-10-06 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:73
+template 
+class OptionRequirement : public RefactoringOptionsRequirement {
+public:

ioeric wrote:
> nit: `OptionRequirement` sounds more general than the base class 
> `RefactoringOptionsRequirement`. 
I couldn't really think of a better name, sorry.


Repository:
  rL LLVM

https://reviews.llvm.org/D37856



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

[PATCH] D38643: PR13575: Fix USR mangling for fixed-size arrays.

2017-10-06 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

Thanks for working on this!
Could you please post the patch with full context (`git diff -U99`)?




Comment at: test/Index/USR/array-type.cpp:1
+// RUN: c-index-test core -print-source-symbols -- %s | grep 
"function(Gen,TS)/C++" | grep foo | cut -s -d "|" -f 4 | uniq | wc -l | grep 3
+

Please use `FileCheck` and verify the exact USR strings. This way you'll know 
that they're unique without actually trying to verify if they're unique in the 
test.


Repository:
  rL LLVM

https://reviews.llvm.org/D38643



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

[PATCH] D38643: PR13575: Fix USR mangling for fixed-size arrays.

2017-10-06 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: lib/Index/USRGeneration.cpp:820
+if (const ArrayType *const AT = dyn_cast(T)) {
+  VisitType(AT->getElementType());
+  Out << "[";

We should probably follow the other types and just set `T = 
AT->getElementType()` instead of using `VisitType` and `continue` instead of 
`return`ing at the end of this if.



Comment at: lib/Index/USRGeneration.cpp:821
+  VisitType(AT->getElementType());
+  Out << "[";
+

The "[" "]" syntax could collide with the vector-type USRs. What about using 
another character? Maybe '{'?



Comment at: lib/Index/USRGeneration.cpp:826
+case ArrayType::Star: Out << "*"; break;
+default : ;
+  }

I think it's better to check 'ArrayType::Normal' instead of `default` to ensure 
we will be able to distinguish between added size modifiers that could be added 
in the future. We should also probably give it some representation, like 'n'.


https://reviews.llvm.org/D38643



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

[PATCH] D38643: PR13575: Fix USR mangling for fixed-size arrays.

2017-10-06 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

Thanks, it looks much better! A couple more comments:




Comment at: lib/Index/USRGeneration.cpp:819
 }
+if (const ArrayType *const AT = dyn_cast(T)) {
+  Out << "{";

You can use `const auto *` here.



Comment at: lib/Index/USRGeneration.cpp:826
+  }
+  if (const ConstantArrayType* const CAT = dyn_cast(T)) 
{
+Out << CAT->getSize();

Ditto. Also, the braces are redundant.



Comment at: lib/Index/USRGeneration.cpp:829
+  }
+  Out << "}";
+  T = AT->getElementType();

I don't think we need the terminating character.


https://reviews.llvm.org/D38643



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

[PATCH] D38643: PR13575: Fix USR mangling for fixed-size arrays.

2017-10-09 Thread Alex Lorenz via Phabricator via cfe-commits

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

LGTM




Comment at: lib/Index/USRGeneration.cpp:819
 }
+if (const auto *const AT = dyn_cast(T)) {
+  Out << "{";

Nit: I don't think you really need the 2nd const here and in the next if.


https://reviews.llvm.org/D38643



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

[PATCH] D38643: PR13575: Fix USR mangling for fixed-size arrays.

2017-10-09 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: lib/Index/USRGeneration.cpp:820
+if (const auto *const AT = dyn_cast(T)) {
+  Out << "{";
+  switch (AT->getSizeModifier()) {

You might also want to use the character literals for one char strings for 
efficiency.


https://reviews.llvm.org/D38643



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

[PATCH] D38708: [AST] Flag the typo-corrected nodes for better tooling

2017-10-09 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.

This patch adds a new boolean field to the `DeclRefExpr`, `MemberExpr`, 
`CXXCtorInitializer`, `ObjCIvarRefExpr`, `ObjCPropertyRefExpr` nodes which is 
set to true when these nodes have been produced during typo-correction.

This is useful for Clang-based tooling as it can distinguish between true 
references and the typo-corrected references. The initial tooling support uses 
the flag to prevent token annotation for typo-corrected references and to 
prevent finding typo-corrected references during single TU reference search.


Repository:
  rL LLVM

https://reviews.llvm.org/D38708

Files:
  include/clang/AST/DeclCXX.h
  include/clang/AST/Expr.h
  include/clang/AST/ExprObjC.h
  include/clang/AST/Stmt.h
  lib/AST/DeclCXX.cpp
  lib/AST/Expr.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaExprMember.cpp
  lib/Sema/SemaExprObjC.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaPseudoObject.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/TreeTransform.h
  lib/Serialization/ASTReader.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterDecl.cpp
  lib/Serialization/ASTWriterStmt.cpp
  test/Index/typo-annotate-tokens.mm
  test/Index/typo-file-refs.cpp
  test/Index/typo-file-refs.m
  tools/libclang/CIndex.cpp
  tools/libclang/CIndexHigh.cpp
  tools/libclang/CursorVisitor.h

Index: tools/libclang/CursorVisitor.h
===
--- tools/libclang/CursorVisitor.h
+++ tools/libclang/CursorVisitor.h
@@ -96,6 +96,9 @@
   /// record entries.
   bool VisitDeclsOnly;
 
+  /// \brief Whether we should visit typo-corrected references.
+  bool VisitTypoCorrected = true;
+
   // FIXME: Eventually remove.  This part of a hack to support proper
   // iteration over all Decls contained lexically within an ObjC container.
   DeclContext::decl_iterator *DI_current;
@@ -187,6 +190,8 @@
 return VisitIncludedEntities;
   }
 
+  void setVisitTypoCorrected(bool V = true) { VisitTypoCorrected = V; }
+
   template
   bool visitPreprocessedEntities(InputIterator First, InputIterator Last,
  PreprocessingRecord &PPRec,
Index: tools/libclang/CIndexHigh.cpp
===
--- tools/libclang/CIndexHigh.cpp
+++ tools/libclang/CIndexHigh.cpp
@@ -169,7 +169,9 @@
 if (clang_isExpression(cursor.kind)) {
   if (cursor.kind == CXCursor_DeclRefExpr ||
   cursor.kind == CXCursor_MemberRefExpr) {
-// continue..
+// Avoid visiting typo-corrected references.
+if (cxcursor::getCursorExpr(cursor)->isTypoCorrected())
+  return CXChildVisit_Continue;
 
   } else if (cursor.kind == CXCursor_ObjCMessageExpr &&
  cxcursor::getSelectorIdentifierIndex(cursor) != -1) {
@@ -228,8 +230,11 @@
   Visitor);
 
   if (const DeclContext *DC = Dcl->getParentFunctionOrMethod()) {
-return clang_visitChildren(cxcursor::MakeCXCursor(cast(DC), TU),
-   findFileIdRefVisit, &data);
+CursorVisitor CursorVis(TU, findFileIdRefVisit, &data,
+/*VisitPreprocessorLast=*/false);
+// Don't include the typo-corrected references.
+CursorVis.setVisitTypoCorrected(false);
+return CursorVis.VisitChildren(cxcursor::MakeCXCursor(cast(DC), TU));
   }
 
   SourceRange Range(SM.getLocForStartOfFile(FID), SM.getLocForEndOfFile(FID));
@@ -239,6 +244,8 @@
   /*VisitIncludedEntities=*/false,
   Range,
   /*VisitDeclsOnly=*/true);
+  // Don't include the typo-corrected references.
+  FindIdRefsVisitor.setVisitTypoCorrected(false);
   return FindIdRefsVisitor.visitFileRegion();
 }
 
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -847,13 +847,15 @@
   // Visit the initializers in source order
   for (unsigned I = 0, N = WrittenInits.size(); I != N; ++I) {
 CXXCtorInitializer *Init = WrittenInits[I];
-if (Init->isAnyMemberInitializer()) {
-  if (Visit(MakeCursorMemberRef(Init->getAnyMember(),
-Init->getMemberLocation(), TU)))
-return true;
-} else if (TypeSourceInfo *TInfo = Init->getTypeSourceInfo()) {
-  if (Visit(TInfo->getTypeLoc()))
-return true;
+if (VisitTypoCorrected | !Init->isTypoCorrected()) {
+  if (Init->isAnyMemberInitializer()) {
+if (Visit(MakeCursorMemberRef(Init->getAnyMember(),
+  Init->getMemberLocation(), TU)))
+  return true;
+  } else if (TypeSourceInfo *TInfo = Init->getTypeSourceInfo()) {
+if (Visit(TInfo->getTypeLoc()))

[PATCH] D38707: PR13575: Fix USR mangling for functions taking function pointers as arguments.

2017-10-09 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: lib/Index/USRGeneration.cpp:757
   VisitType(FT->getReturnType());
-  for (const auto &I : FT->param_types())
+  Out << '(';
+  for (const auto &I : FT->param_types()) {

I believe you can drop the '(' and ')'. The '#' should be enough to prevent the 
USR collision.


https://reviews.llvm.org/D38707



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

[PATCH] D38707: PR13575: Fix USR mangling for functions taking function pointers as arguments.

2017-10-09 Thread Alex Lorenz via Phabricator via cfe-commits

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

LGTM.


https://reviews.llvm.org/D38707



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

[PATCH] D38567: [config] Warn when POSIX_C_SOURCE breaks threading support on Darwin

2017-10-10 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: include/__threading_support:26
 
+#if (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) && 
(__MAC_OS_X_VERSION_MIN_REQUIRED < 101300)) \
+|| (defined(__IPHONE_OS_VERSION_MIN_REQUIRED) && 
(__IPHONE_OS_VERSION_MIN_REQUIRED < 11)) \

Please add a brief comment that describes why the check and the warning are 
needed.


https://reviews.llvm.org/D38567



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

[PATCH] D38538: Avoid printing some redundant name qualifiers in completion

2017-10-10 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: test/CodeCompletion/qualifiers-as-written.cpp:29
+  // CHECK-2: COMPLETION: func : [#int#]func(<#foo a#>, <#bar b#>, <#ns::bar 
c#>, <#ns::baz d#>
+  // CHECK-2: COMPLETION: func : [#int#]func(<#foo::type a#>, <#bar b#>, <#baz 
c#>
+}

Sorry, I see the issue now. However, I don't think that we'd like to change the 
signature for a function like this, as we'd still prefer to show `func 
(foo::type, ns::bar, ns::baz);` on this line.

In Xcode we actually avoid the problem with `std::vector<>`s that you've 
pointed out entirely by using `value_type`. I'll check what our solution does.

Btw, maybe using things like `value_type` is completely wrong (with or without 
the qualifier)? If we have `std::vector` shouldn't we rather show 
`push_back(int _Value)`, rather than the `value_type`? Perhaps this kind of 
change should be discussed with a wider community somehow to find out what's 
best for all users.


https://reviews.llvm.org/D38538



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

[PATCH] D38402: [clang-refactor] Apply source replacements

2017-10-10 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 118471.
arphaman marked 6 inline comments as done.
arphaman added a comment.

Address review comments


Repository:
  rL LLVM

https://reviews.llvm.org/D38402

Files:
  include/clang/Frontend/CommandLineSourceLoc.h
  test/Refactor/tool-apply-replacements.cpp
  test/Refactor/tool-selection-option.c
  tools/clang-refactor/ClangRefactor.cpp

Index: tools/clang-refactor/ClangRefactor.cpp
===
--- tools/clang-refactor/ClangRefactor.cpp
+++ tools/clang-refactor/ClangRefactor.cpp
@@ -14,6 +14,7 @@
 //===--===//
 
 #include "TestSupport.h"
+#include "clang/Frontend/CommandLineSourceLoc.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Refactoring.h"
@@ -54,7 +55,7 @@
 
   /// Prints any additional state associated with the selection argument to
   /// the given output stream.
-  virtual void print(raw_ostream &OS) = 0;
+  virtual void print(raw_ostream &OS) {}
 
   /// Returns a replacement refactoring result consumer (if any) that should
   /// consume the results of a refactoring operation.
@@ -99,6 +100,41 @@
   TestSelectionRangesInFile TestSelections;
 };
 
+/// Stores the parsed -selection=filename:line:column[-line:column] option.
+class SourceRangeSelectionArgument final : public SourceSelectionArgument {
+public:
+  SourceRangeSelectionArgument(ParsedSourceRange Range)
+  : Range(std::move(Range)) {}
+
+  bool forAllRanges(const SourceManager &SM,
+llvm::function_ref Callback) override {
+const FileEntry *FE = SM.getFileManager().getFile(Range.FileName);
+FileID FID = FE ? SM.translateFile(FE) : FileID();
+if (!FE || FID.isInvalid()) {
+  llvm::errs() << "error: -selection=" << Range.FileName
+   << ":... : given file is not in the target TU\n";
+  return true;
+}
+
+SourceLocation Start = SM.getMacroArgExpandedLocation(
+SM.translateLineCol(FID, Range.Begin.first, Range.Begin.second));
+SourceLocation End = SM.getMacroArgExpandedLocation(
+SM.translateLineCol(FID, Range.End.first, Range.End.second));
+if (Start.isInvalid() || End.isInvalid()) {
+  llvm::errs() << "error: -selection=" << Range.FileName << ':'
+   << Range.Begin.first << ':' << Range.Begin.second << '-'
+   << Range.End.first << ':' << Range.End.second
+   << " : invalid source location\n";
+  return true;
+}
+Callback(SourceRange(Start, End));
+return false;
+  }
+
+private:
+  ParsedSourceRange Range;
+};
+
 std::unique_ptr
 SourceSelectionArgument::fromString(StringRef Value) {
   if (Value.startswith("test:")) {
@@ -110,10 +146,12 @@
 return llvm::make_unique(
 std::move(*ParsedTestSelection));
   }
-  // FIXME: Support true selection ranges.
+  Optional Range = ParsedSourceRange::fromString(Value);
+  if (Range)
+return llvm::make_unique(std::move(*Range));
   llvm::errs() << "error: '-selection' option must be specified using "
   ":: or "
-  "::-: format";
+  "::-: format\n";
   return nullptr;
 }
 
@@ -268,11 +306,18 @@
 
 class ClangRefactorConsumer : public RefactoringResultConsumer {
 public:
-  void handleError(llvm::Error Err) {
+  void handleError(llvm::Error Err) override {
 llvm::errs() << llvm::toString(std::move(Err)) << "\n";
   }
 
-  // FIXME: Consume atomic changes and apply them to files.
+  void handle(AtomicChanges Changes) override {
+SourceChanges.insert(SourceChanges.begin(), Changes.begin(), Changes.end());
+  }
+
+  const AtomicChanges &getSourceChanges() const { return SourceChanges; }
+
+private:
+  AtomicChanges SourceChanges;
 };
 
 class ClangRefactorTool {
@@ -352,6 +397,39 @@
 }
   }
 
+  bool applySourceChanges(const AtomicChanges &Replacements) {
+std::set Files;
+for (const auto &Change : Replacements)
+  Files.insert(Change.getFilePath());
+// FIXME: Add automatic formatting support as well.
+tooling::ApplyChangesSpec Spec;
+// FIXME: We should probably cleanup the result by default as well.
+Spec.Cleanup = false;
+for (const auto &File : Files) {
+  llvm::ErrorOr> BufferErr =
+  llvm::MemoryBuffer::getFile(File);
+  if (!BufferErr) {
+llvm::errs() << "error: failed to open " << File << " for rewriting\n";
+return true;
+  }
+  auto Result = tooling::applyAtomicChanges(File, (*BufferErr)->getBuffer(),
+Replacements, Spec);
+  if (!Result) {
+llvm::errs() << toString(Result.takeError());
+return true;
+  }
+
+  std::error_code EC;
+  llvm::raw_fd_ostream OS(File, EC, llvm::sys::fs::F_Text);
+  if (EC) {
+llvm::errs() << EC.message() << "\n";
+return true;
+  }
+

[PATCH] D38755: Fixed crash during indexing default template template param

2017-10-10 Thread Alex Lorenz via Phabricator via cfe-commits

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

Thanks, LGTM.


https://reviews.llvm.org/D38755



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

[PATCH] D38772: [refactor] allow the use of refactoring diagnostics

2017-10-10 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.
Herald added a subscriber: mgorny.

This patch allows the refactoring library to use its own set of 
refactoring-specific diagnostics to reports things like initiation errors.


Repository:
  rL LLVM

https://reviews.llvm.org/D38772

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/Basic/DiagnosticRefactoringKinds.td
  include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  include/clang/Tooling/Refactoring/RefactoringDiagnostic.h
  include/clang/Tooling/Refactoring/RefactoringRuleContext.h
  lib/Basic/DiagnosticIDs.cpp
  lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  test/Refactor/LocalRename/NoSymbolSelectedError.cpp
  tools/clang-refactor/ClangRefactor.cpp
  tools/clang-refactor/TestSupport.cpp
  tools/clang-refactor/TestSupport.h
  tools/clang-refactor/ToolRefactoringResultConsumer.h
  tools/diagtool/DiagnosticNames.cpp

Index: tools/diagtool/DiagnosticNames.cpp
===
--- tools/diagtool/DiagnosticNames.cpp
+++ tools/diagtool/DiagnosticNames.cpp
@@ -42,6 +42,7 @@
 #include "clang/Basic/DiagnosticCommentKinds.inc"
 #include "clang/Basic/DiagnosticSemaKinds.inc"
 #include "clang/Basic/DiagnosticAnalysisKinds.inc"
+#include "clang/Basic/DiagnosticRefactoringKinds.inc"
 #undef DIAG
 };
 
Index: tools/clang-refactor/ToolRefactoringResultConsumer.h
===
--- /dev/null
+++ tools/clang-refactor/ToolRefactoringResultConsumer.h
@@ -0,0 +1,48 @@
+//===--- ToolRefactoringResultConsumer.h - --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_CLANG_REFACTOR_TOOL_REFACTORING_RESULT_CONSUMER_H
+#define LLVM_CLANG_TOOLS_CLANG_REFACTOR_TOOL_REFACTORING_RESULT_CONSUMER_H
+
+#include "clang/AST/ASTContext.h"
+#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
+
+namespace clang {
+namespace refactor {
+
+/// A subclass of \c RefactoringResultConsumer that stores the reference to the
+/// TU-specific diagnostics engine.
+class ClangRefactorToolResultConsumer
+: public tooling::RefactoringResultConsumer {
+public:
+  /// Called when a TU is entered.
+  void beginTU(ASTContext &Context) {
+assert(!Diags && "Diags has been set");
+Diags = &Context.getDiagnostics();
+  }
+
+  /// Called when the tool is done with a TU.
+  void endTU() {
+assert(Diags && "Diags unset");
+Diags = nullptr;
+  }
+
+  DiagnosticsEngine &getDiags() const {
+assert(Diags && "no diags");
+return *Diags;
+  }
+
+private:
+  DiagnosticsEngine *Diags = nullptr;
+};
+
+} // end namespace refactor
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLS_CLANG_REFACTOR_TOOL_REFACTORING_RESULT_CONSUMER_H
Index: tools/clang-refactor/TestSupport.h
===
--- tools/clang-refactor/TestSupport.h
+++ tools/clang-refactor/TestSupport.h
@@ -16,9 +16,9 @@
 #ifndef LLVM_CLANG_TOOLS_CLANG_REFACTOR_TEST_SUPPORT_H
 #define LLVM_CLANG_TOOLS_CLANG_REFACTOR_TEST_SUPPORT_H
 
+#include "ToolRefactoringResultConsumer.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Error.h"
@@ -65,7 +65,7 @@
   bool foreachRange(const SourceManager &SM,
 llvm::function_ref Callback) const;
 
-  std::unique_ptr createConsumer() const;
+  std::unique_ptr createConsumer() const;
 
   void dump(llvm::raw_ostream &OS) const;
 };
Index: tools/clang-refactor/TestSupport.cpp
===
--- tools/clang-refactor/TestSupport.cpp
+++ tools/clang-refactor/TestSupport.cpp
@@ -14,6 +14,7 @@
 //===--===//
 
 #include "TestSupport.h"
+#include "clang/Basic/DiagnosticError.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/STLExtras.h"
@@ -106,7 +107,7 @@
 }
 
 class TestRefactoringResultConsumer final
-: public tooling::RefactoringResultConsumer {
+: public ClangRefactorToolResultConsumer {
 public:
   TestRefactoringResultConsumer(const TestSelectionRangesInFile &TestRanges)
   : TestRanges(TestRanges) {
@@ -182,10 +183,15 @@
   std::string ErrorMessage;
   bool HasResult = !!Result;
   if (!HasResult) {
-// FIXME: Handle diagnostic error as well.
-handleAllErrors(Result.takeError(), [&](StringError &Err) {
-  ErrorMessage = Err.

[PATCH] D37856: [refactor] add support for refactoring options

2017-10-11 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

In https://reviews.llvm.org/D37856#894638, @hokein wrote:

> Sorry for the delay. I saw you have reverted this commit somehow. A post 
> commit.


I had some issues with ppc64/s390x bots for some reason, so I had to revert. 
I'm still trying to investigate what went wrong there, it looks like ASAN/UBSAN 
are not catching anything though.




Comment at: cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp:113
 Rules.push_back(createRefactoringActionRule(
-SymbolSelectionRequirement()));
+SymbolSelectionRequirement(), OptionRequirement()));
 return Rules;

hokein wrote:
> Thought it a bit more: it requires all of the requirements are satisfied, I 
> think we need to support "one-of" option. For example,  we have two option 
> "-a" and "-b",  only one of them is allowed to be present at the same time.
That should be straightforward enough to implement, either with a custom class 
or with a built-in requirement class. I'll probably add a builtin one when the 
need comes up.


Repository:
  rL LLVM

https://reviews.llvm.org/D37856



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

[PATCH] D38772: [refactor] allow the use of refactoring diagnostics

2017-10-11 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: lib/Basic/DiagnosticIDs.cpp:46
   unsigned WarnShowInSystemHeader : 1;
-  unsigned Category : 5;
+  unsigned Category : 6;
 

hokein wrote:
> just curious: is this change needed?
I get a build warning without this change as the bitfield becomes too narrow 
with the new category, so yeah.



Comment at: tools/clang-refactor/ToolRefactoringResultConsumer.h:19
+
+/// A subclass of \c RefactoringResultConsumer that stores the reference to the
+/// TU-specific diagnostics engine.

hokein wrote:
> I'd name it "interface", because it has unimplemented virtual function 
> (`handleError`), clients can't create an instance of it. 
> 
> or alternatively,  does it make more sense to just add these methods and 
> `DiagnosticsEngine` variable to the `tooling::RefactoringResultConsumer` 
> interface? I see you have replaced "RefactoringResultConsumer" with this new 
> interface in many places. 
Right now I don't think having the diagnostics engine will be useful for 
clients outside of tool, so I'd prefer to keep it here. We can reconsider this 
decision in the future if we need to.


Repository:
  rL LLVM

https://reviews.llvm.org/D38772



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

[PATCH] D38772: [refactor] allow the use of refactoring diagnostics

2017-10-11 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 118701.
arphaman marked 2 inline comments as done.
arphaman added a comment.

- rename the common consumer class.


Repository:
  rL LLVM

https://reviews.llvm.org/D38772

Files:
  include/clang/Basic/AllDiagnostics.h
  include/clang/Basic/CMakeLists.txt
  include/clang/Basic/Diagnostic.td
  include/clang/Basic/DiagnosticIDs.h
  include/clang/Basic/DiagnosticRefactoringKinds.td
  include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  include/clang/Tooling/Refactoring/RefactoringDiagnostic.h
  include/clang/Tooling/Refactoring/RefactoringRuleContext.h
  lib/Basic/DiagnosticIDs.cpp
  lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  test/Refactor/LocalRename/NoSymbolSelectedError.cpp
  tools/clang-refactor/ClangRefactor.cpp
  tools/clang-refactor/TestSupport.cpp
  tools/clang-refactor/TestSupport.h
  tools/clang-refactor/ToolRefactoringResultConsumer.h
  tools/diagtool/DiagnosticNames.cpp

Index: tools/diagtool/DiagnosticNames.cpp
===
--- tools/diagtool/DiagnosticNames.cpp
+++ tools/diagtool/DiagnosticNames.cpp
@@ -42,6 +42,7 @@
 #include "clang/Basic/DiagnosticCommentKinds.inc"
 #include "clang/Basic/DiagnosticSemaKinds.inc"
 #include "clang/Basic/DiagnosticAnalysisKinds.inc"
+#include "clang/Basic/DiagnosticRefactoringKinds.inc"
 #undef DIAG
 };
 
Index: tools/clang-refactor/ToolRefactoringResultConsumer.h
===
--- /dev/null
+++ tools/clang-refactor/ToolRefactoringResultConsumer.h
@@ -0,0 +1,48 @@
+//===--- ToolRefactoringResultConsumer.h - --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_CLANG_REFACTOR_TOOL_REFACTORING_RESULT_CONSUMER_H
+#define LLVM_CLANG_TOOLS_CLANG_REFACTOR_TOOL_REFACTORING_RESULT_CONSUMER_H
+
+#include "clang/AST/ASTContext.h"
+#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
+
+namespace clang {
+namespace refactor {
+
+/// An interface that subclasses the \c RefactoringResultConsumer interface
+/// that stores the reference to the TU-specific diagnostics engine.
+class ClangRefactorToolConsumerInterface
+: public tooling::RefactoringResultConsumer {
+public:
+  /// Called when a TU is entered.
+  void beginTU(ASTContext &Context) {
+assert(!Diags && "Diags has been set");
+Diags = &Context.getDiagnostics();
+  }
+
+  /// Called when the tool is done with a TU.
+  void endTU() {
+assert(Diags && "Diags unset");
+Diags = nullptr;
+  }
+
+  DiagnosticsEngine &getDiags() const {
+assert(Diags && "no diags");
+return *Diags;
+  }
+
+private:
+  DiagnosticsEngine *Diags = nullptr;
+};
+
+} // end namespace refactor
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLS_CLANG_REFACTOR_TOOL_REFACTORING_RESULT_CONSUMER_H
Index: tools/clang-refactor/TestSupport.h
===
--- tools/clang-refactor/TestSupport.h
+++ tools/clang-refactor/TestSupport.h
@@ -16,9 +16,9 @@
 #ifndef LLVM_CLANG_TOOLS_CLANG_REFACTOR_TEST_SUPPORT_H
 #define LLVM_CLANG_TOOLS_CLANG_REFACTOR_TEST_SUPPORT_H
 
+#include "ToolRefactoringResultConsumer.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/RefactoringResultConsumer.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Error.h"
@@ -65,7 +65,7 @@
   bool foreachRange(const SourceManager &SM,
 llvm::function_ref Callback) const;
 
-  std::unique_ptr createConsumer() const;
+  std::unique_ptr createConsumer() const;
 
   void dump(llvm::raw_ostream &OS) const;
 };
Index: tools/clang-refactor/TestSupport.cpp
===
--- tools/clang-refactor/TestSupport.cpp
+++ tools/clang-refactor/TestSupport.cpp
@@ -14,6 +14,7 @@
 //===--===//
 
 #include "TestSupport.h"
+#include "clang/Basic/DiagnosticError.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/ADT/STLExtras.h"
@@ -106,7 +107,7 @@
 }
 
 class TestRefactoringResultConsumer final
-: public tooling::RefactoringResultConsumer {
+: public ClangRefactorToolConsumerInterface {
 public:
   TestRefactoringResultConsumer(const TestSelectionRangesInFile &TestRanges)
   : TestRanges(TestRanges) {
@@ -182,10 +183,15 @@
   std::string ErrorMessage;
   bool HasResult = !!Result;
   if (!HasResult) {
-// FIXME: Handle diagnostic error as well.
-handleAllErrors(Result.takeError(), [&](StringError &Err) {
-  ErrorMessage = Err.getMessage();
-

[PATCH] D38835: [refactor] selection: new CodeRangeASTSelection represents a set of selected consecu

2017-10-11 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.

Repository:
  rL LLVM

https://reviews.llvm.org/D38835

Files:
  include/clang/Tooling/Refactoring/ASTSelection.h
  lib/Tooling/Refactoring/ASTSelection.cpp
  unittests/Tooling/ASTSelectionTest.cpp

Index: unittests/Tooling/ASTSelectionTest.cpp
===
--- unittests/Tooling/ASTSelectionTest.cpp
+++ unittests/Tooling/ASTSelectionTest.cpp
@@ -29,12 +29,16 @@
 class SelectionFinderVisitor : public TestVisitor {
   FileLocation Location;
   Optional SelectionRange;
-  llvm::function_ref)> Consumer;
+  llvm::function_ref)>
+  Consumer;
 
 public:
-  SelectionFinderVisitor(
-  FileLocation Location, Optional SelectionRange,
-  llvm::function_ref)> Consumer)
+  SelectionFinderVisitor(FileLocation Location,
+ Optional SelectionRange,
+ llvm::function_ref)>
+ Consumer)
   : Location(Location), SelectionRange(SelectionRange), Consumer(Consumer) {
   }
 
@@ -50,20 +54,35 @@
   SourceLocation Loc = Location.translate(SM);
   SelRange = SourceRange(Loc, Loc);
 }
-Consumer(findSelectedASTNodes(Context, SelRange));
+Consumer(SelRange, findSelectedASTNodes(Context, SelRange));
 return false;
   }
 };
 
-void findSelectedASTNodes(
+void findSelectedASTNodesWithRange(
 StringRef Source, FileLocation Location, Optional SelectionRange,
-llvm::function_ref)> Consumer,
+llvm::function_ref)>
+Consumer,
 SelectionFinderVisitor::Language Language =
 SelectionFinderVisitor::Lang_CXX11) {
   SelectionFinderVisitor Visitor(Location, SelectionRange, Consumer);
   EXPECT_TRUE(Visitor.runOver(Source, Language));
 }
 
+void findSelectedASTNodes(
+StringRef Source, FileLocation Location, Optional SelectionRange,
+llvm::function_ref)> Consumer,
+SelectionFinderVisitor::Language Language =
+SelectionFinderVisitor::Lang_CXX11) {
+  findSelectedASTNodesWithRange(
+  Source, Location, SelectionRange,
+  [&](SourceRange, Optional Selection) {
+Consumer(std::move(Selection));
+  },
+  Language);
+}
+
 void checkNodeImpl(bool IsTypeMatched, const SelectedASTNode &Node,
SourceSelectionKind SelectionKind, unsigned NumChildren) {
   ASSERT_TRUE(IsTypeMatched);
@@ -649,4 +668,103 @@
   SelectionFinderVisitor::Lang_OBJC);
 }
 
+TEST(ASTSelectionFinder, SimpleCodeRangeASTSelection) {
+  StringRef Source = R"(void f(int x, int y) {
+  int z = x;
+  f(2, 3);
+  if (x == 0) {
+return;
+  }
+  x = 1;
+  return;
+}
+void f2() {
+  int m = 0;
+}
+)";
+  // Empty range is an invalid code range.
+  findSelectedASTNodesWithRange(
+  Source, {2, 2}, None,
+  [](SourceRange SelectionRange, Optional Node) {
+EXPECT_TRUE(Node);
+Optional SelectedCode =
+CodeRangeASTSelection::create(SelectionRange, std::move(*Node));
+EXPECT_FALSE(SelectedCode);
+  });
+  // Range that spans multiple functions is an invalid code range.
+  findSelectedASTNodesWithRange(
+  Source, {2, 2}, FileRange{{7, 2}, {12, 1}},
+  [](SourceRange SelectionRange, Optional Node) {
+EXPECT_TRUE(Node);
+Optional SelectedCode =
+CodeRangeASTSelection::create(SelectionRange, std::move(*Node));
+EXPECT_FALSE(SelectedCode);
+  });
+  // Just 'z = x;':
+  findSelectedASTNodesWithRange(
+  Source, {2, 2}, FileRange{{2, 2}, {2, 13}},
+  [](SourceRange SelectionRange, Optional Node) {
+EXPECT_TRUE(Node);
+Optional SelectedCode =
+CodeRangeASTSelection::create(SelectionRange, std::move(*Node));
+EXPECT_TRUE(SelectedCode);
+EXPECT_EQ(SelectedCode->size(), 1u);
+EXPECT_TRUE(isa((*SelectedCode)[0]));
+ArrayRef Parents =
+SelectedCode->getParents();
+EXPECT_EQ(Parents.size(), 3u);
+EXPECT_TRUE(
+isa(Parents[0].get().Node.get()));
+EXPECT_TRUE(isa(Parents[1].get().Node.get()));
+EXPECT_TRUE(isa(Parents[2].get().Node.get()));
+  });
+  // From 'f(2,3)' until just before 'x = 1;':
+  findSelectedASTNodesWithRange(
+  Source, {3, 2}, FileRange{{3, 2}, {7, 1}},
+  [](SourceRange SelectionRange, Optional Node) {
+EXPECT_TRUE(Node);
+Optional SelectedCode =
+CodeRangeASTSelection::create(SelectionRange, std::move(*Node));
+EXPECT_TRUE(SelectedCode);
+EXPECT_EQ(SelectedCode->size(), 2u);
+EXPECT_TRUE(isa((*SelectedCode)[0]));
+EXPECT_TRUE(isa((*SelectedCode)[1]));
+ArrayRef Parents =
+SelectedCode->getParents();
+EXPECT_EQ(Parents.size(), 3u);
+EXPECT_TRUE(
+isa(Parents[0].get().Node.get()));
+EXPECT_TRUE(isa(Parents[1].get().Node.get()));
+EXPECT_TRUE(isa(Parents[2].get().Node.get()));
+  });
+  // From 'f(2,3)' until just before ';' in 'x = 1;':
+  findSelec

[PATCH] D38863: Typos in tutorial

2017-10-12 Thread Alex Lorenz via Phabricator via cfe-commits

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

You generally don't need a pre-commit review for such a change.


https://reviews.llvm.org/D38863



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

[PATCH] D38402: [clang-refactor] Apply source replacements

2017-10-13 Thread Alex Lorenz via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL315738: [clang-refactor] Apply source replacements (authored 
by arphaman).

Changed prior to commit:
  https://reviews.llvm.org/D38402?vs=118471&id=118959#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38402

Files:
  cfe/trunk/include/clang/Frontend/CommandLineSourceLoc.h
  cfe/trunk/test/Refactor/tool-apply-replacements.cpp
  cfe/trunk/test/Refactor/tool-selection-option.c
  cfe/trunk/tools/clang-refactor/ClangRefactor.cpp

Index: cfe/trunk/include/clang/Frontend/CommandLineSourceLoc.h
===
--- cfe/trunk/include/clang/Frontend/CommandLineSourceLoc.h
+++ cfe/trunk/include/clang/Frontend/CommandLineSourceLoc.h
@@ -51,6 +51,51 @@
   }
 };
 
+/// A source range that has been parsed on the command line.
+struct ParsedSourceRange {
+  std::string FileName;
+  /// The starting location of the range. The first element is the line and
+  /// the second element is the column.
+  std::pair Begin;
+  /// The ending location of the range. The first element is the line and the
+  /// second element is the column.
+  std::pair End;
+
+  /// Returns a parsed source range from a string or None if the string is
+  /// invalid.
+  ///
+  /// These source string has the following format:
+  ///
+  /// file:start_line:start_column[-end_line:end_column]
+  ///
+  /// If the end line and column are omitted, the starting line and columns
+  /// are used as the end values.
+  static Optional fromString(StringRef Str) {
+std::pair RangeSplit;
+// Avoid splitting '-' when there's no end line & column as '-' might be
+// part of the filename.
+if (Str.count(':') > 2)
+  RangeSplit = Str.rsplit('-');
+else
+  RangeSplit = {Str, ""};
+auto Begin = ParsedSourceLocation::FromString(RangeSplit.first);
+if (Begin.FileName.empty())
+  return None;
+unsigned EndLine, EndColumn;
+if (RangeSplit.second.empty()) {
+  EndLine = Begin.Line;
+  EndColumn = Begin.Column;
+} else {
+  std::pair Split = RangeSplit.second.rsplit(':');
+  if (Split.first.getAsInteger(10, EndLine) ||
+  Split.second.getAsInteger(10, EndColumn))
+return None;
+}
+return ParsedSourceRange{std::move(Begin.FileName),
+ {Begin.Line, Begin.Column},
+ {EndLine, EndColumn}};
+  }
+};
 }
 
 namespace llvm {
Index: cfe/trunk/test/Refactor/tool-selection-option.c
===
--- cfe/trunk/test/Refactor/tool-selection-option.c
+++ cfe/trunk/test/Refactor/tool-selection-option.c
@@ -0,0 +1,15 @@
+// RUN: rm -f %t.cp.c
+// RUN: cp %s %t.cp.c
+// RUN: clang-refactor local-rename -selection=%t.cp.c:6:5 -new-name=test -v %t.cp.c -- | FileCheck --check-prefix=CHECK1 %s
+// RUN: clang-refactor local-rename -selection=%t.cp.c:6:5-6:9 -new-name=test -v %t.cp.c -- | FileCheck --check-prefix=CHECK2 %s
+
+int test;
+
+// CHECK1: invoking action 'local-rename':
+// CHECK1-NEXT: -selection={{.*}}.cp.c:6:5 -> {{.*}}.cp.c:6:5
+
+// CHECK2: invoking action 'local-rename':
+// CHECK2-NEXT: -selection={{.*}}.cp.c:6:5 -> {{.*}}.cp.c:6:9
+
+// RUN: not clang-refactor local-rename -selection=%s:6:5 -new-name=test -v %t.cp.c -- 2>&1 | FileCheck --check-prefix=CHECK-FILE-ERR %s
+// CHECK-FILE-ERR: given file is not in the target TU
Index: cfe/trunk/test/Refactor/tool-apply-replacements.cpp
===
--- cfe/trunk/test/Refactor/tool-apply-replacements.cpp
+++ cfe/trunk/test/Refactor/tool-apply-replacements.cpp
@@ -0,0 +1,11 @@
+// RUN: rm -f %t.cp.cpp
+// RUN: cp %s %t.cp.cpp
+// RUN: clang-refactor local-rename -selection=%t.cp.cpp:9:7 -new-name=test %t.cp.cpp --
+// RUN: grep -v CHECK %t.cp.cpp | FileCheck %t.cp.cpp
+// RUN: cp %s %t.cp.cpp
+// RUN: clang-refactor local-rename -selection=%t.cp.cpp:9:7-9:15 -new-name=test %t.cp.cpp --
+// RUN: grep -v CHECK %t.cp.cpp | FileCheck %t.cp.cpp
+
+class RenameMe {
+// CHECK: class test {
+};
Index: cfe/trunk/tools/clang-refactor/ClangRefactor.cpp
===
--- cfe/trunk/tools/clang-refactor/ClangRefactor.cpp
+++ cfe/trunk/tools/clang-refactor/ClangRefactor.cpp
@@ -14,6 +14,7 @@
 //===--===//
 
 #include "TestSupport.h"
+#include "clang/Frontend/CommandLineSourceLoc.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Refactoring.h"
@@ -54,7 +55,7 @@
 
   /// Prints any additional state associated with the selection argument to
   /// the given output stream.
-  virtual void print(raw_ostream &OS) = 0;
+  virtual void print(raw_ostream &OS) {}
 
   /// Returns a replacement refactoring result consumer (if any) that should
   /// consume the results of a r

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

2017-10-16 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: include/clang/Tooling/CommonOptionsParser.h:116
+  bool HasError;
+  std::string ErrorMessage;
   std::unique_ptr Compilations;

Would it be better to have an `llvm::Error' instead of the two fields?



Comment at: include/clang/Tooling/Execution.h:47
+  virtual ~ToolResults() {}
+  virtual void addResult(llvm::StringRef Key, llvm::StringRef Value) = 0;
+  virtual std::vector> AllKVResults() = 0;

You don't need to use the `llvm::` prefix for `StringRef`.



Comment at: include/clang/Tooling/Execution.h:76
+private:
+  ToolResults *Results;
+};

Why not `unique_ptr`/`shared_ptr`? Who owns the results?



Comment at: include/clang/Tooling/Execution.h:134
+
+/// \brief A stand alone executor that runs FrontendActions on a given set of
+/// TUs in sequence.

Standalone is one word.



Comment at: include/clang/Tooling/Execution.h:136
+/// TUs in sequence.
+class StandaloneToolExecutor : public ToolExecutor {
+public:

Maybe this class and `InMemoryToolResults` should be in a separate header?


https://reviews.llvm.org/D34272



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

[PATCH] D38772: [refactor] allow the use of refactoring diagnostics

2017-10-16 Thread Alex Lorenz via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL315924: [refactor] allow the use of refactoring diagnostics 
(authored by arphaman).

Changed prior to commit:
  https://reviews.llvm.org/D38772?vs=118701&id=119185#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38772

Files:
  cfe/trunk/include/clang/Basic/AllDiagnostics.h
  cfe/trunk/include/clang/Basic/CMakeLists.txt
  cfe/trunk/include/clang/Basic/Diagnostic.td
  cfe/trunk/include/clang/Basic/DiagnosticIDs.h
  cfe/trunk/include/clang/Basic/DiagnosticRefactoringKinds.td
  
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringDiagnostic.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringRuleContext.h
  cfe/trunk/include/clang/module.modulemap
  cfe/trunk/lib/Basic/DiagnosticIDs.cpp
  cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  cfe/trunk/test/Refactor/LocalRename/NoSymbolSelectedError.cpp
  cfe/trunk/tools/clang-refactor/ClangRefactor.cpp
  cfe/trunk/tools/clang-refactor/TestSupport.cpp
  cfe/trunk/tools/clang-refactor/TestSupport.h
  cfe/trunk/tools/clang-refactor/ToolRefactoringResultConsumer.h
  cfe/trunk/tools/diagtool/DiagnosticNames.cpp
  cfe/trunk/unittests/Tooling/RefactoringActionRulesTest.cpp

Index: cfe/trunk/include/clang/Tooling/Refactoring/RefactoringDiagnostic.h
===
--- cfe/trunk/include/clang/Tooling/Refactoring/RefactoringDiagnostic.h
+++ cfe/trunk/include/clang/Tooling/Refactoring/RefactoringDiagnostic.h
@@ -0,0 +1,30 @@
+//===--- RefactoringDiagnostic.h - --*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLING_REFACTORING_REFACTORINGDIAGNOSTIC_H
+#define LLVM_CLANG_TOOLING_REFACTORING_REFACTORINGDIAGNOSTIC_H
+
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/PartialDiagnostic.h"
+
+namespace clang {
+namespace diag {
+enum {
+#define DIAG(ENUM, FLAGS, DEFAULT_MAPPING, DESC, GROUP, SFINAE, NOWERROR,  \
+ SHOWINSYSHEADER, CATEGORY)\
+  ENUM,
+#define REFACTORINGSTART
+#include "clang/Basic/DiagnosticRefactoringKinds.inc"
+#undef DIAG
+  NUM_BUILTIN_REFACTORING_DIAGNOSTICS
+};
+} // end namespace diag
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLING_REFACTORING_REFACTORINGDIAGNOSTIC_H
Index: cfe/trunk/include/clang/Tooling/Refactoring/RefactoringRuleContext.h
===
--- cfe/trunk/include/clang/Tooling/Refactoring/RefactoringRuleContext.h
+++ cfe/trunk/include/clang/Tooling/Refactoring/RefactoringRuleContext.h
@@ -10,6 +10,7 @@
 #ifndef LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_RULE_CONTEXT_H
 #define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_RULE_CONTEXT_H
 
+#include "clang/Basic/DiagnosticError.h"
 #include "clang/Basic/SourceManager.h"
 
 namespace clang {
@@ -50,6 +51,17 @@
 
   void setASTContext(ASTContext &Context) { AST = &Context; }
 
+  /// Creates an llvm::Error value that contains a diagnostic.
+  ///
+  /// The errors should not outlive the context.
+  llvm::Error createDiagnosticError(SourceLocation Loc, unsigned DiagID) {
+return DiagnosticError::create(Loc, PartialDiagnostic(DiagID, DiagStorage));
+  }
+
+  llvm::Error createDiagnosticError(unsigned DiagID) {
+return createDiagnosticError(SourceLocation(), DiagID);
+  }
+
 private:
   /// The source manager for the translation unit / file on which a refactoring
   /// action might operate on.
@@ -60,6 +72,8 @@
   /// An optional AST for the translation unit on which a refactoring action
   /// might operate on.
   ASTContext *AST = nullptr;
+  /// The allocator for diagnostics.
+  PartialDiagnostic::StorageAllocator DiagStorage;
 };
 
 } // end namespace tooling
Index: cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
===
--- cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
+++ cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
@@ -11,6 +11,7 @@
 #define LLVM_CLANG_TOOLING_REFACTOR_REFACTORING_ACTION_RULE_REQUIREMENTS_H
 
 #include "clang/Basic/LLVM.h"
+#include "clang/Tooling/Refactoring/RefactoringDiagnostic.h"
 #include "clang/Tooling/Refactoring/RefactoringOption.h"
 #include "clang/Tooling/Refactoring/RefactoringRuleContext.h"
 #include "llvm/Support/Error.h"
@@ -47,10 +48,7 @@
   Expected evaluate(RefactoringRuleContext &Context) const {
 if (Context.getSelectionRange().isValid())
   return Context.getSelectionRange();
-// FIXME: Use a diagnostic.
-return llvm::m

[PATCH] D38835: [refactor] selection: new CodeRangeASTSelection represents a set of selected consecutive statements

2017-10-16 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: include/clang/Tooling/Refactoring/ASTSelection.h:74
+/// An AST selection value that corresponds to a selection of a set of
+/// statements that belong to one body of code (like one function).
+///

hokein wrote:
> I see all your tests are for function body,  does it support other body, i.e. 
> "global namespace", "compound statement"? 
> 
> if yes, how about adding more test cases to cover it.
> 
> ```
> // variable in global namespace
> int a; // select int.
> ``` 
> 
> ```
> void f() {
>{
>int a;  // select a.
>}
> }
> ```
Yes, I added a couple that you suggested.



Comment at: unittests/Tooling/ASTSelectionTest.cpp:688
+  Source, {2, 2}, None,
+  [](SourceRange SelectionRange, Optional Node) {
+EXPECT_TRUE(Node);

hokein wrote:
> I'm a bit confused here, if the selection range is none/empty,  shouldn't 
> `SelectedASTNode` be empty?
> 
> If this behavior is intended, I'd suggest documenting it in 
> `findSelectedASTNodesWithRange`.
No, the AST selection will work even with a cursor location, hence it won't be 
empty.

However, the CodeRangeASTSelection requires a non-empty selection range, so it 
won't work with just cursors.


Repository:
  rL LLVM

https://reviews.llvm.org/D38835



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

[PATCH] D38835: [refactor] selection: new CodeRangeASTSelection represents a set of selected consecutive statements

2017-10-16 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 119197.
arphaman marked 6 inline comments as done.
arphaman added a comment.

Address review comments


Repository:
  rL LLVM

https://reviews.llvm.org/D38835

Files:
  include/clang/Tooling/Refactoring/ASTSelection.h
  lib/Tooling/Refactoring/ASTSelection.cpp
  unittests/Tooling/ASTSelectionTest.cpp

Index: unittests/Tooling/ASTSelectionTest.cpp
===
--- unittests/Tooling/ASTSelectionTest.cpp
+++ unittests/Tooling/ASTSelectionTest.cpp
@@ -29,12 +29,16 @@
 class SelectionFinderVisitor : public TestVisitor {
   FileLocation Location;
   Optional SelectionRange;
-  llvm::function_ref)> Consumer;
+  llvm::function_ref)>
+  Consumer;
 
 public:
-  SelectionFinderVisitor(
-  FileLocation Location, Optional SelectionRange,
-  llvm::function_ref)> Consumer)
+  SelectionFinderVisitor(FileLocation Location,
+ Optional SelectionRange,
+ llvm::function_ref)>
+ Consumer)
   : Location(Location), SelectionRange(SelectionRange), Consumer(Consumer) {
   }
 
@@ -50,20 +54,35 @@
   SourceLocation Loc = Location.translate(SM);
   SelRange = SourceRange(Loc, Loc);
 }
-Consumer(findSelectedASTNodes(Context, SelRange));
+Consumer(SelRange, findSelectedASTNodes(Context, SelRange));
 return false;
   }
 };
 
-void findSelectedASTNodes(
+void findSelectedASTNodesWithRange(
 StringRef Source, FileLocation Location, Optional SelectionRange,
-llvm::function_ref)> Consumer,
+llvm::function_ref)>
+Consumer,
 SelectionFinderVisitor::Language Language =
 SelectionFinderVisitor::Lang_CXX11) {
   SelectionFinderVisitor Visitor(Location, SelectionRange, Consumer);
   EXPECT_TRUE(Visitor.runOver(Source, Language));
 }
 
+void findSelectedASTNodes(
+StringRef Source, FileLocation Location, Optional SelectionRange,
+llvm::function_ref)> Consumer,
+SelectionFinderVisitor::Language Language =
+SelectionFinderVisitor::Lang_CXX11) {
+  findSelectedASTNodesWithRange(
+  Source, Location, SelectionRange,
+  [&](SourceRange, Optional Selection) {
+Consumer(std::move(Selection));
+  },
+  Language);
+}
+
 void checkNodeImpl(bool IsTypeMatched, const SelectedASTNode &Node,
SourceSelectionKind SelectionKind, unsigned NumChildren) {
   ASSERT_TRUE(IsTypeMatched);
@@ -649,4 +668,171 @@
   SelectionFinderVisitor::Lang_OBJC);
 }
 
+TEST(ASTSelectionFinder, SimpleCodeRangeASTSelection) {
+  StringRef Source = R"(void f(int x, int y) {
+  int z = x;
+  f(2, 3);
+  if (x == 0) {
+return;
+  }
+  x = 1;
+  return;
+}
+void f2() {
+  int m = 0;
+}
+)";
+  // No selection range.
+  findSelectedASTNodesWithRange(
+  Source, {2, 2}, None,
+  [](SourceRange SelectionRange, Optional Node) {
+EXPECT_TRUE(Node);
+Optional SelectedCode =
+CodeRangeASTSelection::create(SelectionRange, std::move(*Node));
+EXPECT_FALSE(SelectedCode);
+  });
+  findSelectedASTNodesWithRange(
+  Source, {2, 2}, FileRange{{2, 2}, {2, 2}},
+  [](SourceRange SelectionRange, Optional Node) {
+EXPECT_TRUE(Node);
+Optional SelectedCode =
+CodeRangeASTSelection::create(SelectionRange, std::move(*Node));
+EXPECT_FALSE(SelectedCode);
+  });
+  // Range that spans multiple functions is an invalid code range.
+  findSelectedASTNodesWithRange(
+  Source, {2, 2}, FileRange{{7, 2}, {12, 1}},
+  [](SourceRange SelectionRange, Optional Node) {
+EXPECT_TRUE(Node);
+Optional SelectedCode =
+CodeRangeASTSelection::create(SelectionRange, std::move(*Node));
+EXPECT_FALSE(SelectedCode);
+  });
+  // Just 'z = x;':
+  findSelectedASTNodesWithRange(
+  Source, {2, 2}, FileRange{{2, 2}, {2, 13}},
+  [](SourceRange SelectionRange, Optional Node) {
+EXPECT_TRUE(Node);
+Optional SelectedCode =
+CodeRangeASTSelection::create(SelectionRange, std::move(*Node));
+EXPECT_TRUE(SelectedCode);
+EXPECT_EQ(SelectedCode->size(), 1u);
+EXPECT_TRUE(isa((*SelectedCode)[0]));
+ArrayRef Parents =
+SelectedCode->getParents();
+EXPECT_EQ(Parents.size(), 3u);
+EXPECT_TRUE(
+isa(Parents[0].get().Node.get()));
+// Function 'f' definition.
+EXPECT_TRUE(isa(Parents[1].get().Node.get()));
+// Function body of function 'F'.
+EXPECT_TRUE(isa(Parents[2].get().Node.get()));
+  });
+  // From 'f(2,3)' until just before 'x = 1;':
+  findSelectedASTNodesWithRange(
+  Source, {3, 2}, FileRange{{3, 2}, {7, 1}},
+  [](SourceRange SelectionRange, Optional Node) {
+EXPECT_TRUE(Node);
+Optional SelectedCode =
+CodeRangeASTSelection::create(SelectionRange, std::move(*Node));
+EXPECT_TRUE(SelectedCode);
+EX

[PATCH] D38982: [refactor] Initial outline of implementation of "extract function" refactoring

2017-10-16 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.
Herald added a subscriber: mgorny.

This patch adds an initial, skeleton outline of the "extract function" 
refactoring.
The extracted function doesn't capture variables / rewrite code yet, it just 
basically does a simple copy-paste.
The following initiation rules are specified:

- extraction can only be done for executable code in a function/method/block. 
This means that you can't extract a global variable initialize into a function 
right now (should this be allowed though?).
- simple literals and references are not extractable.

This patch also adds support for full source ranges to clang-refactor's test 
mode.


Repository:
  rL LLVM

https://reviews.llvm.org/D38982

Files:
  include/clang/Basic/DiagnosticRefactoringKinds.td
  include/clang/Tooling/Refactoring/ASTSelection.h
  include/clang/Tooling/Refactoring/RefactoringActionRegistry.def
  include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
  include/clang/Tooling/Refactoring/RefactoringRuleContext.h
  lib/Tooling/Refactoring/ASTSelection.cpp
  lib/Tooling/Refactoring/ASTSelectionRequirements.cpp
  lib/Tooling/Refactoring/CMakeLists.txt
  lib/Tooling/Refactoring/Extract.cpp
  test/Refactor/Extract/ExtractExprIntoFunction.cpp
  test/Refactor/LocalRename/Field.cpp
  test/Refactor/tool-test-support.c
  tools/clang-refactor/TestSupport.cpp

Index: tools/clang-refactor/TestSupport.cpp
===
--- tools/clang-refactor/TestSupport.cpp
+++ tools/clang-refactor/TestSupport.cpp
@@ -20,6 +20,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/LineIterator.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/raw_ostream.h"
@@ -101,7 +102,14 @@
   llvm::errs() << toString(Result.takeError());
   return true;
 }
-OS << *Result;
+auto Buf = llvm::MemoryBuffer::getMemBuffer(Result->c_str());
+for (llvm::line_iterator It(*Buf, /*SkipBlanks=*/false); !It.is_at_end();
+ ++It) {
+  // Drop the 'CHECK' lines alltogether to avoid erroneous matches.
+  if (It->contains("CHECK"))
+continue;
+  OS << *It << "\n";
+}
   }
   return false;
 }
@@ -241,9 +249,9 @@
 // Dump the results:
 const auto &TestGroup = TestRanges.GroupedRanges[Group.index()];
 if (!CanonicalResult) {
-  llvm::errs() << TestGroup.Ranges.size() << " '" << TestGroup.Name
+  llvm::outs() << TestGroup.Ranges.size() << " '" << TestGroup.Name
<< "' results:\n";
-  llvm::errs() << *CanonicalErrorMessage << "\n";
+  llvm::outs() << *CanonicalErrorMessage << "\n";
 } else {
   llvm::outs() << TestGroup.Ranges.size() << " '" << TestGroup.Name
<< "' results:\n";
@@ -271,6 +279,25 @@
  (NewlinePos == StringRef::npos ? ColumnOffset : (unsigned)NewlinePos);
 }
 
+static unsigned addEndLineOffsetAndEndColumn(StringRef Source, unsigned Offset,
+ unsigned LineNumberOffset,
+ unsigned Column) {
+  StringRef Line = Source.drop_front(Offset);
+  unsigned LineOffset = 0;
+  for (; LineNumberOffset != 0; --LineNumberOffset) {
+size_t NewlinePos = Line.find_first_of("\r\n");
+// Line offset goes out of bounds.
+if (NewlinePos == StringRef::npos)
+  break;
+LineOffset += NewlinePos + 1;
+Line = Line.drop_front(NewlinePos + 1);
+  }
+  // Source now points to the line at +lineOffset;
+  size_t LineStart = Source.find_last_of("\r\n", /*From=*/Offset + LineOffset);
+  return addColumnOffset(
+  Source, LineStart == StringRef::npos ? 0 : LineStart + 1, Column - 1);
+}
+
 Optional
 findTestSelectionRanges(StringRef Filename) {
   ErrorOr> ErrOrFile =
@@ -282,11 +309,11 @@
   }
   StringRef Source = ErrOrFile.get()->getBuffer();
 
-  // FIXME (Alex L): 3rd capture groups for +line:column.
   // See the doc comment for this function for the explanation of this
   // syntax.
   static Regex RangeRegex("range[[:blank:]]*([[:alpha:]_]*)?[[:blank:]]*=[[:"
-  "blank:]]*(\\+[[:digit:]]+)?");
+  "blank:]]*(\\+[[:digit:]]+)?[[:blank:]]*(->[[:blank:]"
+  "]*[\\+\\:[:digit:]]+)?");
 
   std::map> GroupedRanges;
 
@@ -304,18 +331,22 @@
 StringRef Comment =
 Source.substr(Tok.getLocation().getRawEncoding(), Tok.getLength());
 SmallVector Matches;
-// Allow CHECK: comments to contain range= commands.
-if (!RangeRegex.match(Comment, &Matches) || Comment.contains("CHECK")) {
-  // Try to detect mistyped 'range:' comments to ensure tests don't miss
-  // anything.
+// Try to detect mistyped 'range:' comments to ensure tests don't miss
+// anything.
+auto DetectMistypedCommand = [&]() ->

[PATCH] D38985: [refactor] Add support for editor commands that connect IDEs/editors to the refactoring actions

2017-10-16 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.
Herald added subscribers: ilya-biryukov, mgorny.

This patch adds support for editor commands that allow refactoring to be used 
in editor clients like libclang or clangd.
An editor command can be bound to an refactoring action rule. Once it is bound, 
it's available in editors that use the supported editor clients.

I plan on sending out a follow-up patch for clangd support tomorrow.


Repository:
  rL LLVM

https://reviews.llvm.org/D38985

Files:
  include/clang/Tooling/Refactoring/EditorCommandRegistry.def
  include/clang/Tooling/Refactoring/EditorCommands.h
  include/clang/Tooling/Refactoring/RefactoringActionRule.h
  include/clang/module.modulemap
  lib/Tooling/Refactoring/CMakeLists.txt
  lib/Tooling/Refactoring/EditorCommand.cpp
  lib/Tooling/Refactoring/Extract.cpp
  unittests/Tooling/RefactoringActionRulesTest.cpp

Index: unittests/Tooling/RefactoringActionRulesTest.cpp
===
--- unittests/Tooling/RefactoringActionRulesTest.cpp
+++ unittests/Tooling/RefactoringActionRulesTest.cpp
@@ -10,6 +10,8 @@
 #include "ReplacementTest.h"
 #include "RewriterTestContext.h"
 #include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Refactoring/EditorCommands.h"
+#include "clang/Tooling/Refactoring/RefactoringAction.h"
 #include "clang/Tooling/Refactoring/RefactoringActionRules.h"
 #include "clang/Tooling/Refactoring/RefactoringDiagnostic.h"
 #include "clang/Tooling/Refactoring/Rename/SymbolName.h"
@@ -219,4 +221,23 @@
 SourceRange(Cursor, Cursor.getLocWithOffset(strlen("test";
 }
 
+TEST_F(RefactoringActionRulesTest, EditorCommandBinding) {
+  std::vector> Actions =
+  createRefactoringActions();
+  for (auto &Action : Actions) {
+if (Action->getCommand() == "extract") {
+  std::vector> Rules =
+  Action->createActiveActionRules();
+  ASSERT_FALSE(Rules.empty());
+  const EditorCommand *Cmd = Rules[0]->getEditorCommand();
+  ASSERT_TRUE(Cmd);
+  EXPECT_EQ(Cmd->getName(), "ExtractFunction");
+  EXPECT_EQ(Cmd->getTitle(), "Extract Function");
+  return;
+}
+  }
+  // Never found 'extract'?
+  ASSERT_TRUE(false);
+}
+
 } // end anonymous namespace
Index: lib/Tooling/Refactoring/Extract.cpp
===
--- lib/Tooling/Refactoring/Extract.cpp
+++ lib/Tooling/Refactoring/Extract.cpp
@@ -16,6 +16,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Expr.h"
 #include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/Refactoring/EditorCommands.h"
 #include "clang/Tooling/Refactoring/RefactoringAction.h"
 #include "clang/Tooling/Refactoring/RefactoringActionRules.h"
 #include "clang/Tooling/Refactoring/RefactoringOptions.h"
@@ -214,9 +215,10 @@
   /// action.
   RefactoringActionRules createActionRules() const override {
 RefactoringActionRules Rules;
-Rules.push_back(createRefactoringActionRule(
-ExtractableCodeSelectionRequirement(),
-OptionRequirement()));
+Rules.push_back(EditorCommand::ExtractFunction().bind(
+createRefactoringActionRule(
+ExtractableCodeSelectionRequirement(),
+OptionRequirement(;
 return Rules;
   }
 };
Index: lib/Tooling/Refactoring/EditorCommand.cpp
===
--- /dev/null
+++ lib/Tooling/Refactoring/EditorCommand.cpp
@@ -0,0 +1,62 @@
+//===--- EditorCommand.cpp - refactoring editor commands --===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/EditorCommands.h"
+#include "clang/Tooling/Refactoring/RefactoringActionRule.h"
+
+namespace clang {
+namespace tooling {
+
+#define REFACTORING_EDITOR_COMMAND(Name, Title)\
+  EditorCommand &EditorCommand::Name() {   \
+static std::unique_ptr Command( \
+new EditorCommand(#Name, Title));  \
+return *Command;   \
+  }
+#include "clang/Tooling/Refactoring/EditorCommandRegistry.def"
+
+class BoundEditorRefactoringActionRule : public RefactoringActionRule {
+public:
+  BoundEditorRefactoringActionRule(std::unique_ptr Rule,
+   const EditorCommand &Command)
+  : Rule(std::move(Rule)), Command(Command) {}
+
+  void invoke(RefactoringResultConsumer &Consumer,
+  RefactoringRuleContext &Context) override {
+Rule->invoke(Consumer, Context);
+  }
+
+  bool hasSelectionRequirement() override {
+return Rule->hasSelectionRequirement();
+  }
+
+  void visitRefactoringOptions(RefactoringOp

[PATCH] D39027: [docs][refactor] Add a new tutorial that talks about how one can implement refactoring actions

2017-10-17 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.

This patch adds a new tutorial to Clang's talks that walks through an example 
of a refactoring action and how it can be implemented in Clang.


Repository:
  rL LLVM

https://reviews.llvm.org/D39027

Files:
  docs/RefactoringActionTutorial.rst
  docs/RefactoringEngine.rst
  docs/index.rst

Index: docs/index.rst
===
--- docs/index.rst
+++ docs/index.rst
@@ -61,6 +61,7 @@
HowToSetupToolingForLLVM
JSONCompilationDatabase
RefactoringEngine
+   RefactoringActionTutorial
 
 Using Clang Tools
 =
Index: docs/RefactoringEngine.rst
===
--- docs/RefactoringEngine.rst
+++ docs/RefactoringEngine.rst
@@ -18,7 +18,10 @@
 to the Clang AST ` if you want to learn more
 about how the AST is structured.
 
-..  FIXME: create new refactoring action tutorial and link to the tutorial
+You can also take a look a the
+:doc:`refactoring action tutorial ` to see how
+you can use the components that are described in this document to implement
+actual refactoring actions.
 
 Introduction
 
Index: docs/RefactoringActionTutorial.rst
===
--- /dev/null
+++ docs/RefactoringActionTutorial.rst
@@ -0,0 +1,382 @@
+=
+Tutorial for building refactoring actions
+=
+
+.. warning::
+
+  This tutorial talks about a work-in-progress library in Clang.
+  Some of the described features might not be available yet in trunk, but should
+  be there in the near future.
+
+This document is intended to show how to build refactoring actions that
+perform source-to-source transformations and integrate with editors that
+use ``libclang/clangd`` and the ``clang-refactor`` command-line refactoring
+tool. The document uses Clang's `refactoring engine `_
+and the `LibTooling `_ library.
+
+In order to work on the compiler, you need some basic knowledge of the
+abstract syntax tree (AST). To this end, the reader is encouraged to
+skim the :doc:`Introduction to the Clang
+AST `.
+
+How to obtain & build Clang
+===
+
+Before working on the refactoring action, you'll need to download and build
+LLVM and Clang. The
+:doc:`AST matchers tutorial`
+provides a section that describes how to obtain and build LLVM and Clang.
+
+Implementing a local source transformation
+==
+
+This tutorial walks through an implementation of the
+"flatten nested if statements" refactoring operation. This action can take a
+selected ``if`` statement that's nested in another ``if`` and merge the two
+into ``if``s one just one ``if``. For example, the following code:
+
+.. code-block:: c++
+
+  int computeArrayOffset(int x, int y) {
+if (x >= 0) {
+  if (y >= 0) {
+return x * 4 + y;
+  }
+}
+throw std::runtime_error("negative indices");
+  }
+
+
+becomes:
+
+.. code-block:: c++
+
+  int computeArrayOffset(int x, int y) {
+if (x >= 0 && y >= 0)
+  return x * 4 + y;
+throw std::runtime_error("negative indices");
+  }
+
+after this refactoring.
+
+This refactoring is a local source transformation, since it only requires one
+translation unit in order to perform the refactoring.
+
+Before diving into a potential implementation of a refactoring, let's try
+to break it down. This operation is a "flatten" operation, which in theory
+could be applied to other types of directives, like the ``switch`` statements
+or maybe even loops. We can think of the particular flavor of the refactoring
+that merges the ``if`` statements as just one refactoring operation that's
+contained in the "flatten" refactoring action. The refactoring library
+encourages you to think in terms like these. In particular, it encourages
+developers to decompose refactorings into a single action that contains one or
+more operation that either produce similar results or have different refactoring
+modes of operation. The different operations should still be identifiable
+by just one name, like "flatten". For the rest of the tutorial I will talk
+about the decomposed refactoring using terms like the "flatten" refactoring
+action and the "flatten nested if statements" refactoring operation.
+
+Now that we've established and decomposed the refactoring action, let's go
+ahead and look at how it can be implemented.
+
+Step 1: Create the "flatten nested if statements" refactoring operation
+---
+
+Let's start our implementation with the refactoring operation class that will
+be responsible for making the source changes.
+
+We can start by adding an implementation file in ``lib/Tooling/Refactoring``.
+Let's call it something like ``FlattenIfStatements.cpp``. Don't forget to add
+it to

[PATCH] D39050: Add index-while-building support to Clang

2017-10-18 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

I think this patch should be split into a number of smaller patches to help the 
review process.

Things like `tools/IndexStore`, `DirectoryWatcher` and other components that 
are not directly needed right now should definitely be in their own patches.
It would be nice to find some way to split the implementation into multiple 
patches as well.


https://reviews.llvm.org/D39050



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

[PATCH] D39055: [refactor] Add an editor client that is used in clangd

2017-10-18 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.
Herald added subscribers: mgorny, klimek.

This patch adds an editor client that can do things like:

- find a list of available refactoring editor commands in a particular range.
- perform a particular refactoring editor command in a particular range.

This editor client is used in clangd's support for refactoring commands (See 
dependent revision).

(I've ran out of time to add a unit test before devmeeting, so for now this is 
tested in clangd, but I'll add a unit test when I update the patch)


Repository:
  rL LLVM

https://reviews.llvm.org/D39055

Files:
  include/clang/Tooling/Refactoring/EditorClient.h
  include/clang/Tooling/Refactoring/RefactoringActionRule.h
  include/clang/Tooling/Refactoring/RefactoringActionRules.h
  include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
  lib/Tooling/Refactoring/CMakeLists.txt
  lib/Tooling/Refactoring/EditorClient.cpp
  lib/Tooling/Refactoring/EditorCommand.cpp

Index: lib/Tooling/Refactoring/EditorCommand.cpp
===
--- lib/Tooling/Refactoring/EditorCommand.cpp
+++ lib/Tooling/Refactoring/EditorCommand.cpp
@@ -28,8 +28,9 @@
   : Rule(std::move(Rule)), Command(Command) {}
 
   void invoke(RefactoringResultConsumer &Consumer,
-  RefactoringRuleContext &Context) override {
-Rule->invoke(Consumer, Context);
+  RefactoringRuleContext &Context,
+  RefactoringStage StopAfter) override {
+Rule->invoke(Consumer, Context, StopAfter);
   }
 
   bool hasSelectionRequirement() override {
Index: lib/Tooling/Refactoring/EditorClient.cpp
===
--- /dev/null
+++ lib/Tooling/Refactoring/EditorClient.cpp
@@ -0,0 +1,117 @@
+//===--- EditorClient.cpp - refactoring editor client -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/EditorClient.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/Tooling/Refactoring/EditorCommands.h"
+#include "clang/Tooling/Refactoring/RefactoringAction.h"
+#include "clang/Tooling/Refactoring/RefactoringActionRule.h"
+#include "llvm/ADT/STLExtras.h"
+
+namespace clang {
+namespace tooling {
+
+RefactoringEditorClient::Refactoring::~Refactoring() {}
+
+RefactoringEditorClient::RefactoringEditorClient() {
+  std::vector> Actions =
+  createRefactoringActions();
+
+  // Create subcommands and command-line options.
+  for (auto &Action : Actions) {
+RefactoringActionRules Rules = Action->createActiveActionRules();
+// Filter out refactoring rules without an source selection requirement and
+// without an editor command.
+Rules.resize(
+llvm::remove_if(Rules,
+[](const std::unique_ptr &Rule) {
+  return !Rule->hasSelectionRequirement() ||
+ !Rule->getEditorCommand();
+}) -
+Rules.begin());
+// FIXME (Alex L): Filter out rules with required options that can't be
+// fulfilled by editor clients (Also potentially simplify when selection
+// gets treated just like another option).
+if (Rules.empty())
+  continue;
+for (const auto &Rule : Rules)
+  EditorCommandsToRule.insert(
+  std::make_pair(Rule->getEditorCommand()->getName(), Rule.get()));
+RefactoringActions.push_back({std::move(Action), std::move(Rules)});
+  }
+}
+
+std::vector
+RefactoringEditorClient::getAvailableRefactorings(ASTContext &Context,
+  SourceRange SelectionRange) {
+  std::vector Commands;
+  RefactoringRuleContext RuleContext(Context.getSourceManager());
+  RuleContext.setSelectionRange(SelectionRange);
+  RuleContext.setASTContext(Context);
+
+  class ErrorChecker final : public RefactoringResultConsumer {
+  public:
+bool InitiationSucceeded = true;
+
+void handleError(llvm::Error Err) override {
+  llvm::consumeError(std::move(Err));
+  InitiationSucceeded = false;
+}
+  };
+
+  // Figure out which refactorings are available by running the initiation
+  // stage only.
+  for (const auto &Action : RefactoringActions) {
+for (const auto &Rule : Action.ActionRules) {
+  ErrorChecker Consumer;
+  Rule->invoke(Consumer, RuleContext,
+   /*StopAfter=*/RefactoringStage::Initiation);
+  if (Consumer.InitiationSucceeded)
+Commands.push_back(Rule->getEditorCommand());
+}
+  }
+  return Commands;
+}
+
+Expected RefactoringEditorClient::performRefactoring(
+ASTContext &Context, StringRef CommandName, SourceRange SelectionRange) {
+  auto It = EditorCommandsToRule.find(CommandName);
+  if (It == EditorCommandsTo

[PATCH] D39057: [clangd][WIP] Integrate the refactoring actions into clangd

2017-10-18 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman created this revision.
arphaman added a project: clang-tools-extra.
Herald added a subscriber: mgorny.

This WIP patch provides a sample implementation of an integration of Clang's 
refactoring actions from libToolingRefactor into clangd.

In terms of protocol support, the patch adds:

- Support for the `Command` & `CommandArgument` structures.
- Support for the `workspace/executeCommand` command.

Note that the rename is not supported as an editor command since LSP has 
another entry in the protocol for it.

The integration with the refactoring library is done through the 
`RefactoringEditorClient` class that's implemented in a parent revision. Right 
now the test checks that the initial version of the "extract function" 
refactoring can be performed.


Repository:
  rL LLVM

https://reviews.llvm.org/D39057

Files:
  clangd/CMakeLists.txt
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  clangd/ProtocolHandlers.cpp
  clangd/ProtocolHandlers.h
  test/clangd/refactoring.test

Index: test/clangd/refactoring.test
===
--- /dev/null
+++ test/clangd/refactoring.test
@@ -0,0 +1,28 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+#
+Content-Length: 181
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///foo.c","languageId":"c","version":1,"text":"int main(int i, char **a) { if (i == 2) {}}"}}}
+#
+# CHECK: {"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///foo.c","diagnostics":[]}}
+#
+Content-Length: 214
+
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"file:///foo.c"},"range":{"start":{"line":0,"character":32},"end":{"line":0,"character":38}},"context":{"diagnostics":[]}}}
+#
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"title":"Extract Function", "command": "clangd.refactor.ExtractFunction", "arguments": [{"doc":{"uri":"file:///foo.c"}},{"selection":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 38}}}, ]}]}
+#
+Content-Length: 243
+
+{"jsonrpc":"2.0","id":2,"method":"workspace/executeCommand","params":{"command":"clangd.refactor.ExtractFunction","arguments": [{"doc":{"uri":"file:///foo.c"}}, {"range":{"start":{"line":0,"character":32},"end":{"line":0,"character":38}}}]}}
+#
+# CHECK: {"jsonrpc":"2.0","id":2,"result":[{"range": {"start": {"line": 0, "character": 0}, "end": {"line": 0, "character": 0}}, "newText": "static int extracted() {\nreturn i == 2;\n}\n\n"},{"range": {"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 38}}, "newText": "extracted()"}]}
+#
+Content-Length: 44
+
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
Index: clangd/ProtocolHandlers.h
===
--- clangd/ProtocolHandlers.h
+++ clangd/ProtocolHandlers.h
@@ -51,6 +51,8 @@
   virtual void onGoToDefinition(Ctx C, TextDocumentPositionParams &Params) = 0;
   virtual void onSwitchSourceHeader(Ctx C, TextDocumentIdentifier &Params) = 0;
   virtual void onFileEvent(Ctx C, DidChangeWatchedFilesParams &Params) = 0;
+  virtual void onWorkspaceExecuteCommand(Ctx C,
+ ExecuteCommandParams &Params) = 0;
 };
 
 void registerCallbackHandlers(JSONRPCDispatcher &Dispatcher, JSONOutput &Out,
Index: clangd/ProtocolHandlers.cpp
===
--- clangd/ProtocolHandlers.cpp
+++ clangd/ProtocolHandlers.cpp
@@ -67,4 +67,6 @@
   Register("textDocument/switchSourceHeader",
&ProtocolCallbacks::onSwitchSourceHeader);
   Register("workspace/didChangeWatchedFiles", &ProtocolCallbacks::onFileEvent);
+  Register("workspace/executeCommand",
+   &ProtocolCallbacks::onWorkspaceExecuteCommand);
 }
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -530,6 +530,72 @@
   static std::string unparse(const SignatureHelp &);
 };
 
+/// Represents an editor command argument.
+struct CommandArgument {
+  union {
+llvm::AlignedCharArrayUnion SelectionRange;
+llvm::AlignedCharArrayUnion DocumentID;
+  };
+  enum Kind { SelectionRangeKind, TextDocumentIdentifierKind };
+  Kind ArgumentKind;
+
+  CommandArgument() {}
+
+  static CommandArgument makeSelectionRange(Range R) {
+CommandArgument Result;
+*reinterpret_cast(Result.SelectionRange.buffer) = R;
+Result.ArgumentKind = SelectionRangeKind;
+return Result;
+  }
+
+  const Range &getSelectionRange() const {
+assert(ArgumentKind == SelectionRangeKind);

[PATCH] D39057: [clangd][WIP] Integrate the refactoring actions into clangd

2017-10-18 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

I think that the clangd editor plugin will have to be modified as well, but I 
haven't looked into that yet.


Repository:
  rL LLVM

https://reviews.llvm.org/D39057



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

[PATCH] D38985: [refactor] Add support for editor commands that connect IDEs/editors to the refactoring actions

2017-10-18 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: include/clang/Tooling/Refactoring/RefactoringActionRule.h:58
+  /// Returns the editor command that's was bound to this rule.
+  virtual const EditorCommand *getEditorCommand() { return nullptr; }
 };

ioeric wrote:
> I'm still not quite sure about the intention of `EditorCommand` (is it 
> supposed to be used as a mapping from name to rule, or the other way 
> around?), but I'm a bit concerned about mixing editor logic with the 
> refactoring rules this way. Also to enable a editor command, it seems to me 
> that we would need to touch both the registry and the rule creation code, 
> which seems a bit cumbersome.
> 
> I think we should either 1) separate out the command logic cleanly without 
> touching action/rule interfaces in the refactoring engine or 2) simply bake 
> the logic into the refactoring engine.
> 
> It is unclear to me if 1) is possible, but for 2), I think we could introduce 
> a `RefactoringEngine` class which carries all refactoring actions as well as 
> a map to serve the purpose of `EditorCommand`. And I think by doing this, we 
> could also make the interfaces of `RefactoringEngine` more explicit.
(Quick comment before the devmeeting:)
Mapping from name to rule.
You're right though, It might be better to explore an alternative solution, but 
I don't quite follow your proposal yet. I'll be in the tooling hacker's lab at 
the dev meeting today if you want to discuss this in person.


Repository:
  rL LLVM

https://reviews.llvm.org/D38985



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

[PATCH] D38835: [refactor] selection: new CodeRangeASTSelection represents a set of selected consecutive statements

2017-10-18 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: unittests/Tooling/ASTSelectionTest.cpp:688
+  Source, {2, 2}, None,
+  [](SourceRange SelectionRange, Optional Node) {
+EXPECT_TRUE(Node);

hokein wrote:
> arphaman wrote:
> > hokein wrote:
> > > I'm a bit confused here, if the selection range is none/empty,  shouldn't 
> > > `SelectedASTNode` be empty?
> > > 
> > > If this behavior is intended, I'd suggest documenting it in 
> > > `findSelectedASTNodesWithRange`.
> > No, the AST selection will work even with a cursor location, hence it won't 
> > be empty.
> > 
> > However, the CodeRangeASTSelection requires a non-empty selection range, so 
> > it won't work with just cursors.
> I see, thanks.
> 
> > the AST selection will work even with a cursor location, hence it won't be 
> > empty.
> 
> Is this documented somewhere in the code? I couldn't find any comments in the 
> definition of `findSelectedASTNodesWithRange` or `findSelectedASTNodes` in 
> this file.  Would be nice to document it although this is a test API, so that 
> it won't confuse future code readers.
Not quite, I'll add a comment in a follow-up commit.


Repository:
  rL LLVM

https://reviews.llvm.org/D38835



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

[PATCH] D38835: [refactor] selection: new CodeRangeASTSelection represents a set of selected consecutive statements

2017-10-18 Thread Alex Lorenz via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL316104: [refactor] selection: new CodeRangeASTSelection 
represents a set of selected (authored by arphaman).

Changed prior to commit:
  https://reviews.llvm.org/D38835?vs=119197&id=119514#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38835

Files:
  cfe/trunk/include/clang/Tooling/Refactoring/ASTSelection.h
  cfe/trunk/lib/Tooling/Refactoring/ASTSelection.cpp
  cfe/trunk/unittests/Tooling/ASTSelectionTest.cpp

Index: cfe/trunk/lib/Tooling/Refactoring/ASTSelection.cpp
===
--- cfe/trunk/lib/Tooling/Refactoring/ASTSelection.cpp
+++ cfe/trunk/lib/Tooling/Refactoring/ASTSelection.cpp
@@ -225,3 +225,108 @@
 }
 
 void SelectedASTNode::dump(llvm::raw_ostream &OS) const { ::dump(*this, OS); }
+
+/// Returns true if the given node has any direct children with the following
+/// selection kind.
+///
+/// Note: The direct children also include children of direct children with the
+/// "None" selection kind.
+static bool hasAnyDirectChildrenWithKind(const SelectedASTNode &Node,
+ SourceSelectionKind Kind) {
+  assert(Kind != SourceSelectionKind::None && "invalid predicate!");
+  for (const auto &Child : Node.Children) {
+if (Child.SelectionKind == Kind)
+  return true;
+if (Child.SelectionKind == SourceSelectionKind::None)
+  return hasAnyDirectChildrenWithKind(Child, Kind);
+  }
+  return false;
+}
+
+namespace {
+struct SelectedNodeWithParents {
+  SelectedNodeWithParents(SelectedNodeWithParents &&) = default;
+  SelectedNodeWithParents &operator=(SelectedNodeWithParents &&) = default;
+  SelectedASTNode::ReferenceType Node;
+  llvm::SmallVector Parents;
+};
+} // end anonymous namespace
+
+/// Finds the set of bottom-most selected AST nodes that are in the selection
+/// tree with the specified selection kind.
+///
+/// For example, given the following selection tree:
+///
+/// FunctionDecl "f" contains-selection
+///   CompoundStmt contains-selection [#1]
+/// CallExpr inside
+/// ImplicitCastExpr inside
+///   DeclRefExpr inside
+/// IntegerLiteral inside
+/// IntegerLiteral inside
+/// FunctionDecl "f2" contains-selection
+///   CompoundStmt contains-selection [#2]
+/// CallExpr inside
+/// ImplicitCastExpr inside
+///   DeclRefExpr inside
+/// IntegerLiteral inside
+/// IntegerLiteral inside
+///
+/// This function will find references to nodes #1 and #2 when searching for the
+/// \c ContainsSelection kind.
+static void findDeepestWithKind(
+const SelectedASTNode &ASTSelection,
+llvm::SmallVectorImpl &MatchingNodes,
+SourceSelectionKind Kind,
+llvm::SmallVectorImpl &ParentStack) {
+  if (!hasAnyDirectChildrenWithKind(ASTSelection, Kind)) {
+// This node is the bottom-most.
+MatchingNodes.push_back(SelectedNodeWithParents{
+std::cref(ASTSelection), {ParentStack.begin(), ParentStack.end()}});
+return;
+  }
+  // Search in the children.
+  ParentStack.push_back(std::cref(ASTSelection));
+  for (const auto &Child : ASTSelection.Children)
+findDeepestWithKind(Child, MatchingNodes, Kind, ParentStack);
+  ParentStack.pop_back();
+}
+
+static void findDeepestWithKind(
+const SelectedASTNode &ASTSelection,
+llvm::SmallVectorImpl &MatchingNodes,
+SourceSelectionKind Kind) {
+  llvm::SmallVector ParentStack;
+  findDeepestWithKind(ASTSelection, MatchingNodes, Kind, ParentStack);
+}
+
+Optional
+CodeRangeASTSelection::create(SourceRange SelectionRange,
+  const SelectedASTNode &ASTSelection) {
+  // Code range is selected when the selection range is not empty.
+  if (SelectionRange.getBegin() == SelectionRange.getEnd())
+return None;
+  llvm::SmallVector ContainSelection;
+  findDeepestWithKind(ASTSelection, ContainSelection,
+  SourceSelectionKind::ContainsSelection);
+  // We are looking for a selection in one body of code, so let's focus on
+  // one matching result.
+  if (ContainSelection.size() != 1)
+return None;
+  SelectedNodeWithParents &Selected = ContainSelection[0];
+  if (!Selected.Node.get().Node.get())
+return None;
+  const Stmt *CodeRangeStmt = Selected.Node.get().Node.get();
+  if (!isa(CodeRangeStmt)) {
+// FIXME (Alex L): Canonicalize.
+return CodeRangeASTSelection(Selected.Node, Selected.Parents,
+ /*AreChildrenSelected=*/false);
+  }
+  // FIXME (Alex L): Tweak selection rules for compound statements, see:
+  // https://github.com/apple/swift-clang/blob/swift-4.1-branch/lib/Tooling/
+  // Refactor/ASTSlice.cpp#L513
+  // The user selected multiple statements in a compound statement.
+  Selected.Parents.push_back(Selected.Node);
+  return CodeRangeASTSelection(Selected.Node, Selected.Parents,
+   /*AreChildrenSelected=*/true);
+}
Index: cfe/trunk/unittests/Tooling/AS

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

2017-10-18 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: include/clang/Tooling/StandaloneExecution.h:1
+//===--- Execution.h - Standalone clang action execution. -*- C++ 
---*-===//
+//

StandaloneExecution.h?


https://reviews.llvm.org/D34272



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

[PATCH] D39092: [clang-refactor] Add "-Inplace" option to the commandline tool.

2017-10-19 Thread Alex Lorenz via Phabricator via cfe-commits

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

LGTM


https://reviews.llvm.org/D39092



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

[PATCH] D38982: [refactor] Initial outline of implementation of "extract function" refactoring

2017-10-20 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 119708.
arphaman marked 4 inline comments as done.
arphaman added a comment.

Address review comments.


Repository:
  rL LLVM

https://reviews.llvm.org/D38982

Files:
  include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  include/clang/Tooling/Refactoring/RefactoringRuleContext.h
  lib/Tooling/Refactoring/ASTSelection.cpp
  lib/Tooling/Refactoring/ASTSelectionRequirements.cpp
  lib/Tooling/Refactoring/Extract.cpp
  test/Refactor/Extract/ExtractExprIntoFunction.cpp
  test/Refactor/LocalRename/Field.cpp
  test/Refactor/LocalRename/NoSymbolSelectedError.cpp
  tools/clang-refactor/TestSupport.cpp

Index: tools/clang-refactor/TestSupport.cpp
===
--- tools/clang-refactor/TestSupport.cpp
+++ tools/clang-refactor/TestSupport.cpp
@@ -102,14 +102,7 @@
   llvm::errs() << toString(Result.takeError());
   return true;
 }
-auto Buf = llvm::MemoryBuffer::getMemBuffer(Result->c_str());
-for (llvm::line_iterator It(*Buf, /*SkipBlanks=*/false); !It.is_at_end();
- ++It) {
-  // Drop the 'CHECK' lines alltogether to avoid erroneous matches.
-  if (It->contains("CHECK"))
-continue;
-  OS << *It << "\n";
-}
+OS << *Result;
   }
   return false;
 }
Index: test/Refactor/LocalRename/NoSymbolSelectedError.cpp
===
--- test/Refactor/LocalRename/NoSymbolSelectedError.cpp
+++ test/Refactor/LocalRename/NoSymbolSelectedError.cpp
@@ -1,5 +1,5 @@
-// RUN: not clang-refactor local-rename -selection=%s:4:1 -new-name=Bar %s -- 2>&1 | FileCheck %s
-// RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- 2>&1 | FileCheck --check-prefix=TESTCHECK %s
+// RUN: not clang-refactor local-rename -selection=%s:4:1 -new-name=Bar %s -- 2>&1 | grep -v CHECK | FileCheck %s
+// RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- 2>&1 | grep -v CHECK | FileCheck --check-prefix=TESTCHECK %s
 
 class Baz { // CHECK: [[@LINE]]:1: error: there is no symbol at the given location
 };
Index: test/Refactor/LocalRename/Field.cpp
===
--- test/Refactor/LocalRename/Field.cpp
+++ test/Refactor/LocalRename/Field.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- | FileCheck %s
+// RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- | grep -v CHECK | FileCheck %s
 
 class Baz {
   int /*range=*/Foo;
Index: test/Refactor/Extract/ExtractExprIntoFunction.cpp
===
--- test/Refactor/Extract/ExtractExprIntoFunction.cpp
+++ test/Refactor/Extract/ExtractExprIntoFunction.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-refactor extract -selection=test:%s %s -- -std=c++11 2>&1 | FileCheck %s
+// RUN: clang-refactor extract -selection=test:%s %s -- -std=c++11 2>&1 | grep -v CHECK | FileCheck %s
 
 
 void simpleExtractNoCaptures() {
Index: lib/Tooling/Refactoring/Extract.cpp
===
--- lib/Tooling/Refactoring/Extract.cpp
+++ lib/Tooling/Refactoring/Extract.cpp
@@ -45,12 +45,12 @@
 }
 
 class ExtractableCodeSelectionRequirement final
-: public CodeRangeSelectionRequirement {
+: public CodeRangeASTSelectionRequirement {
 public:
   Expected
   evaluate(RefactoringRuleContext &Context) const {
 Expected Selection =
-CodeRangeSelectionRequirement::evaluate(Context);
+CodeRangeASTSelectionRequirement::evaluate(Context);
 if (!Selection)
   return Selection.takeError();
 CodeRangeASTSelection &Code = *Selection;
@@ -207,7 +207,8 @@
   StringRef getCommand() const override { return "extract"; }
 
   StringRef getDescription() const override {
-return "Extracts code into a new function / method / variable";
+return "(WIP action; use with caution!) Extracts code into a new function "
+   "/ method / variable";
   }
 
   /// Returns a set of refactoring actions rules that are defined by this
Index: lib/Tooling/Refactoring/ASTSelectionRequirements.cpp
===
--- lib/Tooling/Refactoring/ASTSelectionRequirements.cpp
+++ lib/Tooling/Refactoring/ASTSelectionRequirements.cpp
@@ -28,8 +28,8 @@
   return std::move(*Selection);
 }
 
-Expected
-CodeRangeSelectionRequirement::evaluate(RefactoringRuleContext &Context) const {
+Expected CodeRangeASTSelectionRequirement::evaluate(
+RefactoringRuleContext &Context) const {
   // FIXME: Memoize so that selection is evaluated only once.
   Expected ASTSelection =
   ASTSelectionRequirement::evaluate(Context);
Index: lib/Tooling/Refactoring/ASTSelection.cpp
===
--- lib/Tooling/Refactoring/ASTSelection.cpp
+++ lib/Tooling/Refactoring/ASTSelection.cpp
@@ -337,6 +337,9

[PATCH] D38982: [refactor] Initial outline of implementation of "extract function" refactoring

2017-10-20 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

In https://reviews.llvm.org/D38982#900744, @ioeric wrote:

> Code looks good in general. I see there are a bunch of major features 
> missing; it might make sense to print a warning or document the major missing 
> features in the high-level API.


I added a warning in the description of the action.




Comment at: 
include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h:75
 /// \see CodeRangeASTSelection
-class CodeRangeSelectionRequirement : public ASTSelectionRequirement {
+class CodeRangeASTSelectionRequirement : public ASTSelectionRequirement {
 public:

Renamed to `CodeRangeASTSelectionRequirement`



Comment at: lib/Tooling/Refactoring/Extract.cpp:167
+  OS << "return ";
+OS << ExtractedCodeRewriter.getRewrittenText(ExtractedRange);
+// FIXME: Compute the correct semicolon policy.

ioeric wrote:
> An alternative way to get the source code is:
> ``` 
> Lexer::getSourceText(
>   CharSourceRange::getTokenRange(SM.getSpellingLoc(Start), 
> SM.getSpellingLoc(End)),
>   SM, Result.Context->getLangOpts());
> ```
I will need to get the rewritten source, so I've started using it in the first 
patch.



Comment at: tools/clang-refactor/TestSupport.cpp:106
+auto Buf = llvm::MemoryBuffer::getMemBuffer(Result->c_str());
+for (llvm::line_iterator It(*Buf, /*SkipBlanks=*/false); !It.is_at_end();
+ ++It) {

ioeric wrote:
> Can we filter the `CHECK`s with `sed` in the test? Similar to 
> https://github.com/llvm-mirror/clang-tools-extra/blob/master/test/change-namespace/simple-move.cpp#L1
Fixed.


Repository:
  rL LLVM

https://reviews.llvm.org/D38982



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

[PATCH] D38982: [refactor] Initial outline of implementation of "extract function" refactoring

2017-10-20 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 119711.
arphaman added a comment.

Fix diff


Repository:
  rL LLVM

https://reviews.llvm.org/D38982

Files:
  include/clang/Basic/DiagnosticRefactoringKinds.td
  include/clang/Tooling/Refactoring/ASTSelection.h
  include/clang/Tooling/Refactoring/RefactoringActionRegistry.def
  include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
  include/clang/Tooling/Refactoring/RefactoringRuleContext.h
  lib/Tooling/Refactoring/ASTSelection.cpp
  lib/Tooling/Refactoring/ASTSelectionRequirements.cpp
  lib/Tooling/Refactoring/CMakeLists.txt
  lib/Tooling/Refactoring/Extract.cpp
  test/Refactor/Extract/ExtractExprIntoFunction.cpp
  test/Refactor/LocalRename/Field.cpp
  test/Refactor/LocalRename/NoSymbolSelectedError.cpp
  test/Refactor/tool-test-support.c
  tools/clang-refactor/TestSupport.cpp

Index: tools/clang-refactor/TestSupport.cpp
===
--- tools/clang-refactor/TestSupport.cpp
+++ tools/clang-refactor/TestSupport.cpp
@@ -20,6 +20,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/LineIterator.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Regex.h"
 #include "llvm/Support/raw_ostream.h"
@@ -241,9 +242,9 @@
 // Dump the results:
 const auto &TestGroup = TestRanges.GroupedRanges[Group.index()];
 if (!CanonicalResult) {
-  llvm::errs() << TestGroup.Ranges.size() << " '" << TestGroup.Name
+  llvm::outs() << TestGroup.Ranges.size() << " '" << TestGroup.Name
<< "' results:\n";
-  llvm::errs() << *CanonicalErrorMessage << "\n";
+  llvm::outs() << *CanonicalErrorMessage << "\n";
 } else {
   llvm::outs() << TestGroup.Ranges.size() << " '" << TestGroup.Name
<< "' results:\n";
@@ -271,6 +272,25 @@
  (NewlinePos == StringRef::npos ? ColumnOffset : (unsigned)NewlinePos);
 }
 
+static unsigned addEndLineOffsetAndEndColumn(StringRef Source, unsigned Offset,
+ unsigned LineNumberOffset,
+ unsigned Column) {
+  StringRef Line = Source.drop_front(Offset);
+  unsigned LineOffset = 0;
+  for (; LineNumberOffset != 0; --LineNumberOffset) {
+size_t NewlinePos = Line.find_first_of("\r\n");
+// Line offset goes out of bounds.
+if (NewlinePos == StringRef::npos)
+  break;
+LineOffset += NewlinePos + 1;
+Line = Line.drop_front(NewlinePos + 1);
+  }
+  // Source now points to the line at +lineOffset;
+  size_t LineStart = Source.find_last_of("\r\n", /*From=*/Offset + LineOffset);
+  return addColumnOffset(
+  Source, LineStart == StringRef::npos ? 0 : LineStart + 1, Column - 1);
+}
+
 Optional
 findTestSelectionRanges(StringRef Filename) {
   ErrorOr> ErrOrFile =
@@ -282,11 +302,11 @@
   }
   StringRef Source = ErrOrFile.get()->getBuffer();
 
-  // FIXME (Alex L): 3rd capture groups for +line:column.
   // See the doc comment for this function for the explanation of this
   // syntax.
   static Regex RangeRegex("range[[:blank:]]*([[:alpha:]_]*)?[[:blank:]]*=[[:"
-  "blank:]]*(\\+[[:digit:]]+)?");
+  "blank:]]*(\\+[[:digit:]]+)?[[:blank:]]*(->[[:blank:]"
+  "]*[\\+\\:[:digit:]]+)?");
 
   std::map> GroupedRanges;
 
@@ -304,18 +324,22 @@
 StringRef Comment =
 Source.substr(Tok.getLocation().getRawEncoding(), Tok.getLength());
 SmallVector Matches;
-// Allow CHECK: comments to contain range= commands.
-if (!RangeRegex.match(Comment, &Matches) || Comment.contains("CHECK")) {
-  // Try to detect mistyped 'range:' comments to ensure tests don't miss
-  // anything.
+// Try to detect mistyped 'range:' comments to ensure tests don't miss
+// anything.
+auto DetectMistypedCommand = [&]() -> bool {
   if (Comment.contains_lower("range") && Comment.contains("=") &&
   !Comment.contains_lower("run") && !Comment.contains("CHECK")) {
 llvm::errs() << "error: suspicious comment '" << Comment
  << "' that "
 "resembles the range command found\n";
 llvm::errs() << "note: please reword if this isn't a range command\n";
-return None;
   }
+  return false;
+};
+// Allow CHECK: comments to contain range= commands.
+if (!RangeRegex.match(Comment, &Matches) || Comment.contains("CHECK")) {
+  if (DetectMistypedCommand())
+return None;
   continue;
 }
 unsigned Offset = Tok.getEndLoc().getRawEncoding();
@@ -325,9 +349,27 @@
   if (Matches[2].drop_front().getAsInteger(10, ColumnOffset))
 assert(false && "regex should have produced a number");
 }
-// FIXME (Alex L): Support true ranges.
 Offset = addColumnOffset(Source, Offset, Col

[PATCH] D38618: Do not add a colon chunk to the code completion of class inheritance access modifiers

2017-10-23 Thread Alex Lorenz via Phabricator via cfe-commits

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

Overall LGTM, just a couple of comments:




Comment at: include/clang/Sema/Scope.h:131
+
+/// We are between inheritance colon and the real class/struct definition 
scope
+ClassInheritanceScope = 0x80,

Nit: Add '.' to the end of the comment + clang-format.



Comment at: lib/Parse/ParseDeclCXX.cpp:3198
   if (Tok.is(tok::colon)) {
+ParseScope InheritanceScope(this, Scope::ClassScope | Scope::DeclScope |
+  Scope::ClassInheritanceScope);

Might be better to use `getCurScope()->getFlags() | 
Scope::ClassInheritanceScope` to avoid dealing with any future scoping rule 
changes if such changes will occur.



Comment at: lib/Sema/SemaCodeComplete.cpp:1661
 
+bool IsNotInheritanceScope = !(S->getFlags() & 
Scope::ClassInheritanceScope);
 // public:

80 columns violation, please clang-format


https://reviews.llvm.org/D38618



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

[PATCH] D38538: Avoid printing some redundant name qualifiers in completion

2017-10-23 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman accepted this revision.
arphaman added inline comments.
This revision is now accepted and ready to land.



Comment at: test/CodeCompletion/qualifiers-as-written.cpp:29
+  // CHECK-2: COMPLETION: func : [#int#]func(<#foo a#>, <#bar b#>, <#ns::bar 
c#>, <#ns::baz d#>
+  // CHECK-2: COMPLETION: func : [#int#]func(<#foo::type a#>, <#bar b#>, <#baz 
c#>
+}

ilya-biryukov wrote:
> arphaman wrote:
> > Sorry, I see the issue now. However, I don't think that we'd like to change 
> > the signature for a function like this, as we'd still prefer to show `func 
> > (foo::type, ns::bar, ns::baz);` on this line.
> > 
> > In Xcode we actually avoid the problem with `std::vector<>`s that you've 
> > pointed out entirely by using `value_type`. I'll check what our solution 
> > does.
> > 
> > Btw, maybe using things like `value_type` is completely wrong (with or 
> > without the qualifier)? If we have `std::vector` shouldn't we rather 
> > show `push_back(int _Value)`, rather than the `value_type`? Perhaps this 
> > kind of change should be discussed with a wider community somehow to find 
> > out what's best for all users.
> Using `int _Value` may be good in case of `vector`, but it would 
> probably loose typedefs, i.e. `vector` would still have `int`, which 
> may be undesirable.
> Also, for `vector>`, the type inside `push_back` would probably 
> end up being `vector>`. 
> 
> 
> As for the current vs new behavior, I can totally see why you want to see 
> more context in completion items. 
> 
> I would argue that the current code does not do a great job there, as it only 
> adds qualifiers to unqualified references. But the context may be missing for 
> qualified references as well.
> For example, in the following code:
> ```
>   template >
>   struct vector {
> typedef T value_type;
> 
> typename value_type::some_type foo();
> value_type bar()
>   };
> ```
> 
> Completion item for `vector::bar` will have return type `vector std::allocator>::value_type`. However, completion item for `vector std::allocator>::foo` will have return type `value_type::some_type` (note 
> that no `vector>` qualifier was added).
> 
> I would also question the intent of the current implementation. The following 
> line suggest that adding those qualifers was not intended in the first place:
> ```
>   Policy.SuppressUnwrittenScope = true;
> ```
> But that's just a wild guess, I may be completely wrong.
> 
> 
> That being said, I don't think there is one right preference, different 
> people may prefer different options. We can surely discuss that in a wider 
> community, though.
> 
> Would you be open to adding an option for that in completion and keeping the 
> current behavior as a default?
> 
> 
Sorry about the delay.

I clarified Xcode's behavior - we actually didn't show the qualifiers for 
typedefs and typealiases because of an older Clang, with ToT the behavior is as 
you describe.

So this patch will remove qualifiers for structs and classes, like in the 
`func` example here. I'm willing to accept it right now. We might have to 
revisit this change at some point though. Let's also keep one behavior right 
now, I don't think we need to diverge yet.



https://reviews.llvm.org/D38538



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

[PATCH] D36790: [ObjC] Messages to 'self' in class methods should use class method dispatch to avoid multiple method ambiguities

2017-10-23 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added a comment.

@doug.gregor Ping


Repository:
  rL LLVM

https://reviews.llvm.org/D36790



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

[PATCH] D37341: [Sema] Fix an assert-on-invalid by avoiding function template specialisation deduction for invalid functions with fabricated template arguments

2017-10-23 Thread Alex Lorenz via Phabricator via cfe-commits

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

In https://reviews.llvm.org/D37341#869042, @vsapsai wrote:

> Does your fix work for deeper nesting too (e.g. template in template in 
> template)? Looks like it should, just want to confirm.


Yes.

> Are there other places where you need to avoid calling 
> `DeduceTemplateArguments` due to templates depth mismatch? 
> `CheckDependentFunctionTemplateSpecialization` should be OK as it's not 
> deducing template arguments. But I'm not sure about 
> `CheckMemberSpecialization`. It doesn't call `DeduceTemplateArguments` 
> directly but I haven't dug deep enough to confirm it's not performing 
> deduction indirectly.

The problem only seems to happen when the TPL is fabricated. I didn't see other 
potential places where such an issue might happen.




Comment at: lib/Sema/SemaDecl.cpp:8880-8881
   << NewFD->getDeclName();
-  } else if (CheckFunctionTemplateSpecialization(NewFD,
+  } else if (!HasFabricatedTemplateSpecializationTemplateParams &&
+ CheckFunctionTemplateSpecialization(NewFD,
   (HasExplicitTemplateArgs ? &TemplateArgs

vsapsai wrote:
> Will something more general like
> 
> !NewFD->isInvalidDecl() && CheckFunctionTemplateSpecialization(...)
> 
> work here? Because if matching template parameters to scope specifier fails, 
> `NewFD` is marked as invalid decl and seems like we can use that.
Yeah, good idea.


Repository:
  rL LLVM

https://reviews.llvm.org/D37341



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

[PATCH] D37341: [Sema] Fix an assert-on-invalid by avoiding function template specialisation deduction for invalid functions with fabricated template arguments

2017-10-23 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman updated this revision to Diff 119967.
arphaman added a comment.

Use just the `isInvalidDecl` check.


Repository:
  rL LLVM

https://reviews.llvm.org/D37341

Files:
  lib/Sema/SemaDecl.cpp
  test/SemaTemplate/deduction-crash.cpp
  test/SemaTemplate/explicit-specialization-member.cpp


Index: test/SemaTemplate/explicit-specialization-member.cpp
===
--- test/SemaTemplate/explicit-specialization-member.cpp
+++ test/SemaTemplate/explicit-specialization-member.cpp
@@ -38,24 +38,20 @@
 
   template
   template
-  void Baz::bar() { // expected-note {{couldn't infer template argument 
'N'}}
+  void Baz::bar() {
   }
 
-  // FIXME: We shouldn't try to match this against a prior declaration if
-  // template parameter matching failed.
   template
-  void Baz::bar<0>() { // expected-error {{cannot specialize a member of an 
unspecialized template}} \
-  // expected-error {{no function template matches}}
+  void Baz::bar<0>() { // expected-error {{cannot specialize a member of an 
unspecialized template}}
   }
 }
 
 namespace PR19340 {
 template struct Helper {
-  template static void func(const T *m) {} // expected-note {{failed 
template argument deduction}}
+  template static void func(const T *m) {}
 };
 
-template void Helper::func<2>() {} // expected-error {{cannot 
specialize a member}} \
-  // expected-error {{no 
function template matches}}
+template void Helper::func<2>() {} // expected-error {{cannot 
specialize a member}}
 }
 
 namespace SpecLoc {
Index: test/SemaTemplate/deduction-crash.cpp
===
--- test/SemaTemplate/deduction-crash.cpp
+++ test/SemaTemplate/deduction-crash.cpp
@@ -144,3 +144,20 @@
   template int n; // expected-error +{{}} 
expected-note {{}}
   int k = n;
 }
+
+namespace deduceFunctionSpecializationForInvalidOutOfLineFunction {
+
+template 
+struct SourceSelectionRequirement {
+  template
+  OutputT evaluateSelectionRequirement(InputT &&Value) {
+  }
+};
+
+template 
+OutputT SourceSelectionRequirement::
+evaluateSelectionRequirement(InputT &&Value) { // expected-error 
{{cannot specialize a member of an unspecialized template}}
+  return Value;
+}
+
+}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -8962,10 +8962,10 @@
   diag::ext_function_specialization_in_class :
   diag::err_function_specialization_in_class)
   << NewFD->getDeclName();
-  } else if (CheckFunctionTemplateSpecialization(NewFD,
-  (HasExplicitTemplateArgs ? &TemplateArgs
-   : nullptr),
- Previous))
+  } else if (!NewFD->isInvalidDecl() &&
+ CheckFunctionTemplateSpecialization(
+ NewFD, (HasExplicitTemplateArgs ? &TemplateArgs : 
nullptr),
+ Previous))
 NewFD->setInvalidDecl();
 
   // C++ [dcl.stc]p1:


Index: test/SemaTemplate/explicit-specialization-member.cpp
===
--- test/SemaTemplate/explicit-specialization-member.cpp
+++ test/SemaTemplate/explicit-specialization-member.cpp
@@ -38,24 +38,20 @@
 
   template
   template
-  void Baz::bar() { // expected-note {{couldn't infer template argument 'N'}}
+  void Baz::bar() {
   }
 
-  // FIXME: We shouldn't try to match this against a prior declaration if
-  // template parameter matching failed.
   template
-  void Baz::bar<0>() { // expected-error {{cannot specialize a member of an unspecialized template}} \
-  // expected-error {{no function template matches}}
+  void Baz::bar<0>() { // expected-error {{cannot specialize a member of an unspecialized template}}
   }
 }
 
 namespace PR19340 {
 template struct Helper {
-  template static void func(const T *m) {} // expected-note {{failed template argument deduction}}
+  template static void func(const T *m) {}
 };
 
-template void Helper::func<2>() {} // expected-error {{cannot specialize a member}} \
-  // expected-error {{no function template matches}}
+template void Helper::func<2>() {} // expected-error {{cannot specialize a member}}
 }
 
 namespace SpecLoc {
Index: test/SemaTemplate/deduction-crash.cpp
===
--- test/SemaTemplate/deduction-crash.cpp
+++ test/SemaTemplate/deduction-crash.cpp
@@ -144,3 +144,20 @@
   template int n; // expected-error +{{}} expected-note {{}}
   int k = n;
 }
+
+namespace deduceFunctionSpecializationForInvalidOutOfLineFunction {
+
+template 
+struct SourceSelectionRequirement {
+  template
+  OutputT evaluateSelectionRequirement(InputT &&Value) {
+

[PATCH] D38982: [refactor] Initial outline of implementation of "extract function" refactoring

2017-10-24 Thread Alex Lorenz via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
arphaman marked an inline comment as done.
Closed by commit rL316465: [refactor] Initial outline of implementation of 
"extract function" refactoring (authored by arphaman).

Changed prior to commit:
  https://reviews.llvm.org/D38982?vs=119711&id=120095#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D38982

Files:
  cfe/trunk/include/clang/Basic/DiagnosticRefactoringKinds.td
  cfe/trunk/include/clang/Tooling/Refactoring/ASTSelection.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRegistry.def
  
cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRuleRequirements.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRulesInternal.h
  cfe/trunk/include/clang/Tooling/Refactoring/RefactoringRuleContext.h
  cfe/trunk/lib/Tooling/Refactoring/ASTSelection.cpp
  cfe/trunk/lib/Tooling/Refactoring/ASTSelectionRequirements.cpp
  cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
  cfe/trunk/lib/Tooling/Refactoring/Extract.cpp
  cfe/trunk/test/Refactor/Extract/ExtractExprIntoFunction.cpp
  cfe/trunk/test/Refactor/LocalRename/Field.cpp
  cfe/trunk/test/Refactor/LocalRename/NoSymbolSelectedError.cpp
  cfe/trunk/test/Refactor/tool-test-support.c
  cfe/trunk/tools/clang-refactor/TestSupport.cpp

Index: cfe/trunk/test/Refactor/tool-test-support.c
===
--- cfe/trunk/test/Refactor/tool-test-support.c
+++ cfe/trunk/test/Refactor/tool-test-support.c
@@ -10,10 +10,13 @@
 
 /*range named =+0*/int test5;
 
+/*range =->+0:22*/int test6;
+
 // CHECK: Test selection group '':
 // CHECK-NEXT:   105-105
 // CHECK-NEXT:   158-158
 // CHECK-NEXT:   197-197
+// CHECK-NEXT:   248-251
 // CHECK-NEXT: Test selection group 'named':
 // CHECK-NEXT:   132-132
 // CHECK-NEXT:   218-218
@@ -29,6 +32,8 @@
 // CHECK: invoking action 'local-rename':
 // CHECK-NEXT: -selection={{.*}}tool-test-support.c:9:29
 
+// CHECK: invoking action 'local-rename':
+// CHECK-NEXT: -selection={{.*}}tool-test-support.c:13:19 -> {{.*}}tool-test-support.c:13:22
 
 // The following invocations are in the 'named' group, and they follow
 // the default invocation even if some of their ranges occur prior to the
Index: cfe/trunk/test/Refactor/LocalRename/NoSymbolSelectedError.cpp
===
--- cfe/trunk/test/Refactor/LocalRename/NoSymbolSelectedError.cpp
+++ cfe/trunk/test/Refactor/LocalRename/NoSymbolSelectedError.cpp
@@ -1,5 +1,5 @@
-// RUN: not clang-refactor local-rename -selection=%s:4:1 -new-name=Bar %s -- 2>&1 | FileCheck %s
-// RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- 2>&1 | FileCheck --check-prefix=TESTCHECK %s
+// RUN: not clang-refactor local-rename -selection=%s:4:1 -new-name=Bar %s -- 2>&1 | grep -v CHECK | FileCheck %s
+// RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- 2>&1 | grep -v CHECK | FileCheck --check-prefix=TESTCHECK %s
 
 class Baz { // CHECK: [[@LINE]]:1: error: there is no symbol at the given location
 };
Index: cfe/trunk/test/Refactor/LocalRename/Field.cpp
===
--- cfe/trunk/test/Refactor/LocalRename/Field.cpp
+++ cfe/trunk/test/Refactor/LocalRename/Field.cpp
@@ -1,9 +1,11 @@
-// RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- | FileCheck %s
+// RUN: clang-refactor local-rename -selection=test:%s -new-name=Bar %s -- | grep -v CHECK | FileCheck %s
 
 class Baz {
-  int /*range=*/Foo; // CHECK: int /*range=*/Bar;
+  int /*range=*/Foo;
+  // CHECK: int /*range=*/Bar;
 public:
   Baz();
 };
 
-Baz::Baz() : /*range=*/Foo(0) {}  // CHECK: Baz::Baz() : /*range=*/Bar(0) {};
+Baz::Baz() : /*range=*/Foo(0) {}
+// CHECK: Baz::Baz() : /*range=*/Bar(0) {}
Index: cfe/trunk/test/Refactor/Extract/ExtractExprIntoFunction.cpp
===
--- cfe/trunk/test/Refactor/Extract/ExtractExprIntoFunction.cpp
+++ cfe/trunk/test/Refactor/Extract/ExtractExprIntoFunction.cpp
@@ -0,0 +1,56 @@
+// RUN: clang-refactor extract -selection=test:%s %s -- -std=c++11 2>&1 | grep -v CHECK | FileCheck %s
+
+
+void simpleExtractNoCaptures() {
+  int i = /*range=->+0:33*/1 + 2;
+}
+
+// CHECK: 1 '' results:
+// CHECK:  static int extracted() {
+// CHECK-NEXT: return 1 + 2;{{$}}
+// CHECK-NEXT: }{{[[:space:]].*}}
+// CHECK-NEXT: void simpleExtractNoCaptures() {
+// CHECK-NEXT:   int i = /*range=->+0:33*/extracted();{{$}}
+// CHECK-NEXT: }
+
+void simpleExtractStmtNoCaptures() {
+  /*range astatement=->+1:13*/int a = 1;
+  int b = 2;
+}
+// CHECK: 1 'astatement' results:
+// CHECK:  static void extracted() {
+// CHECK-NEXT: int a = 1;
+// CHECK-NEXT: int b = 2;;{{$}}
+// CHECK-NEXT: }{{[[:space:]].*}}
+// CHECK-NEXT: void simpleExtractStmtNoCaptures() {
+// CHECK-NEXT:   /*range astatement=->+1:13*/extracted(){{$}}
+// CHECK-NEXT: }
+
+
+void blankRangeN

[PATCH] D38982: [refactor] Initial outline of implementation of "extract function" refactoring

2017-10-24 Thread Alex Lorenz via Phabricator via cfe-commits

arphaman added inline comments.



Comment at: include/clang/Basic/DiagnosticRefactoringKinds.td:24
+  "not overlap with the AST nodes of interest">;
+
+def err_refactor_code_outside_of_function : Error<"the selected code is not a "

ioeric wrote:
> nit: was this newline intended?
Yeah, it separates the extraction errors from the others.


Repository:
  rL LLVM

https://reviews.llvm.org/D38982



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

1 2 3 4 5 6 7 8 9 10 >

1 - 100 of 1474 matches

Mail list logo