[PATCH] Modularize assistant mode

[John Thompson]

  Thanks!  I'll post an updated revision.


================
Comment at: modularize/ModuleAssistant.h:10-11
@@ +9,4 @@
+///
+/// \file
+/// \brief Module map generation classes and functions.
+///
----------------
Sean Silva wrote:
> This is really unhelpful. At least give the reader a little guidance.
Got it.

================
Comment at: modularize/ModuleAssistant.h:32
@@ +31,3 @@
+  createModuleMap(llvm::StringRef ModuleMapPath,
+                  llvm::SmallVectorImpl<std::string> &HeaderFileNames,
+                  DependencyMap &Dependencies, llvm::StringRef HeaderPrefix,
----------------
Sean Silva wrote:
> ArrayRef is probably better unless you plan on appending/modifying the SmallVector.
Got it.

================
Comment at: modularize/ModuleAssistant.h:33
@@ +32,3 @@
+                  llvm::SmallVectorImpl<std::string> &HeaderFileNames,
+                  DependencyMap &Dependencies, llvm::StringRef HeaderPrefix,
+                  llvm::StringRef RootModuleName) = 0;
----------------
Sean Silva wrote:
> Where is this `DependencyMap` type coming from? It doesn't seem to be in any of the #includes?
I added a Modularize.h file to hold definitions common to more than one file, like this one.

================
Comment at: modularize/ModuleAssistant.h:25-36
@@ +24,14 @@
+/// \brief Provides the top-level logic for generating the module map.
+class ModuleAssistant {
+public:
+  // Create and return an object implementing the assistant.
+  static ModuleAssistant *createModuleAssistant();
+  // Create the module map file.
+  virtual bool
+  createModuleMap(llvm::StringRef ModuleMapPath,
+                  llvm::SmallVectorImpl<std::string> &HeaderFileNames,
+                  DependencyMap &Dependencies, llvm::StringRef HeaderPrefix,
+                  llvm::StringRef RootModuleName) = 0;
+};
+}
+
----------------
Sean Silva wrote:
> Do you expect to have multiple "module assistant" implementations? Would just a single free function
> 
>   bool
>   createModuleMap(llvm::StringRef ModuleMapPath,
>                   llvm::SmallVectorImpl<std::string> &HeaderFileNames,
>                   DependencyMap &Dependencies, llvm::StringRef HeaderPrefix,
>                   llvm::StringRef RootModuleName)
> 
> be sufficient for now?
Got it.  Made a free function, though it relays to an implmentation.


================
Comment at: modularize/ModuleAssistant.cpp:39-40
@@ +38,4 @@
+#include <vector>
+
+namespace Modularize {
+
----------------
Sean Silva wrote:
> I don't think I've ever a namespace explicitly opened in a .cpp file in the LLVM codebase. Usually there is just a "using namespace Modularize", which works when you then define `Module::foo`.
> 
> All classes that are actually declared in the .cpp file (e.g. `class Module` and `class ModuleAssistantImpl` below) are only visible in this .cpp file and should be enclosed in an anonymous namespace <http://llvm.org/docs/CodingStandards.html#anonymous-namespaces>. 
Wow, I've been writing C++ for 24 years, and I never knew this about namespaces.  Intuitively it seems wrong that you can implement class members outside of the namespace, but I bow to convention.

I put the internals in an anonymous namespace.  It seems the conventions say that anonymous namespaces are only for small private chunks, but perhaps this is small enough.

================
Comment at: modularize/ModuleAssistant.cpp:95
@@ +94,3 @@
+
+#define INDENT std::string(Indent * 2, ' ')
+
----------------
Sean Silva wrote:
> Ew. Can you use raw_ostream::indent to achieve this more cleanly `OS.indent(...) << ...`? Or maybe a member function `getIndent()`? 
Got it.

================
Comment at: modularize/ModuleAssistant.cpp:98-99
@@ +97,4 @@
+// Write a module hierarchy to the given output stream.
+bool Module::Output(llvm::raw_fd_ostream &OS, int Indent) {
+  // If this is not the nameless root module, start a module definition.
+  if (Name.size() != 0) {
----------------
Sean Silva wrote:
> Method names are not capitalized <http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly>
Got it.

================
Comment at: modularize/ModuleAssistant.cpp:142
@@ +141,3 @@
+  }
+  return NULL;
+}
----------------
Sean Silva wrote:
> LLVM style uses 0 for the null pointer. E.g. <http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code>. Of course, once we have C++11, nullptr is the obvious thing.
Got it.

================
Comment at: modularize/ModuleAssistant.cpp:152-158
@@ +151,9 @@
+//
+// 1. Sets up for writing the module map output file using the
+//    "tool_output_file" class from LLVM.
+// 2. Builds an internal tree structure of "Module" objects to represent
+//    the modules, based on the header list input.
+// 3. Writes the module map file by walking the internal module tree by
+//    calling the "output" member of the root module.
+// 4. Finalizes the output.
+bool ModuleAssistantImpl::createModuleMap(
----------------
Sean Silva wrote:
> 1-3 here are really helpful comments. 4 is not as helpful; can you improve 4?
Got it.

================
Comment at: modularize/ModuleAssistant.cpp:204-207
@@ +203,6 @@
+       I != E; ++I) {
+    llvm::StringRef Path = *I;
+    // Add as a module.
+    if (!addModuleDescription(Path, HeaderPrefix, Dependencies))
+      return false;
+  }
----------------
Sean Silva wrote:
> can fold this to be just `if (!addModuleDescription(*I, ...`
Got it.

================
Comment at: modularize/ModuleAssistant.cpp:228-230
@@ +227,5 @@
+  if (Count != 0) {
+    llvm::errs() << "warning: " << FilePath
+                 << " has dependents, meaning module.map won't compile."
+                 << "  Ignoring this header.\n";
+    return true;
----------------
Sean Silva wrote:
> Will the user understand what "dependents" means in this context? I sure don't.
Got it.

================
Comment at: modularize/ModuleAssistant.cpp:265-266
@@ +264,4 @@
+  std::string SafeName = MightBeReservedName;
+  int Index;
+  for (Index = 0; ReservedNames[Index] != NULL; ++Index) {
+    if (MightBeReservedName == ReservedNames[Index]) {
----------------
Sean Silva wrote:
> for (int Index = ...
Got it.

================
Comment at: modularize/ModuleAssistant.cpp:328-330
@@ +327,5 @@
+
+  // Clean up tool output buffer/manager.
+  delete Out;
+  Out = NULL;
+
----------------
Sean Silva wrote:
> Ew. Can you use OwningPtr to capture these lifetime semantics? <http://llvm.org/docs/doxygen/html/classllvm_1_1OwningPtr.html>
Got it.


http://llvm-reviews.chandlerc.com/D1891