[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] D38772: [refactor] allow the use of refactoring diagnostics

2017-10-12 Thread Haojian Wu via Phabricator via cfe-commits 
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

Looks good.




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

arphaman wrote:
> 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.
Thanks for explanation.


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 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] D38772: [refactor] allow the use of refactoring diagnostics

2017-10-11 Thread Haojian Wu via Phabricator via cfe-commits 
hokein added a comment.

The code looks most good to me, a few nits.




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

just curious: is this change needed?



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

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. 


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-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.