[llvm-commits] [PATCH] llvm-strings tool

[Dmitri Gribenko]

  A ton of nitpicks...


================
Comment at: llvm-strings/llvm-strings.cpp:1
@@ +1,2 @@
+//===-- llvm-strings.cpp -  find the printable strings in a file ----------===//
+//
----------------
Extra space after the dash.

================
Comment at: llvm-strings/llvm-strings.cpp:10-13
@@ +9,6 @@
+//
+// This program is a utility that works like traditional Unix "strings", that 
+// is, it prints out printable strings in a binary, object, or archive file.
+//
+// "llvm-strings" supports many of the features of GNU "strings", but not all.
+//
----------------
Please add \file and make this comment a Doxygen comment (three slashes).

See include/llvm/Attributes.h for an example.

================
Comment at: llvm-strings/llvm-strings.cpp:43-48
@@ +42,8 @@
+
+/*
+  A string is just a sequence of printable characters 
+  followed by a non-printable character.
+  For each string we find, call the callback.
+  If the callback fails, quit immediately.
+*/
+template <typename PrintFunction>
----------------
Make this a proper Doxygen comment (use line comments with three slashes, format to 80 cols)

================
Comment at: llvm-strings/llvm-strings.cpp:50
@@ +49,3 @@
+template <typename PrintFunction>
+error_code findStringsInBlob(StringRef theData, std::size_t initialOffset,
+                             std::size_t sizeThreshold, bool verbose,
----------------
Variable names should start with an uppercase letter (here and below).

================
Comment at: llvm-strings/llvm-strings.cpp:52
@@ +51,3 @@
+                             std::size_t sizeThreshold, bool verbose,
+                             PrintFunction pf) {
+  error_code RetVal;
----------------
pf -> PF

================
Comment at: llvm-strings/llvm-strings.cpp:99
@@ +98,3 @@
+
+      uint64_t start_of_section;
+      StringRef contents;
----------------
Variable names should be CamelCase.

================
Comment at: llvm-strings/llvm-strings.cpp:174
@@ +173,3 @@
+error_code printFn(std::size_t offset, StringRef str) {
+  // print the offset, if necessary
+  switch (OptionOffsetDisplay) {
----------------
Comments should be sentences (start with a capital letter, end with full stop).

================
Comment at: llvm-strings/llvm-strings.cpp:212-213
@@ +211,4 @@
+
+  if ((RetVal = MemoryBuffer::getFileOrSTDIN(main_fn, aFile)))
+    errs() << "## Can't open file '" << main_fn << "'!" << endl;
+  else {
----------------
Simplify code with early returns.  Just return here, and remove the else {} block and de-indent.

================
Comment at: llvm-strings/llvm-strings.cpp:36-41
@@ +35,8 @@
+
+bool isPrintable(char c) {
+  return isalnum(c) || ispunct(c) ||
+         (isspace(c) && (!iscntrl(c) || c == '\t')) ||
+         (isascii(c) && isprint(c));
+  //  Easy to replace this with a table at some point
+}
+
----------------
We have discussed before that this is locale-dependent.  I think this should eventually be configurable with a command-line option: ASCII-only, locale-dependent, UTF-8.



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