[PATCH] D53379: GSYM symbolication format

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

Thanks Greg! I came across a few more places that don't need braces but otherwise this looks good style-wise.



================
Comment at: include/llvm/DebugInfo/GSYM/DwarfTransformer.h:37
+    if (auto Binary = getObjectFile(filename)) {
+      return loadDwarf(*Binary.getValue().getBinary());
+    }
----------------
No braces


================
Comment at: include/llvm/DebugInfo/GSYM/DwarfTransformer.h:45
+    if (auto Binary = getObjectFile(filename)) {
+      return loadSymbolTable(*Binary.getValue().getBinary());
+    }
----------------
No braces


================
Comment at: include/llvm/DebugInfo/GSYM/InlineInfo.h:37
+
+  // Decode all InlineInfo from address info data.
+  // Returns true if successful, false if InlineInfo is empty.
----------------
`///` for Doxygen comments?


================
Comment at: include/llvm/DebugInfo/GSYM/LineTable.h:27
+enum LineTableOpCode {
+  LTOC_EndSequence = 0x00,  // End of the line table
+  LTOC_SetFile = 0x01,      // Set LineTableRow.file_idx, don't push a row
----------------
You should considering using `///<` so it becomes a Doxygen comment.


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:73
+    const ArrayRef<uint8_t> MachUUID = MachO->getUuid();
+    if (!MachUUID.empty()) {
+      UUID.assign(MachUUID.data(), MachUUID.data() + MachUUID.size());
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:81
+      Sect.getName(SectName);
+      if (SectName == GNUBuildID) {
+        StringRef BuildIDData;
----------------
You could make this an early break by inverting the condition. Same for the condition below. I think that would improve the readability of this code.


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:109
+    if (DWARFDie SpecParent = GetParentDeclContextDIE(SpecDie)) {
+      return SpecParent;
+    }
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:114
+          Die.getAttributeValueAsReferencedDie(dwarf::DW_AT_abstract_origin)) {
+    if (DWARFDie AbstParent = GetParentDeclContextDIE(AbstDie)) {
+      return AbstParent;
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:127
+  DWARFDie ParentDie = Die.getParent();
+  if (!ParentDie) {
+    return DWARFDie();
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:152
+                          nullptr)) {
+    return LinkageName;
+  }
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:167
+        // in some binaries. This should hurt, so let's do it for C as well
+        Language == dwarf::DW_LANG_C)) {
+    return ShortName.str();
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:174
+  if (ShortName.startswith("_Z") &&
+      (ShortName.contains(".isra.") || ShortName.contains(".part."))) {
+    return ShortName.str();
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:390
+  }
+  for (DWARFDie ChildDie : Die.children()) {
+    handleDie(OS, CUI, ChildDie);
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:406
+  for (const object::SectionRef &Sect : Obj.sections()) {
+    if (Sect.isText()) {
+      const uint64_t Size = Sect.getSize();
----------------
Simplify this with early break.


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:421
+
+  if (!Initialized) {
+    initDataFromObj(Obj);
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:443
+    // its local data.
+    for (const auto &CU : DICtx->compile_units()) {
+      CU->getAbbreviations();
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:448
+    for (const auto &CU : DICtx->compile_units()) {
+      pool.async([&CU]() { CU->getUnitDIE(false /*CUDieOnly*/); });
+    }
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:482
+  using namespace llvm::object;
+  if (!Initialized) {
+    initDataFromObj(Obj);
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/DwarfTransformer.cpp:496
+    if (Expected<StringRef> Name = Sym.getName()) {
+      Gsym.addFunctionInfo(FunctionInfo(addr, size, Gsym.insertString(*Name)));
+    }
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/FileTableCreator.cpp:34
+  if (R.second) { // if newly inserted
+    FileEntries.emplace_back(Entry);
+  }
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/FunctionInfo.cpp:25
+    OS << "Lines:\n";
+    for (const auto &Line : Lines) {
+      Line.dump(OS);
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/GsymReader.cpp:60
+
+std::string Header::getError() const {
+  // TODO: support swapped GSYM files
----------------
I strongly prefer wrapping this in an llvm::StringError. It's a little more verbose but keeps things consistent. 


================
Comment at: lib/DebugInfo/GSYM/GsymReader.cpp:255
+  for (uint32_t I = 0; I < GSYMHeader->NumAddresses; ++I) {
+    dumpAddressInfo(OS, I);
+  }
----------------
No braces


================
Comment at: lib/DebugInfo/GSYM/InlineInfo.cpp:96
+      if (StartAddr <= *LookupAddr && *LookupAddr < EndAddr) {
+        Inline.Ranges.emplace_back(AddressRange(StartAddr, EndAddr));
+      }
----------------
No braces


https://reviews.llvm.org/D53379