I did an audit in Phabricator but I guess those don't get sent to the cfe-dev list. Here's what I said:
I think I would have liked to see more review on this latest modularize tests patch (I'm referring to modularize_tests_4.patch attached in the thread "modularize, modularize tests - please review"). It's certainly not the obvious fix I think Sean thought you were talking about. Inline Comments: /clang-tools-extra/trunk/modularize/Modularize.cpp 355 Could you run clang-format on these lines. Something about them doesn't quite look right. clang-format might not handle the string constants nicely. You may find you have to re-break the lines after the format. 378 LLVM coding style says all new code for reading files should use llvm::MemoryBuffer interface. I don't have much experience with that but there's a general aversion to iostream stuff. 380 .c_str() is not necessary. 400 Redundant use of llvm:: given the using directive. 368 .c_str() not necessary. 21 Should mention that the input file can have lines commented out if character 0 on the line is a '#'. 27 perhaps clearer: "...relative to the directory containing the header list file". 90 Now that you've added this using directive, can you remove all the redundant uses of llvm:: in the code below. -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of John Thompson Sent: Monday, March 25, 2013 9:18 PM To: [email protected] Subject: [clang-tools-extra] r177959 - Revised to use file list path as reference path, or path provide by new -prefix option. Revised to use LLVM option parser. Author: jtsoftware Date: Mon Mar 25 20:17:48 2013 New Revision: 177959 URL: http://llvm.org/viewvc/llvm-project?rev=177959&view=rev Log: Revised to use file list path as reference path, or path provide by new -prefix option. Revised to use LLVM option parser. Modified: clang-tools-extra/trunk/modularize/Modularize.cpp Modified: clang-tools-extra/trunk/modularize/Modularize.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/modularize/Modularize.cpp?rev=177959&r1=177958&r2=177959&view=diff ============================================================================== --- clang-tools-extra/trunk/modularize/Modularize.cpp (original) +++ clang-tools-extra/trunk/modularize/Modularize.cpp Mon Mar 25 +++ 20:17:48 2013 @@ -19,7 +19,13 @@ // newline-separated list of headers to check with respect to each other. // Modularize also accepts regular front-end arguments. // -// Usage: modularize (include-files_list) [(front-end-options) ...] +// Usage: modularize [-prefix (optional header path prefix)] \ +// (include-files_list) [(front-end-options) ...] +// +// Note that unless a "-prefex (header path)" option is specified, // +non-absolute file paths in the header list file will be relative // to +the header list file directory. Use -prefix to specify a different // +directory. // // Modularize will do normal parsing, reporting normal errors and warnings, // but will also report special error messages like the following: @@ -60,7 +66,9 @@ //===----------------------------------------------------------------------===// #include "llvm/Config/config.h" +#include "llvm/Support/CommandLine.h" #include "llvm/Support/FileSystem.h" +#include "llvm/Support/Path.h" #include "llvm/ADT/StringRef.h" #include "clang/Basic/SourceManager.h" #include "clang/Lex/Preprocessor.h" @@ -79,7 +87,7 @@ using namespace clang::tooling; using namespace clang; -using llvm::StringRef; +using namespace llvm; // FIXME: The Location class seems to be something that we might // want to design to be applicable to a wider range of tools, and stick it @@ -331,43 +339,68 @@ private: EntityMap &Entities; }; +// Option to specify a file name for a list of header files to check. +cl::opt<std::string> ListFileName(cl::Positional, + cl::desc("<name of file containing list of headers to check>")); + +// Collect all other arguments, which will be passed to the front end. +cl::list<std::string> CC1Arguments(cl::ConsumeAfter, + cl::desc("<arguments to be passed to front end>...")); + +// Option to specify a prefix to be prepended to the header names. +cl::opt<std::string> HeaderPrefix("prefix", cl::init(""), + cl::desc("Prepend header file paths with this prefix." + " If not specified," + " the files are considered to be relative to the header list +file.")); + int main(int argc, const char **argv) { - // Figure out command-line arguments. - if (argc < 2) { - llvm::errs() << "Usage: modularize <file containing header names> <arguments>\n"; - return 1; - } - + + // This causes options to be parsed. + cl::ParseCommandLineOptions(argc, argv, "modularize.\n"); + + // No go if we have no header list file. + if (ListFileName.size() == 0) { + cl::PrintHelpMessage(); + return -1; + } + + // By default, use the path component of the list file name. + SmallString<256> HeaderDirectory(ListFileName.c_str()); + sys::path::remove_filename(HeaderDirectory); + + // Get the prefix if we have one. + if (HeaderPrefix.size() != 0) + HeaderDirectory = HeaderPrefix; + // Load the list of headers. - std::string File = argv[1]; llvm::SmallVector<std::string, 8> Headers; { - std::ifstream In(File.c_str()); + std::ifstream In(ListFileName.c_str()); if (!In) { - llvm::errs() << "Unable to open header list file \"" << File.c_str() << "\"\n"; + llvm::errs() << "Unable to open header list file \"" << + ListFileName.c_str() << "\"\n"; return 2; } - std::string Line; while (std::getline(In, Line)) { if (Line.empty() || Line[0] == '#') continue; - - Headers.push_back(Line); + SmallString<256> headerFileName; + if (sys::path::is_absolute(Line)) + headerFileName = Line; + else { + headerFileName = HeaderDirectory; + sys::path::append(headerFileName, Line); + } + Headers.push_back(headerFileName.c_str()); } } - + // Create the compilation database. + SmallString<256> PathBuf; + llvm::sys::fs::current_path(PathBuf); llvm::OwningPtr<CompilationDatabase> Compilations; - { - std::vector<std::string> Arguments; - for (int I = 2; I < argc; ++I) - Arguments.push_back(argv[I]); - SmallString<256> PathBuf; - llvm::sys::fs::current_path(PathBuf); - Compilations.reset(new FixedCompilationDatabase(Twine(PathBuf), Arguments)); - } - + Compilations.reset(new FixedCompilationDatabase(Twine(PathBuf), + CC1Arguments)); + // Parse all of the headers, detecting duplicates. EntityMap Entities; ClangTool Tool(*Compilations, Headers); _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
