[PATCH] Initial commit for the llvm-dsymutil tool.

Alexey Samsonov vonosmas at gmail.com
Thu Dec 11 15:15:02 PST 2014


LGTM. I have a handful of comments, feel free to submit after addressing them.


================
Comment at: tools/dsymutil/DebugMap.cpp:38
@@ +37,3 @@
+  Entries.reserve(Symbols.getNumItems());
+  for (auto SymIt = Symbols.begin(), End = Symbols.end(); SymIt != End; ++SymIt)
+    Entries.push_back(std::make_pair(SymIt->getKey(), SymIt->getValue()));
----------------
Can you write range-based for loop here instead?

================
Comment at: tools/dsymutil/DebugMap.cpp:42
@@ +41,3 @@
+      Entries.begin(), Entries.end(),
+      [](const Entry &LHS, const Entry &RHS) { return LHS.first < RHS.first; });
+  for (const auto &Sym : Entries) {
----------------
You don't need a comparator here - all keys in StringMap are different, and std::pair<> objects are compared lexicographically by default, which is what you need.

================
Comment at: tools/dsymutil/DebugMap.h:21
@@ +20,3 @@
+//===----------------------------------------------------------------------===//
+#ifndef DSYMUTIL_DEBUGMAP_H
+#define DSYMUTIL_DEBUGMAP_H
----------------
prepend with LLVM_TOOLS_ ?

================
Comment at: tools/dsymutil/DebugMap.h:62
@@ +61,3 @@
+/// }
+class DebugMap {
+  typedef std::vector<std::unique_ptr<DebugMapObject>> ObjectContainer;
----------------
consider wrapping it in one more namespace (like llvm::dsymuitl) (probably not a big deal, though).

================
Comment at: tools/dsymutil/DwarfLinker.cpp:15
@@ +14,3 @@
+
+class DwarfLinker {
+  std::string OutputFilename;
----------------
This class does nothing for now. You can just delete it from this patch, and add it when it will actually do some work.

================
Comment at: tools/dsymutil/MachODebugMapParser.cpp:216
@@ +215,3 @@
+  const MachOObjectFile &Binary = *MainOwningBinary.getBinary();
+  section_iterator Section = Binary.section_end();
+  for (const auto &Sym : Binary.symbols()) {
----------------
Shouldn't this be a local variable defined inside for-loop with no default value? (as it will be initialized by Sym.getSection()) ?

================
Comment at: tools/dsymutil/dsymutil.cpp:34
@@ +33,3 @@
+                                       desc("Specify a path to prepend to the "
+                                            "object file's filenames."),
+                                       value_desc("path"));
----------------
"names of object files"?

================
Comment at: tools/dsymutil/dsymutil.h:16
@@ +15,3 @@
+//===----------------------------------------------------------------------===//
+#ifndef DSYMUTIL_DSYMUTIL_H
+#define DSYMUTIL_DSYMUTIL_H
----------------
LLVM_TOOLS_DSYMUTIL_DSYMUTIL_H

================
Comment at: tools/dsymutil/dsymutil.h:25
@@ +24,3 @@
+
+class DebugMap;
+
----------------
Shouldn't you #include DebugMap.h here to use DebugMap in unique_ptr?

http://reviews.llvm.org/D6242

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list