subject:"\[PATCH\] D24192\: \[clang\-refactor\] introducing clang\-refactor"

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-21 Thread Eric Liu via cfe-commits

ioeric added inline comments.


Comment at: clang-refactor/driver/Driver.cpp:36
@@ +35,3 @@
+argv[1] = (llvm::Twine(argv[0]) + " " + 
llvm::Twine(argv[1])).str().c_str();
+Manager.dispatch(Command, argc - 1, argv + 1);
+  } else {

arphaman wrote:
> Is there a particular reason why you don't return a value that's returned by 
> Manager.dispatch here? It seems odd to me that Manager.dispatch returns an 
> integer value that's not actually used here.
BTW, isn't `argv[1]` pointing to a temp string above?


https://reviews.llvm.org/D24192



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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-20 Thread Alex Lorenz via cfe-commits

arphaman added inline comments.


Comment at: clang-refactor/driver/Driver.cpp:36
@@ +35,3 @@
+argv[1] = (llvm::Twine(argv[0]) + " " + 
llvm::Twine(argv[1])).str().c_str();
+Manager.dispatch(Command, argc - 1, argv + 1);
+  } else {

Is there a particular reason why you don't return a value that's returned by 
Manager.dispatch here? It seems odd to me that Manager.dispatch returns an 
integer value that's not actually used here.


https://reviews.llvm.org/D24192



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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-20 Thread Alex Lorenz via cfe-commits

arphaman added inline comments.


Comment at: clang-refactor/core/SymbolIndexClient.h:31
@@ +30,3 @@
+} // end namespace refactor
+} // end namespace refactor
+

End namespace clang?


https://reviews.llvm.org/D24192



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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-19 Thread Eric Liu via cfe-commits

ioeric added inline comments.


Comment at: clang-refactor/driver/ModuleManager.h:14-20
@@ +13,9 @@
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/StringRef.h"
+
+#include 
+#include 
+#include 
+
+#include "core/RefactoringModule.h"
+

curdeius wrote:
> I thought that idea behind sorting includes using clang-format is to avoid 
> handling groups and order manually.
> I don't think that there is any policy prohibiting separating includes into 
> groups, but AFAIK, there is one that says that STL includes should be the 
> last (you should include in order from the most specific to the most generic, 
> i.e. subproject, clang, llvm, STL).
Fortunately, clang-format is smart enough to categorize #include groups (e.g. 
LLVM includes, STL includes, main header etc). We actually encourage people to 
combine #includes groups into one block and let clang-format handle the 
categorization (fyi: clang-format only sort includes within a block). The point 
is to free you from worrying about the formatting.


https://reviews.llvm.org/D24192



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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-19 Thread Kirill Bobyrev via cfe-commits

omtcyfz updated this revision to Diff 71776.
omtcyfz added a comment.
Herald added a subscriber: mgorny.

The diff doesn't really do much right now, but it kind of gives the idea of 
what's happening with clang-refactor.


https://reviews.llvm.org/D24192

Files:
  CMakeLists.txt
  clang-refactor/CMakeLists.txt
  clang-refactor/core/BasicExecutor.cpp
  clang-refactor/core/BasicExecutor.h
  clang-refactor/core/CMakeLists.txt
  clang-refactor/core/Executor.h
  clang-refactor/core/ModuleEntryPoint.cpp
  clang-refactor/core/ModuleEntryPoint.h
  clang-refactor/core/ModuleManager.cpp
  clang-refactor/core/ModuleManager.h
  clang-refactor/core/RefactoringContext.cpp
  clang-refactor/core/RefactoringContext.h
  clang-refactor/core/RefactoringModule.cpp
  clang-refactor/core/RefactoringModule.h
  clang-refactor/core/RefactoringOptions.h
  clang-refactor/core/SymbolIndexClient.h
  clang-refactor/driver/CMakeLists.txt
  clang-refactor/driver/Driver.cpp
  clang-refactor/modules/CMakeLists.txt

Index: clang-refactor/driver/Driver.cpp
===
--- /dev/null
+++ clang-refactor/driver/Driver.cpp
@@ -0,0 +1,48 @@
+//===--- Driver.cpp - clang-refactor *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+///
+///  \file This file implements a clang-refactor tool.
+///
+===--===//
+
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include "ModuleManager.h"
+
+namespace clang {
+namespace refactor {
+
+static int printHelpMessage(ModuleManager ) {
+  llvm::outs() << Manager.getTopLevelHelp();
+  return 1;
+}
+
+int clangRefactorMain(int argc, const char **argv) {
+  ModuleManager Manager;
+  if (argc > 1) {
+llvm::StringRef FirstArgument(argv[1]);
+if (FirstArgument == "-h" || FirstArgument == "-help" ||
+FirstArgument == "--help")
+  return printHelpMessage(Manager);
+const std::string Command = argv[1];
+argv[1] = (llvm::Twine(argv[0]) + " " + llvm::Twine(argv[1])).str().c_str();
+Manager.dispatch(Command, argc - 1, argv + 1);
+  } else {
+return printHelpMessage(Manager);
+  }
+  return 0;
+}
+
+} // end namespace refactor
+} // end namespace clang
+
+int main(int argc, const char **argv) {
+  return clang::refactor::clangRefactorMain(argc, argv);
+}
Index: clang-refactor/driver/CMakeLists.txt
===
--- /dev/null
+++ clang-refactor/driver/CMakeLists.txt
@@ -0,0 +1,17 @@
+set(LLVM_LINK_COMPONENTS
+  Support
+  )
+
+add_clang_executable(clang-refactor
+  Driver.cpp
+  )
+
+target_link_libraries(clang-refactor
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangFrontend
+  clangTooling
+
+  clangRefactorCore
+  )
Index: clang-refactor/core/SymbolIndexClient.h
===
--- /dev/null
+++ clang-refactor/core/SymbolIndexClient.h
@@ -0,0 +1,33 @@
+//===--- SymbolIndexClient.h - clang-refactor ---*- 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_EXTRA_CLANG_REFACTOR_SYMBOL_INDEX_CLIENT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_REFACTOR_SYMBOL_INDEX_CLIENT_H
+
+#include 
+#include 
+
+namespace clang {
+namespace refactor {
+
+// FIXME: Implement two following classes.
+// SymbolDeclaration and SymbolUsage might reuse clang::tooling::Range.
+struct SymbolDeclaration {};
+struct SymbolUsage {};
+
+class SymbolIndexClient {
+public:
+  std::vector getDeclarationsForUSR(StringRef USR);
+  std::vector getReferencesForUSR(StringRef USR);
+};
+
+} // end namespace refactor
+} // end namespace refactor
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_REFACTOR_SYMBOL_INDEX_CLIENT_H
Index: clang-refactor/core/RefactoringOptions.h
===
--- /dev/null
+++ clang-refactor/core/RefactoringOptions.h
@@ -0,0 +1,25 @@
+//===--- RefactoringOptions.h - clang-refactor --*- 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_EXTRA_CLANG_REFACTOR_REFACTORING_OPTIONS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_REFACTOR_REFACTORING_OPTIONS_H
+
+#include "llvm/ADT/StringMap.h"
+#include 
+

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-09 Thread Kirill Bobyrev via cfe-commits

