Do you think that this may benefit in the future from being interactive?
================ Comment at: modularize/ModuleAssistant.cpp:328-330 @@ +327,5 @@ + + // Clean up tool output buffer/manager. + delete Out; + Out = NULL; + ---------------- Ew. Can you use OwningPtr to capture these lifetime semantics? <http://llvm.org/docs/doxygen/html/classllvm_1_1OwningPtr.html> ================ 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]) { ---------------- for (int Index = ... ================ 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; ---------------- Will the user understand what "dependents" means in this context? I sure don't. ================ 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; + } ---------------- can fold this to be just `if (!addModuleDescription(*I, ...` ================ 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( ---------------- 1-3 here are really helpful comments. 4 is not as helpful; can you improve 4? ================ Comment at: modularize/ModuleAssistant.cpp:95 @@ +94,3 @@ + +#define INDENT std::string(Indent * 2, ' ') + ---------------- Ew. Can you use raw_ostream::indent to achieve this more cleanly `OS.indent(...) << ...`? Or maybe a member function `getIndent()`? ================ Comment at: modularize/ModuleAssistant.cpp:32-38 @@ +31,9 @@ + +#include "llvm/ADT/SmallString.h" +#include "llvm/Support/FileSystem.h" +#include "llvm/Support/Path.h" +#include "llvm/Support/ToolOutputFile.h" +#include "Modularize.h" +#include "ModuleAssistant.h" +#include <vector> + ---------------- <http://llvm.org/docs/CodingStandards.html#include-style> (the most "local" headers go first). ================ Comment at: modularize/ModuleAssistant.cpp:39-40 @@ +38,4 @@ +#include <vector> + +namespace Modularize { + ---------------- 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>. ================ 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) { ---------------- Method names are not capitalized <http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly> ================ 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; +}; +} + ---------------- 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? ================ Comment at: modularize/ModuleAssistant.h:29-34 @@ +28,8 @@ + 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; +}; ---------------- Please add a doxygen comment describing each parameter. ================ Comment at: modularize/ModuleAssistant.h:10-11 @@ +9,4 @@ +/// +/// \file +/// \brief Module map generation classes and functions. +/// ---------------- This is really unhelpful. At least give the reader a little guidance. ================ Comment at: modularize/ModuleAssistant.h:33 @@ +32,3 @@ + llvm::SmallVectorImpl<std::string> &HeaderFileNames, + DependencyMap &Dependencies, llvm::StringRef HeaderPrefix, + llvm::StringRef RootModuleName) = 0; ---------------- Where is this `DependencyMap` type coming from? It doesn't seem to be in any of the #includes? ================ Comment at: modularize/ModuleAssistant.h:32 @@ +31,3 @@ + createModuleMap(llvm::StringRef ModuleMapPath, + llvm::SmallVectorImpl<std::string> &HeaderFileNames, + DependencyMap &Dependencies, llvm::StringRef HeaderPrefix, ---------------- ArrayRef is probably better unless you plan on appending/modifying the SmallVector. ================ Comment at: modularize/ModuleAssistant.cpp:142 @@ +141,3 @@ + } + return NULL; +} ---------------- 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. http://llvm-reviews.chandlerc.com/D1891 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
