[llvm-commits] [llvm] r140722 - in /llvm/trunk/tools: CMakeLists.txt Makefile llvm-size/ llvm-size/CMakeLists.txt llvm-size/Makefile llvm-size/llvm-size.cpp

[Chris Lattner]

On Sep 28, 2011, at 1:57 PM, Michael J. Spencer wrote:

> Author: mspencer
> Date: Wed Sep 28 15:57:46 2011
> New Revision: 140722
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=140722&view=rev
> Log:
> Add llvm-size.

Cool.

> +namespace {
> +  enum OutputFormatTy {berkeley, sysv};
> +  cl::opt<OutputFormatTy>
> +  OutputFormat("format",

Per the coding standards, please use 'static' instead of an anon namespace for data.

> +  if (OutputFormat == sysv) {
> +    // Run two passes over all sections. The first gets the lengths needed for
> +    // formatting the output. The second actually does the output.
> +    std::size_t max_name_len = strlen("section");
> +    int max_size_len = strlen("size");
> +    int max_addr_len = strlen("addr");

These (and the return value of getNumLengthAsString) should be unsigned.

>    // Setup per section format.
> +    fmt << "%-" << max_name_len << "s "
> +        << "%#" << max_size_len << radix_fmt << " "
> +        << "%#" << max_addr_len << radix_fmt << "\n";


This might be nicer with formatted_raw_ostream and the 'indent' method instead of building a format string.

> +static void PrintFileSectionSizes(StringRef file) {
> +  // If file is not stdin, check that it exists.
> +  if (file != "-") {
> +    bool exists;
> +    if (sys::fs::exists(file, exists) || !exists) {
> +      errs() << ToolName << ": '" << file << "': " << "No such file\n";
> +      return;
> +    }
> +  }
> +
> +  OwningPtr<Binary> binary;
> +  if (error_code ec = createBinary(file, binary)) {
> +    errs() << ToolName << ": " << file << ": " << ec.message() << ".\n";
> +    return;
> +  }

Please add doxygen comments to the functions, and comments to the code in general.

> ++
> +  ToolName = argv[0];
> +  if (OutputFormatShort.getNumOccurrences())
> +    OutputFormat = OutputFormatShort;
> +  if (RadixShort.getNumOccurrences())
> +    Radix = int(RadixShort);

'Radix' should be a unsigned.

-Chris