omtcyfz updated this revision to Diff 70802.
omtcyfz marked 2 inline comments as done.
omtcyfz added a comment.

Slightly improve the interface.

Patch is still not complete, though.


https://reviews.llvm.org/D24192

Files:
  CMakeLists.txt
  clang-refactor/CMakeLists.txt
  clang-refactor/driver/CMakeLists.txt
  clang-refactor/driver/Driver.cpp
  clang-refactor/driver/ModuleManager.cpp
  clang-refactor/driver/ModuleManager.h
  clang-refactor/modules/CMakeLists.txt
  clang-refactor/modules/core/CMakeLists.txt
  clang-refactor/modules/core/RefactoringModule.cpp
  clang-refactor/modules/core/RefactoringModule.h

Index: clang-refactor/modules/core/RefactoringModule.h
===
--- /dev/null
+++ clang-refactor/modules/core/RefactoringModule.h
@@ -0,0 +1,142 @@
+//===--- RefactoringModule.h - clang-refactor ---*- 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_EXTRA_CLANG_REFACTOR_REFACTORING_MODULE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_REFACTOR_REFACTORING_MODULE_H
+
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/CompilationDatabase.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "clang/Basic/Diagnostic.h"
+#include "llvm/Support/Signals.h"
+#include 
+
+namespace clang {
+namespace refactor {
+
+class RefactoringModule {
+public:
+  RefactoringModule(const std::string ,
+const std::string )
+  : ModuleName(ModuleName), ModuleMetaDescription(ModuleMetaDescription) {}
+
+  //======//
+  // Refactoring cosists of 3 stages.
+  //
+  //======//
+  //
+  // Stage I: Plan execution.
+  //
+  // In this stage the submodule parses arguments, determines whether the
+  // refactoring invokation is correct and stores information neeeded to perform
+  // that refactoring in each affected translation unit.
+  //
+  // Input: Command line (?) arguments.
+  //
+  // Ouput: List of affected translation units, in which the refactoring would
+  // happen, and submodule-specific information needed for the execution.
+  //
+  // Has access to: AST  belongs to, cross-reference index.
+  //
+  //======//
+  //
+  // Stage II: Running on a single translation unit.
+  //
+  // Input: submodule-specific information needed for the execution from
+  // Stage I.
+  //
+  // Output: list of replacements and diagnostics.
+  //
+  // Has access to: AST of given translation unit.
+  //
+  //======//
+  //
+  // Stage III: Apply replacements and output diagnostics.
+  //
+  // This step is not submodule-specific. Therefore, it should be delegated to
+  // the common interface.
+  //
+  // Input: Replacements and diagnostics from stage II.
+  //
+  // Output: Applied replacements and issued diagnostics.
+  //
+  //======//
+
+  int run(int argc, const char **argv);
+
+  // Routine for registering common refactoring options.
+  //
+  // Common options reused by multiple tools like "-offset", "verbose" etc
+  // should go here.
+  void registerCommonOptions(llvm::cl::OptionCategory );
+
+  // A way for each tool to provide additional options. Overriding this one is
+  // optional.
+  virtual void registerCustomOptions(llvm::cl::OptionCategory ) = 0;
+
+  //======//
+  // Stage I: Plan execution.
+
+  // This processes given translation unit of  and retrieves
+  // information needed for the refactoring.
+  //
+  // Input: ASTContext of translation unit  belongs to.
+  //
+  // Output: Serialized refactoring-specific information;
+  virtual std::string getExecutionInformation() = 0;
+
+  // Return translation units affected by certain refactoring. As a temporary
+  // solution it can return all translation units, regardless of them being
+  // affected by refactoring.
+  //
+  // Input: Serialized refactroing-specific information.
+  //
+  // Output: List of translation units.
+  virtual std::vector getAffectedTUs(tooling::CompilationDatabase,
+  std::string RefactoringInfo);
+  //======//
+
+

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-08 Thread Marek Kurdej via cfe-commits

curdeius added inline comments.


Comment at: clang-refactor/driver/ModuleManager.h:14-20
@@ +13,9 @@
+#include "clang/Basic/LLVM.h"
+#include "llvm/ADT/StringRef.h"
+
+#include 
+#include 
+#include 
+
+#include "core/RefactoringModule.h"
+

I thought that idea behind sorting includes using clang-format is to avoid 
handling groups and order manually.
I don't think that there is any policy prohibiting separating includes into 
groups, but AFAIK, there is one that says that STL includes should be the last 
(you should include in order from the most specific to the most generic, i.e. 
subproject, clang, llvm, STL).


https://reviews.llvm.org/D24192



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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-08 Thread Kirill Bobyrev via cfe-commits

omtcyfz added inline comments.


Comment at: clang-refactor/driver/ModuleManager.cpp:22-24
@@ +21,5 @@
+int ModuleManager::Dispatch(StringRef Command, int argc, const char **argv) {
+  if (CommandToModuleID.find(Command) != CommandToModuleID.end()) {
+return RegisteredModules[CommandToModuleID[Command]]->run(argc, argv);
+  }
+  return 1;

curdeius wrote:
> Using `find` and then `operator[]` makes the search twice. IMO, it would be 
> better to avoid that by:
> 
> ```
>   auto it = CommandToModuleID.find(Command);
>   if (it != CommandToModuleID.end()) {
> return RegisteredModules[*it]->run(argc, argv);
>   }
> ```
The search is O(1) anyway, but the code itself probably becomes more 
reasonable, thanks.


Comment at: clang-refactor/driver/ModuleManager.h:13-19
@@ +12,9 @@
+
+#include "llvm/ADT/StringRef.h"
+
+#include 
+#include 
+#include 
+
+#include "core/RefactoringModule.h"
+

curdeius wrote:
> clang-format includes (and make it a single block)?
I like to separate includes into three groups (LLVM includes, STL includes, 
subproject includes), which I think is not very bad. Haven't seen any policy 
prohibiting it. Feel free to elaborate, though.

Sorted the includes within the second group.


Comment at: clang-refactor/modules/core/RefactoringModule.h:148
@@ +147,3 @@
+  // Panic: if there are conflicting replacements.
+  virtual int applyReplacementsOrOutputDiagnostics() = 0;
+

alexfh wrote:
> This should be possible to implement in a refactoring-independent way in 
> clang-refactor itself. Or do you see any issues with this?
Nope, totally agree.


https://reviews.llvm.org/D24192



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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-08 Thread Kirill Bobyrev via cfe-commits

omtcyfz updated this revision to Diff 70682.
omtcyfz marked 8 inline comments as done.
omtcyfz added a comment.
Herald added a subscriber: beanz.

Addressing few comments.

Major improvements on the way.


https://reviews.llvm.org/D24192

Files:
  CMakeLists.txt
  clang-refactor/CMakeLists.txt
  clang-refactor/driver/CMakeLists.txt
  clang-refactor/driver/Driver.cpp
  clang-refactor/driver/ModuleManager.cpp
  clang-refactor/driver/ModuleManager.h
  clang-refactor/modules/CMakeLists.txt
  clang-refactor/modules/core/RefactoringModule.h

Index: clang-refactor/modules/core/RefactoringModule.h
===
--- /dev/null
+++ clang-refactor/modules/core/RefactoringModule.h
@@ -0,0 +1,169 @@
+//===--- RefactoringModule.h - clang-refactor ---*- 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_EXTRA_CLANG_REFACTOR_REFACTORING_MODULE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_REFACTOR_REFACTORING_MODULE_H
+
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Signals.h"
+
+#include 
+
+namespace clang {
+namespace refactor {
+
+class RefactoringModule {
+public:
+  RefactoringModule(const std::string ,
+const std::string )
+  : ModuleName(ModuleName), ModuleMetaDescription(ModuleMetaDescription) {}
+
+  // Refactoring consists of 3.5 stages:
+  //
+  // 0. Check arguments for validity.
+  //
+  // 1. Extract information needed for the refactoring upon tool invocation.
+  //
+  // 2. Handle translation unit and identify whether the refactoring can be
+  // applied. If it can, store the replacements. Panic otherwise.
+  //
+  // 3. Merge duplicate replacements, panic if there are conflicting ones.
+  // Process translation unit and apply refactoring.
+  //
+  // Few examples of how each of these stages would look like for future
+  // modules "rename" and "inline".
+  //
+  // Rename
+  // ==
+  //
+  // 0. Check arguments for validity.
+  //
+  // 1. Rename operates with USRs, so the first stage would be finding the
+  // symbol, which will be renamed and storing its USR.
+  //
+  // 2. Check whether renaming will introduce any name conflicts. If it won't
+  // find each occurrence of the symbol in translation unit using USR and store
+  // replacements.
+  //
+  // 3. Apply replacements or output diagnostics. Handle duplicates and panic if
+  // there are conflicts.
+  //
+  // Inline
+  // ==
+  //
+  // 0. Check arguments for validity.
+  //
+  // 1. Find function, identify what the possible issues might be.
+  //
+  // 2. Check whether inlining will introduce any issues, e.g. there is a
+  // pointer passed somewhere to the inlined function and after inlining it the
+  // code will no longer compile. If it won't find function calls, add needed
+  // temporary variables and replace the call with function body.
+  //
+  // 3. Apply replacements. Handle duplicates and panic if there are conflicts.
+  //
+  // Summary
+  // ===
+  //
+  // As one can easily see, step 1 should be performed upon module invocation
+  // and it needs to process translation unit, from which the passed 
+  // comes from.
+  //
+  // With appropriate facilities step 2 can be parallelized to process multiple
+  // translation units of the project independently. If none of them have any
+  // issues with applying this refactoring replacements are stored and queued
+  // for later.
+  //
+  // Step 3 can be parallelized even more easily. It basically consists of text
+  // replacements.
+  //
+  int run(int argc, const char **argv) {
+// Register options.
+llvm::cl::OptionCategory RefactoringModuleOptions(ModuleName.c_str(),
+  ModuleMetaDescription.c_str());
+registerCustomOptions(RefactoringModuleOptions);
+registerCustomOptions(RefactoringModuleOptions);
+// Parse options and get compilations.
+llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+tooling::CommonOptionsParser OptionsParser(argc, argv,
+   RefactoringModuleOptions);
+tooling::RefactoringTool Tool(OptionsParser.getCompilations(),
+  OptionsParser.getSourcePathList());
+
+// Actual routine.
+checkArguments();
+extractRefactoringInformation();
+handleTranslationUnit();
+applyReplacementsOrOutputDiagnostics();
+return 0;
+  }
+
+  // Routine for registering common modules options.
+  void registerCommonOptions(llvm::cl::OptionCategory ) {
+

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-07 Thread Marek Kurdej via cfe-commits

curdeius added a subscriber: curdeius.
curdeius added a comment.

For the moment, just a few nitty-gritty comments inline.
What I miss here is (as already pointed by someone) an example on how to write 
a new module, register it etc.



Comment at: clang-refactor/driver/Driver.cpp:41
@@ +40,3 @@
+Command = argv[1];
+std::string Invocation = std::string(argv[0]) + " " + argv[1];
+argv[1] = Invocation.c_str();

Simpler:

```
std::string Invocation = argv[0] + (" " + Command);
```


Comment at: clang-refactor/driver/ModuleManager.cpp:22-24
@@ +21,5 @@
+int ModuleManager::Dispatch(StringRef Command, int argc, const char **argv) {
+  if (CommandToModuleID.find(Command) != CommandToModuleID.end()) {
+return RegisteredModules[CommandToModuleID[Command]]->run(argc, argv);
+  }
+  return 1;

Using `find` and then `operator[]` makes the search twice. IMO, it would be 
better to avoid that by:

```
  auto it = CommandToModuleID.find(Command);
  if (it != CommandToModuleID.end()) {
return RegisteredModules[*it]->run(argc, argv);
  }
```


Comment at: clang-refactor/driver/ModuleManager.h:13-19
@@ +12,9 @@
+
+#include "llvm/ADT/StringRef.h"
+
+#include 
+#include 
+#include 
+
+#include "core/RefactoringModule.h"
+

clang-format includes (and make it a single block)?


Comment at: clang-refactor/driver/ModuleManager.h:21
@@ +20,3 @@
+
+using namespace llvm;
+

`using namespace` in header?!


Comment at: clang-refactor/modules/core/RefactoringModule.h:36
@@ +35,3 @@
+  //
+  // 1. Extract infromation needed for the refactoring upon tool invocation.
+  //

s/infromation/information


Comment at: clang-refactor/modules/core/RefactoringModule.h:56
@@ +55,3 @@
+  // 2. Check whether renaming will introduce any name conflicts. If it won't
+  // find each occurance of the symbol in translation unit using USR and store
+  // replacements.

s/occurance/occurrence


Comment at: clang-refactor/modules/core/RefactoringModule.h:72
@@ +71,3 @@
+  // code will no longer compile. If it won't find function calls, add needed
+  // temprorary variables and replace the call with function body.
+  //

s/temprorary/temporary


Comment at: clang-refactor/modules/core/RefactoringModule.h:112
@@ +111,3 @@
+
+  // Routine for regiestering common modules options.
+  void registerCommonOptions(llvm::cl::OptionCategory ) {

s/regiestering/registering


https://reviews.llvm.org/D24192



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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-06 Thread Kirill Bobyrev via cfe-commits

omtcyfz added a comment.

Thank you for reviewing, @hokein!

Also, please note that this is not a final version, the interface will change a 
lot in the upcoming diffs.



Comment at: clang-refactor/driver/Driver.cpp:46
@@ +45,3 @@
+llvm::StringRef FirstArgument(argv[1]);
+if (FirstArgument == "-h" || FirstArgument == "-help" ||
+FirstArgument == "--help")

hokein wrote:
> Any reason implementing the command-line options and not using the `cl::opt` 
> here?
I might be mistaken, but it'd require parsing whole option sequence, which 
should be delegated to the submodules.

E.g. "help" should be only called if the the tool is invoked as `clang-refactor 
--help`, but if I parse all options I'd also invoke "help" while having 
"clang-refactor rename --help" for example. At least that's what I have been 
thinking.


Comment at: clang-refactor/modules/core/RefactoringModule.h:89
@@ +88,3 @@
+  //
+  int run(int argc, const char **argv) {
+// Register options.

hokein wrote:
> I'd make this interface a virtual function too, and provide this default 
> implementation.
I'll update the interface soonish.

Had an offline discussion with Alex about the interface and we came up to a 
conclusion that the interface should be way more strict. This one is just a 
general idea of how it might be implemented.

Thus, run shouldn't be overridden in my opinion, but I'll update the diff and 
make some high-level comments on that later.


Comment at: clang-refactor/modules/core/RefactoringModule.h:128
@@ +127,3 @@
+  //
+  // Panic: if refactoring can not be applied. E.g. unsupported cases like
+  // renaming macros etc.

hokein wrote:
> Does the panic mean something like `exit(1)`? But from the interface, it 
> looks like returning an error code, right?
1. Well, no. I believe that the error should be thrown to the `run` and 
"printed out" there.
2. Also, not that we're aiming for a multi-TU multi-threaded architecture in 
the end (although it doesn't make sense with the current interface, but again, 
I'll update it later).


Comment at: clang-refactor/modules/core/RefactoringModule.h:146
@@ +145,3 @@
+  // Panic: if there are conflicting replacements.
+  virtual int applyReplacements() = 0;
+

hokein wrote:
> A further thought:
> 
> `applyReplacement` is more likely a postprocess step of 
> `handleTranslationUnit`, what if some clang-refactor subtools just want to 
> dump the results only? I'd prefer to rename it to 
> `OnEndOfHandlingTranslationUnit`. I'm open to hear better suggestions. 
/* comment about multi-TU multi-threaded stuff */

Suppose a refactoring can't be applied to some translation units. Say, "rename" 
tool encountered name conflict in any TU. In this case the tool should fail and 
not process the last step.

If your comment is about naming only, I'll change it, too.


Comment at: clang-refactor/modules/template/TemplateModule.h:15
@@ +14,3 @@
+
+#include 
+

hokein wrote:
> Not needed.
Deleting TemplateModule.


Comment at: clang-refactor/modules/template/TemplateModule.h:20
@@ +19,3 @@
+namespace template_module {
+
+class TemplateModule : public RefactoringModule {

hokein wrote:
> It'd be clear to add a small paragraph describing that this is a template 
> module which is only used to lower the "barrier to entry" for writing 
> clang-refactor subtool.
Deleting TemplateModule.


https://reviews.llvm.org/D24192



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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-06 Thread Kirill Bobyrev via cfe-commits

omtcyfz updated this revision to Diff 70397.
omtcyfz marked 11 inline comments as done.
omtcyfz added a comment.

Addressing a round of comments.


https://reviews.llvm.org/D24192

Files:
  CMakeLists.txt
  clang-refactor/CMakeLists.txt
  clang-refactor/driver/CMakeLists.txt
  clang-refactor/driver/Driver.cpp
  clang-refactor/driver/ModuleManager.cpp
  clang-refactor/driver/ModuleManager.h
  clang-refactor/modules/CMakeLists.txt
  clang-refactor/modules/core/RefactoringModule.h

Index: clang-refactor/modules/core/RefactoringModule.h
===
--- /dev/null
+++ clang-refactor/modules/core/RefactoringModule.h
@@ -0,0 +1,169 @@
+//===--- RefactoringModule.h - clang-refactor ---*- 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_EXTRA_CLANG_REFACTOR_REFACTORING_MODULE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_REFACTOR_REFACTORING_MODULE_H
+
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Signals.h"
+
+#include 
+
+namespace clang {
+namespace refactor {
+
+class RefactoringModule {
+public:
+  RefactoringModule(const std::string ,
+const std::string )
+  : ModuleName(ModuleName), ModuleMetaDescription(ModuleMetaDescription) {}
+
+  // Refactoring consists of 3.5 stages:
+  //
+  // 0. Check arguments for validity.
+  //
+  // 1. Extract infromation needed for the refactoring upon tool invocation.
+  //
+  // 2. Handle translation unit and identify whether the refactoring can be
+  // applied. If it can, store the replacements. Panic otherwise.
+  //
+  // 3. Merge duplicate replacements, panic if there are conflicting ones.
+  // Process translation unit and apply refactoring.
+  //
+  // Few examples of how each of these stages would look like for future
+  // modules "rename" and "inline".
+  //
+  // Rename
+  // ==
+  //
+  // 0. Check arguments for validity.
+  //
+  // 1. Rename operates with USRs, so the first stage would be finding the
+  // symbol, which will be renamed and storing its USR.
+  //
+  // 2. Check whether renaming will introduce any name conflicts. If it won't
+  // find each occurance of the symbol in translation unit using USR and store
+  // replacements.
+  //
+  // 3. Apply replacements or output diagnostics. Handle duplicates and panic if
+  // there are conflicts.
+  //
+  // Inline
+  // ==
+  //
+  // 0. Check arguments for validity.
+  //
+  // 1. Find function, identify what the possible issues might be.
+  //
+  // 2. Check whether inlining will introduce any issues, e.g. there is a
+  // pointer passed somewhere to the inlined function and after inlining it the
+  // code will no longer compile. If it won't find function calls, add needed
+  // temprorary variables and replace the call with function body.
+  //
+  // 3. Apply replacements. Handle duplicates and panic if there are conflicts.
+  //
+  // Summary
+  // ===
+  //
+  // As one can easily see, step 1 should be performed upon module invocation
+  // and it needs to process translation unit, from which the passed 
+  // comes from.
+  //
+  // With appropriate facilities step 2 can be parallelized to process multiple
+  // translation units of the project independently. If none of them have any
+  // issues with applying this refactoring replacements are stored and queued
+  // for later.
+  //
+  // Step 3 can be parallelized even more easily. It basically consists of text
+  // replacements.
+  //
+  int run(int argc, const char **argv) {
+// Register options.
+llvm::cl::OptionCategory RefactoringModuleOptions(ModuleName.c_str(),
+  ModuleMetaDescription.c_str());
+registerCustomOptions(RefactoringModuleOptions);
+registerCustomOptions(RefactoringModuleOptions);
+// Parse options and get compilations.
+llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
+tooling::CommonOptionsParser OptionsParser(argc, argv,
+   RefactoringModuleOptions);
+tooling::RefactoringTool Tool(OptionsParser.getCompilations(),
+  OptionsParser.getSourcePathList());
+
+// Actual routine.
+checkArguments();
+extractRefactoringInformation();
+handleTranslationUnit();
+applyReplacementsOrOutputDiagnostics();
+return 0;
+  }
+
+  // Routine for regiestering common modules options.
+  void registerCommonOptions(llvm::cl::OptionCategory ) {
+  }
+
+  // A way for each tool to provide additional

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-06 Thread Kirill Bobyrev via cfe-commits

omtcyfz added a comment.

Bringing results of an offline discussion with Eric (@ioeric) live.

Eric's point was that this patch should only care about `clang-refactor` and 
introduce changes directly related to creating `clang-rename`. `clang-rename` 
and all other tools migration can be done later, which is also good in terms of 
this patch not growing too large.

Another point was that it should be easier if there would be a class, from 
which refactoring tools would derive and use it as an interface.

Advantages are as follows:

- It would be easier to implement multi-TU support as the tools wouldn't simply 
care about that part while delegating the "hard work" to the common part.
- It would be easier to implement new tools, because there will be a slightly 
simpler interface.
- It would be easier to setup unit tests and other routine, because right now 
all tools have to implement basic routine for unit tests, for example, which is 
just copied from the other tools' unit tests with slight changes. Delegating 
that to the common interface is also nice.

The current diff isn't final, but I wanted to bring it to the reviews, because 
it gives an overview of what I am doing and if there are any high-level 
comments to address it's better to get them sooner than later.


https://reviews.llvm.org/D24192



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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-06 Thread Kirill Bobyrev via cfe-commits

omtcyfz updated the summary for this revision.
omtcyfz removed a reviewer: bkramer.
omtcyfz added subscribers: bkramer, hokein.
omtcyfz updated this revision to Diff 70373.
omtcyfz added a comment.

Removed whole clang-rename part, only making this patch clang-rename specific.


https://reviews.llvm.org/D24192

Files:
  CMakeLists.txt
  clang-refactor/CMakeLists.txt
  clang-refactor/driver/CMakeLists.txt
  clang-refactor/driver/Driver.cpp
  clang-refactor/driver/ModuleManager.cpp
  clang-refactor/driver/ModuleManager.h
  clang-refactor/modules/CMakeLists.txt
  clang-refactor/modules/core/RefactoringModule.h
  clang-refactor/modules/template/CMakeLists.txt
  clang-refactor/modules/template/TemplateModule.cpp
  clang-refactor/modules/template/TemplateModule.h

Index: clang-refactor/modules/template/TemplateModule.h
===
--- /dev/null
+++ clang-refactor/modules/template/TemplateModule.h
@@ -0,0 +1,42 @@
+//===--- RefactoringModuleTemplate.h - clang-refactor ---*- 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_EXTRA_CLANG_REFACTOR_REFACTORING_MODULE_TEMPLATE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_REFACTOR_REFACTORING_MODULE_TEMPLATE_H
+
+#include "../core/RefactoringModule.h"
+
+#include 
+
+namespace clang {
+namespace clang_refactor {
+namespace template_module {
+
+class TemplateModule : public RefactoringModule {
+public:
+  TemplateModule(const std::string ,
+ const std::string )
+  : RefactoringModule(ModuleName, ModuleMetaDesciption) {}
+
+  int checkArguments() override;
+
+  int extractRefactoringInformation() override;
+
+  int handleTranslationUnit() override;
+
+  int applyReplacements() override;
+
+  ~TemplateModule() override {}
+};
+
+} // end namespace template_module
+} // end namespace clang_refactor
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_REFACTOR_REFACTORING_MODULE_TEMPLATE_H
Index: clang-refactor/modules/template/TemplateModule.cpp
===
--- /dev/null
+++ clang-refactor/modules/template/TemplateModule.cpp
@@ -0,0 +1,36 @@
+//===--- RefactoringModuleTemplate.cpp - clang-refactor -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "TemplateModule.h"
+
+#include 
+
+namespace clang {
+namespace clang_refactor {
+namespace template_module {
+
+int TemplateModule::checkArguments() {
+  return 0;
+}
+
+int TemplateModule::extractRefactoringInformation() {
+  return 0;
+}
+
+int TemplateModule::handleTranslationUnit() {
+  return 0;
+}
+
+int TemplateModule::applyReplacements() {
+  return 0;
+}
+
+} // end namespace template_module
+} // end namespace clang_refactor
+} // end namespace clang
Index: clang-refactor/modules/template/CMakeLists.txt
===
--- /dev/null
+++ clang-refactor/modules/template/CMakeLists.txt
@@ -0,0 +1,12 @@
+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangRefactorTemplateModule
+  TemplateModule.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangFrontend
+  clangTooling
+  )
Index: clang-refactor/modules/core/RefactoringModule.h
===
--- /dev/null
+++ clang-refactor/modules/core/RefactoringModule.h
@@ -0,0 +1,167 @@
+//===--- RefactoringModule.h - clang-refactor ---*- 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_EXTRA_CLANG_REFACTOR_REFACTORING_MODULE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_REFACTOR_REFACTORING_MODULE_H
+
+#include "clang/Tooling/CommonOptionsParser.h"
+#include "clang/Tooling/Refactoring.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Signals.h"
+
+#include 
+
+namespace clang {
+namespace clang_refactor {
+
+class RefactoringModule {
+public:
+  RefactoringModule(const std::string ,
+const std::string )
+  : ModuleName(ModuleName), ModuleMetaDescription(ModuleMetaDescription) {}
+
+  // Refactoring consists of 3.5 stages:
+  //
+  // 0. Check arguments for validity.
+  //
+  // 1. Extract infromation needed for the

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-04 Thread Kirill Bobyrev via cfe-commits

omtcyfz added a comment.

In https://reviews.llvm.org/D24192#533220, @ioeric wrote:

> Don't worry about a patch being small. Reviewers like small patches :)
>
> The reason that I suggested a dummy sub-tool is to lower the bar for 
> developers, especially those who have never developed a clang tool, so that 
> they can easily figure out how to set up a clang-refactor sub-tool.
>
> For a "swiss-army knife" tool like clang-refactor, I would worry more about 
> the extendibility of the infrastructure instead of functionality at this 
> stage. At least, I would expect it to be much easier to create a refactor 
> tool under clang-refactor than creating a standalone clang tool. For example, 
> it would be really nice if there is a tool base that a sub-module can derive 
> from so that I don't have to go through the whole process of creating a clang 
> tool and setting up unit/lit test environment :)

Yes, you are absolutely right. This patch is just a mess, because I tried to 
push everything at once while most of the stuff is not related to 
clang-refactor part at all.

I pushed 3 very basic cleanup patches (namely 
https://reviews.llvm.org/rL280638, https://reviews.llvm.org/rL280639 and 
https://reviews.llvm.org/rL280640), which don't really need reviewing (and also 
for the sake of development speed). And I also kept clang-rename improvements 
separate in https://reviews.llvm.org/D24224, which is up for review.

I'll update this patch tomorrow.

What I'm about to do after is to either:

1. Make two patches, one of which would basically create "empty" 
`clang-refactor` and introduce dummy submodule to show how it works (as you 
proposed), and the other would merge `clang-rename` as a submodule.
2. Make one patch, which both creates `clang-refactor` binary and merges 
`clang-rename` as a submodule.

The first variant seems better to me while I see a couple of issues with that, 
which makes sticking to the second variant a bit less challenging. We can 
discuss it tomorr..today and come up with a solution.

Thank you very much for your comments, Eric!

https://reviews.llvm.org/D24192

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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-04 Thread Kirill Bobyrev via cfe-commits

omtcyfz added a comment.

In https://reviews.llvm.org/D24192#533233, @ioeric wrote:

> It was not trivial to me why USREngine is so important to those tools. You 
> might want to address that in the design doc as well. And given the weight 
> USREngine carries in clang-refactor as you suggested, I think it deserves an 
> more detailed introduction in the design doc.

Yes, I agree, it might be a little confusing. The only mention I have is:

> The most important facility would allow lookup of symbol definitions and 
> usages (probably, based on Unified Symbol Resolutions, USR)

But it doesn't get into too much details anyway. However, the idea of the 
design doc was not to focus on implementation details too much as they are a 
subject to change.

Thus, I'm not sure whether I should elaborate details of USREngine there.

Comment at: clang-refactor/driver/Rename.h:192
@@ +191,3 @@
+  auto ID = Sources.translateFile(Entry);
+  Rewrite.getEditBuffer(ID).write(outs());
+}

alexshap wrote:
> if i am not mistaken if the file has not been modified this will print smth 
> like
> "invalid source location" or similar. Pls, double check me.
Sorry, can you please elaborate?

Also, this might be unrelated to this exact patch, but if there is an issue 
it's good to fix it in another patch. Basically, this is not new code, it's 
been there in `clang-rename`.

https://reviews.llvm.org/D24192

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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-03 Thread Alexander Shaposhnikov via cfe-commits

alexshap added a subscriber: alexshap.


Comment at: clang-refactor/driver/Rename.h:192
@@ +191,3 @@
+  auto ID = Sources.translateFile(Entry);
+  Rewrite.getEditBuffer(ID).write(outs());
+}

if i am not mistaken if the file has not been modified this will print smth like
"invalid source location" or similar. Pls, double check me.


https://reviews.llvm.org/D24192



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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-02 Thread Eric Liu via cfe-commits

ioeric added a comment.

In https://reviews.llvm.org/D24192#533200, @omtcyfz wrote:

> In https://reviews.llvm.org/D24192#533198, @ioeric wrote:
>
> > In https://reviews.llvm.org/D24192#533174, @omtcyfz wrote:
> >
> > > In https://reviews.llvm.org/D24192#532981, @ioeric wrote:
> > >
> > > > - It would make the review easier if you could separate the migration 
> > > > of clang-rename into another patch...
> > >
> > >
> > > Another point is that if I try to separate the migration - what do I do 
> > > about USREngine? USREngine is basically the core of clang-refactor at the 
> > > moment and I can't detach it from clang-rename at the same time.
> >
> >
> > I'm not sure why USREngine is the core of clang-refactor. It seems to me 
> > that USREngine is more closely tied to clang-rename than to clang-refactor. 
> > At least USREngine is not essential to all refactor tools, and it is more 
> > like a library that sub-modules can use.
>
>
> It is essential to all of the tools I wrote about in design doc.
>
> Well, are you proposing to create an "empty" `clang-refactor` binary in one 
> patch and adding meaningful code in the other? I am not sure if just creating 
> `clang-refactor/driver/Driver.cpp` with `main`, which doesn't do anything is 
> a good idea, but if you think it is - I'll do that.


It was not trivial to me why USREngine is so important to those tools. You 
might want to address that in the design doc as well. And given the weight 
USREngine carries in clang-refactor as you suggested, I think it deserves an 
more detailed introduction in the design doc.


https://reviews.llvm.org/D24192



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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-02 Thread Eric Liu via cfe-commits

ioeric added a comment.

In https://reviews.llvm.org/D24192#533082, @omtcyfz wrote:

> In https://reviews.llvm.org/D24192#532981, @ioeric wrote:
>
> > - You mentioned a design doc in the summary; maybe also include a link to 
> > it?
>
>
> Done.
>
> > - It would make the review easier if you could separate the migration of 
> > clang-rename into another patch...I think clang-refactor is really the 
> > interesting part here. And maybe also add a small dummy sub-module as an 
> > example demonstrating how to add a new sub-module, which I believe will be 
> > helpful for future developers. (See also tool-template 
> > 
> >  which exists for the same purpose.)
>
>
> I am not against it, but I do not think that an empty tool, which just 
> introduces the binary without any code would be reasonable. This is basically 
> why I wanted it to be all-in-place.
>
> The whole changelist doesn't introduce anything really new, so I assume it 
> might be fine. But if you are not okay with it I can split.

Don't worry about a patch being small. Reviewers like small patches :)

The reason that I suggested a dummy sub-tool is to lower the bar for 
developers, especially those who have never developed a clang tool, so that 
they can easily figure out how to set up a clang-refactor sub-tool.

For a "swiss-army knife" tool like clang-refactor, I would worry more about the 
extendibility of the infrastructure instead of functionality at this stage. At 
least, I would expect it to be much easier to create a refactor tool under 
clang-refactor than creating a standalone clang tool. For example, it would be 
really nice if there is a tool base that a sub-module can derive from so that I 
don't have to go through the whole process of creating a clang tool and setting 
up unit/lit test environment :)

https://reviews.llvm.org/D24192

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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-02 Thread Kirill Bobyrev via cfe-commits

omtcyfz added a comment.

In https://reviews.llvm.org/D24192#533198, @ioeric wrote:

> In https://reviews.llvm.org/D24192#533174, @omtcyfz wrote:
>
> > In https://reviews.llvm.org/D24192#532981, @ioeric wrote:
> >
> > > - It would make the review easier if you could separate the migration of 
> > > clang-rename into another patch...
> >
> >
> > Another point is that if I try to separate the migration - what do I do 
> > about USREngine? USREngine is basically the core of clang-refactor at the 
> > moment and I can't detach it from clang-rename at the same time.
>
>
> I'm not sure why USREngine is the core of clang-refactor. It seems to me that 
> USREngine is more closely tied to clang-rename than to clang-refactor. At 
> least USREngine is not essential to all refactor tools, and it is more like a 
> library that sub-modules can use.

It is essential to all of the tools I wrote about in design doc.

Well, are you proposing to create an "empty" `clang-refactor` binary in one 
patch and adding meaningful code in the other? I am not sure if just creating 
`clang-refactor/driver/Driver.cpp` with `main`, which doesn't do anything is a 
good idea, but if you think it is - I'll do that.

https://reviews.llvm.org/D24192

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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-02 Thread Eric Liu via cfe-commits

ioeric added a comment.

In https://reviews.llvm.org/D24192#533174, @omtcyfz wrote:

> In https://reviews.llvm.org/D24192#532981, @ioeric wrote:
>
> > - It would make the review easier if you could separate the migration of 
> > clang-rename into another patch...
>
>
> Another point is that if I try to separate the migration - what do I do about 
> USREngine? USREngine is basically the core of clang-refactor at the moment 
> and I can't detach it from clang-rename at the same time.

I'm not sure why USREngine is the core of clang-refactor. It seems to me that 
USREngine is more closely tied to clang-rename than to clang-refactor. At least 
USREngine is not essential to all refactor tools, and it is more like a library 
that sub-modules can use.

https://reviews.llvm.org/D24192

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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-02 Thread Kirill Bobyrev via cfe-commits

omtcyfz added a comment.

In https://reviews.llvm.org/D24192#532981, @ioeric wrote:

> - It would make the review easier if you could separate the migration of 
> clang-rename into another patch...

Another point is that if I try to separate the migration - what do I do about 
USREngine? USREngine is basically the core of clang-refactor at the moment and 
I can't detach it from clang-rename at the same time.

https://reviews.llvm.org/D24192

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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-02 Thread Kirill Bobyrev via cfe-commits

omtcyfz updated this revision to Diff 70194.
omtcyfz added a comment.

Revert diff, as the last one "deletes and creates" files instead of "moving and 
changing them" in the filesystem.


https://reviews.llvm.org/D24192

Files:
  CMakeLists.txt
  TemplatedClassFunction.cpp
  clang-refactor/CMakeLists.txt
  clang-refactor/USREngine/CMakeLists.txt
  clang-refactor/USREngine/USRFinder.cpp
  clang-refactor/USREngine/USRFinder.h
  clang-refactor/USREngine/USRFindingAction.cpp
  clang-refactor/USREngine/USRFindingAction.h
  clang-refactor/USREngine/USRLocFinder.cpp
  clang-refactor/USREngine/USRLocFinder.h
  clang-refactor/driver/CMakeLists.txt
  clang-refactor/driver/ClangRefactorOptions.h
  clang-refactor/driver/Driver.cpp
  clang-refactor/driver/Rename.h
  clang-refactor/editor-integrations/CMakeLists.txt
  clang-refactor/editor-integrations/clang-refactor-rename.el
  clang-refactor/editor-integrations/clang-refactor-rename.py
  clang-refactor/modules/CMakeLists.txt
  clang-refactor/modules/rename/CMakeLists.txt
  clang-refactor/modules/rename/RenamingAction.cpp
  clang-refactor/modules/rename/RenamingAction.h
  clang-rename/USRFindingAction.h
  clang-rename/tool/CMakeLists.txt
  clang-rename/tool/ClangRename.cpp
  docs/clang-refactor/index.rst
  docs/clang-refactor/rename.rst
  docs/clang-rename.rst
  docs/index.rst
  test/CMakeLists.txt
  test/clang-refactor/rename/ClassAsTemplateArgument.cpp
  test/clang-refactor/rename/ClassFindByName.cpp
  test/clang-refactor/rename/ClassReplacements.cpp
  test/clang-refactor/rename/ClassSimpleRenaming.cpp
  test/clang-refactor/rename/ClassTestMulti.cpp
  test/clang-refactor/rename/ClassTestMultiByName.cpp
  test/clang-refactor/rename/ClassTestMultiByNameYAML.cpp
  test/clang-refactor/rename/ComplexFunctionOverride.cpp
  test/clang-refactor/rename/ComplicatedClassType.cpp
  test/clang-refactor/rename/Ctor.cpp
  test/clang-refactor/rename/CtorInitializer.cpp
  test/clang-refactor/rename/DeclRefExpr.cpp
  test/clang-refactor/rename/Field.cpp
  test/clang-refactor/rename/FunctionMacro.cpp
  test/clang-refactor/rename/FunctionOverride.cpp
  test/clang-refactor/rename/FunctionWithClassFindByName.cpp
  test/clang-refactor/rename/InvalidNewName.cpp
  test/clang-refactor/rename/MemberExprMacro.cpp
  test/clang-refactor/rename/Namespace.cpp
  test/clang-refactor/rename/NoNewName.cpp
  test/clang-refactor/rename/QualifiedNameNotFound.cpp
  test/clang-refactor/rename/TemplateClassInstantiation.cpp
  test/clang-refactor/rename/TemplateTypename.cpp
  test/clang-refactor/rename/UserDefinedConversion.cpp
  test/clang-refactor/rename/Variable.cpp
  test/clang-refactor/rename/VariableMacro.cpp
  test/clang-rename/ClassTestMultiByName.cpp
  test/clang-rename/ClassTestMultiByNameYAML.cpp
  test/clang-rename/InvalidNewName.cpp
  test/clang-rename/InvalidOldName.cpp
  test/clang-rename/NoNewName.cpp
  unittests/CMakeLists.txt
  unittests/clang-rename/CMakeLists.txt
  unittests/clang-rename/USRLocFindingTest.cpp

Index: unittests/clang-rename/USRLocFindingTest.cpp
===
--- unittests/clang-rename/USRLocFindingTest.cpp
+++ unittests/clang-rename/USRLocFindingTest.cpp
@@ -1,83 +0,0 @@
-#include "USRFindingAction.h"
-#include "clang/Tooling/Tooling.h"
-#include "gtest/gtest.h"
-#include 
-#include 
-#include 
-
-namespace clang {
-namespace rename {
-namespace test {
-
-// Determines if the symbol group invariants hold. To recap, those invariants
-// are:
-//  (1) All symbols in the same symbol group share the same USR.
-//  (2) Two symbols from two different groups do not share the same USR.
-static void testOffsetGroups(const char *Code,
- const std::vector Groups) {
-  std::set AllUSRs, CurrUSR;
-
-  for (const auto  : Groups) {
-// Groups the invariants do not hold then the value of USR is also invalid,
-// but at that point the test has already failed and USR ceases to be
-// useful.
-std::string USR;
-for (const auto  : Group) {
-  USRFindingAction Action(Offset, std::string());
-  auto Factory = tooling::newFrontendActionFactory();
-  EXPECT_TRUE(tooling::runToolOnCode(Factory->create(), Code));
-  const auto  = Action.getUSRs();
-  EXPECT_EQ(1u, USRs.size());
-  USR = USRs[0];
-  CurrUSR.insert(USR);
-}
-EXPECT_EQ(1u, CurrUSR.size());
-CurrUSR.clear();
-AllUSRs.insert(USR);
-  }
-
-  EXPECT_EQ(Groups.size(), AllUSRs.size());
-}
-
-
-TEST(USRLocFinding, FindsVarUSR) {
-  const char VarTest[] = "\n\
-namespace A {\n\
-int foo;\n\
-}\n\
-int foo;\n\
-int bar = foo;\n\
-int baz = A::foo;\n\
-void fun1() {\n\
-  struct {\n\
-int foo;\n\
-  } b = { 100 };\n\
-  int foo = 100;\n\
-  baz = foo;\n\
-  {\n\
-extern int foo;\n\
-baz = foo;\n\
-foo = A::foo + baz;\n\
-A::foo = b.foo;\n\
-  }\n\
- foo = b.foo;\n\
-}\n";
-  std::vector VarTestOffsets(3);
-  VarTestOffsets[0].push_back(19);
-

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-02 Thread Kirill Bobyrev via cfe-commits

omtcyfz added a comment.

In https://reviews.llvm.org/D24192#532981, @ioeric wrote:

> - You mentioned a design doc in the summary; maybe also include a link to it?

Done.

> - It would make the review easier if you could separate the migration of 
> clang-rename into another patch...I think clang-refactor is really the 
> interesting part here. And maybe also add a small dummy sub-module as an 
> example demonstrating how to add a new sub-module, which I believe will be 
> helpful for future developers. (See also tool-template 
>  
> which exists for the same purpose.)

I am not against it, but I do not think that an empty tool, which just 
introduces the binary without any code would be reasonable. This is basically 
why I wanted it to be all-in-place.

The whole changelist doesn't introduce anything really new, so I assume it 
might be fine. But if you are not okay with it I can split.

Comment at: TemplatedClassFunction.cpp:1
@@ +1,2 @@
+template 
+class A {

ioeric wrote:
> What is this file for?
This is a test. It is the reason clang-refactor rename fails :)

https://reviews.llvm.org/D24192

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

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-02 Thread Eric Liu via cfe-commits

ioeric added a comment.

- You mentioned a design doc in the summary; maybe also include a link to it?
- It would make the review easier if you could separate the migration of 
clang-rename into another patch...I think clang-refactor is really the 
interesting part here. And maybe also add a small dummy sub-module as an 
example demonstrating how to add a new sub-module, which I believe will be 
helpful for future developers. (See also tool-template 
 
which exists for the same purpose.)



Comment at: TemplatedClassFunction.cpp:1
@@ +1,2 @@
+template 
+class A {

What is this file for?


https://reviews.llvm.org/D24192



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

[PATCH] D24192: [clang-refactor] introducing clang-refactor

2016-09-02 Thread Kirill Bobyrev via cfe-commits

omtcyfz created this revision.
omtcyfz added reviewers: alexfh, klimek, ioeric, bkramer.
omtcyfz added a subscriber: cfe-commits.

This patch basically creates `clang-refactor` binary and merges both 
`clang-rename rename-at` and `clang-rename rename-at` a.k.a. `clang-rename` 
into `clang-refactor rename` subcommand.

All USR processing routine is moved to `USREngine`, which should be reused by 
future potential submodules.

`clang-refactor` (as opposed to `clang-rename`) now uses LLVM policy about 
brackets around single line statements in `if`/`while`/...

Editor integrations are moved with minor changes, i.e. they are just named 
differently now.

Docs are updated.

The tool itself doesn't have to be perfect in the first iteration, so I would 
be really happy to push this one early enough so that everyone could start 
building modules on top of it. As soon as this one is landed more refactoring 
and cleanup patches are welcome. This also isn't about "the one true way how 
clang-refactor has to be designed". Thus said, consider this version of 
clang-refactor to be highly experimental.

https://reviews.llvm.org/D24192

Files:
  CMakeLists.txt
  TemplatedClassFunction.cpp
  clang-refactor/CMakeLists.txt
  clang-refactor/USREngine/CMakeLists.txt
  clang-refactor/USREngine/USRFinder.cpp
  clang-refactor/USREngine/USRFinder.h
  clang-refactor/USREngine/USRFindingAction.cpp
  clang-refactor/USREngine/USRFindingAction.h
  clang-refactor/USREngine/USRLocFinder.cpp
  clang-refactor/USREngine/USRLocFinder.h
  clang-refactor/driver/CMakeLists.txt
  clang-refactor/driver/ClangRefactorOptions.h
  clang-refactor/driver/Driver.cpp
  clang-refactor/driver/Rename.h
  clang-refactor/editor-integrations/CMakeLists.txt
  clang-refactor/editor-integrations/clang-refactor-rename.el
  clang-refactor/editor-integrations/clang-refactor-rename.py
  clang-refactor/modules/CMakeLists.txt
  clang-refactor/modules/rename/CMakeLists.txt
  clang-refactor/modules/rename/RenamingAction.cpp
  clang-refactor/modules/rename/RenamingAction.h
  clang-rename/CMakeLists.txt
  clang-rename/RenamingAction.cpp
  clang-rename/RenamingAction.h
  clang-rename/USRFinder.cpp
  clang-rename/USRFinder.h
  clang-rename/USRFindingAction.cpp
  clang-rename/USRFindingAction.h
  clang-rename/USRLocFinder.cpp
  clang-rename/USRLocFinder.h
  clang-rename/tool/CMakeLists.txt
  clang-rename/tool/ClangRename.cpp
  clang-rename/tool/clang-rename.el
  clang-rename/tool/clang-rename.py
  docs/clang-refactor/index.rst
  docs/clang-refactor/rename.rst
  docs/clang-rename.rst
  docs/index.rst
  test/CMakeLists.txt
  test/clang-refactor/rename/ClassAsTemplateArgument.cpp
  test/clang-refactor/rename/ClassFindByName.cpp
  test/clang-refactor/rename/ClassReplacements.cpp
  test/clang-refactor/rename/ClassSimpleRenaming.cpp
  test/clang-refactor/rename/ClassTestMulti.cpp
  test/clang-refactor/rename/ClassTestMultiByName.cpp
  test/clang-refactor/rename/ClassTestMultiByNameYAML.cpp
  test/clang-refactor/rename/ComplexFunctionOverride.cpp
  test/clang-refactor/rename/ComplicatedClassType.cpp
  test/clang-refactor/rename/Ctor.cpp
  test/clang-refactor/rename/CtorInitializer.cpp
  test/clang-refactor/rename/DeclRefExpr.cpp
  test/clang-refactor/rename/Field.cpp
  test/clang-refactor/rename/FunctionMacro.cpp
  test/clang-refactor/rename/FunctionOverride.cpp
  test/clang-refactor/rename/FunctionWithClassFindByName.cpp
  test/clang-refactor/rename/Inputs/ClassTestMultiByNameYAMLRenameAll.yaml
  test/clang-refactor/rename/Inputs/ClassTestMultiByNameYAMLRenameAt.yaml
  test/clang-refactor/rename/InvalidNewName.cpp
  test/clang-refactor/rename/MemberExprMacro.cpp
  test/clang-refactor/rename/Namespace.cpp
  test/clang-refactor/rename/NoNewName.cpp
  test/clang-refactor/rename/QualifiedNameNotFound.cpp
  test/clang-refactor/rename/TemplateClassInstantiation.cpp
  test/clang-refactor/rename/TemplateTypename.cpp
  test/clang-refactor/rename/UserDefinedConversion.cpp
  test/clang-refactor/rename/Variable.cpp
  test/clang-refactor/rename/VariableMacro.cpp
  test/clang-rename/ClassAsTemplateArgument.cpp
  test/clang-rename/ClassFindByName.cpp
  test/clang-rename/ClassReplacements.cpp
  test/clang-rename/ClassSimpleRenaming.cpp
  test/clang-rename/ClassTestMulti.cpp
  test/clang-rename/ClassTestMultiByName.cpp
  test/clang-rename/ClassTestMultiByNameYAML.cpp
  test/clang-rename/ComplexFunctionOverride.cpp
  test/clang-rename/ComplicatedClassType.cpp
  test/clang-rename/Ctor.cpp
  test/clang-rename/CtorInitializer.cpp
  test/clang-rename/DeclRefExpr.cpp
  test/clang-rename/Field.cpp
  test/clang-rename/FunctionMacro.cpp
  test/clang-rename/FunctionOverride.cpp
  test/clang-rename/FunctionWithClassFindByName.cpp
  test/clang-rename/Inputs/ClassTestMultiByNameYAMLRenameAll.yaml
  test/clang-rename/Inputs/ClassTestMultiByNameYAMLRenameAt.yaml
  test/clang-rename/InvalidNewName.cpp
  test/clang-rename/InvalidOldName.cpp
  test/clang-rename/MemberExprMacro.cpp
  test/clang-rename/Namespace.cpp

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

Re: [PATCH] D24192: [clang-refactor] introducing clang-refactor

[PATCH] D24192: [clang-refactor] introducing clang-refactor

26 matches

Site Navigation

Mail list logo

Footer information