[PATCH] D63104: Add GSYM utility files along with unit tests.

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/DebugInfo/GSYM/FileEntry.h:13
+
+#include <functional>
+#include <stdint.h>
----------------
This doesn't conform to the LLVM #include style: https://llvm.org/docs/CodingStandards.html#include-style


================
Comment at: include/llvm/DebugInfo/GSYM/FileEntry.h:24
+struct FileEntry {
+  // Files in GSYM are contained in FileEntry structs where we split the 
+  // directory and basename into two different strings in the string 
----------------
Should this be a Doxygen comment for this struct?


================
Comment at: include/llvm/DebugInfo/GSYM/FileEntry.h:28
+  // strings and saves space.
+  uint32_t Dir = 0;  ///< String table offset in the string table.
+  uint32_t Base = 0; ///< String table offset in the string table.
----------------
The comment for both is identical. I'd group them like 

```
/// Offsets in the string table.
/// @{
...
/// @}
```


================
Comment at: include/llvm/DebugInfo/GSYM/FunctionInfo.h:26
+struct FunctionInfo {
+  // Function information in GSYM files encodes information for one 
+  // contiguous address range. The name of the function is encoded as
----------------
Same comment.


================
Comment at: include/llvm/DebugInfo/GSYM/FunctionInfo.h:51
+
+  bool isValid() const {
+    // Address and size can be zero and there can be no line entries for a
----------------
Can we make it so that only valid entries are created, for instance by using an Expected<FunctionInfo> wherever it's created? That way we don't need the isValid and maybe even the clear? This is more of a high level comment that applies to the whole diff. 


================
Comment at: include/llvm/DebugInfo/GSYM/FunctionInfo.h:82
+}
+// This sorting will order things consistently by address range first, but then
+// followed by inlining being valid and line tables. We might end up with a
----------------
Doxygen comment?


================
Comment at: include/llvm/DebugInfo/GSYM/InlineInfo.h:24
+struct InlineInfo {
+  // Inline information stores the name of the inline function along with
+  // an array of address ranges. It also stores the call file and call line
----------------
Should this be a Doxygen comment for the struct? Applies for the other classes/structs as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63104/new/

https://reviews.llvm.org/D63104