[PATCH] D67666: GSYM: Add the llvm::gsym::Header header class with tests

[Greg Clayton via Phabricator via llvm-commits]

clayborg marked 2 inline comments as done.
clayborg added inline comments.


================
Comment at: include/llvm/DebugInfo/GSYM/Header.h:90
+  /// \returns True if the header is valid and if the version is supported.
+  bool isValid() const {
+    if (Magic != GSYM_MAGIC)
----------------
aprantl wrote:
> JDevlieghere wrote:
> > Any reason you prefer `isValid` over `operator bool`?
> As usual, I'm going to point out that it may be nicer to have the parser return an Expected<Header> instead of a header that may be invalid.
This is because people will create a Header using information retrieved from another source, like DWARF, breakpad or compiler metadata. Once the header has been populated, it might get emitted. Also, for native endianness, we won't be parsing these, we will just be mmap'ing the data and getting a pointer to the start of the GSYM data. In this case, we will call this function to ensure the GSYM header is good. Note that the encode and decode functions do the right thing. This function will be used by clients that mmap and try to use the data as is.


================
Comment at: include/llvm/DebugInfo/GSYM/Header.h:90
+  /// \returns True if the header is valid and if the version is supported.
+  bool isValid() const {
+    if (Magic != GSYM_MAGIC)
----------------
clayborg wrote:
> aprantl wrote:
> > JDevlieghere wrote:
> > > Any reason you prefer `isValid` over `operator bool`?
> > As usual, I'm going to point out that it may be nicer to have the parser return an Expected<Header> instead of a header that may be invalid.
> This is because people will create a Header using information retrieved from another source, like DWARF, breakpad or compiler metadata. Once the header has been populated, it might get emitted. Also, for native endianness, we won't be parsing these, we will just be mmap'ing the data and getting a pointer to the start of the GSYM data. In this case, we will call this function to ensure the GSYM header is good. Note that the encode and decode functions do the right thing. This function will be used by clients that mmap and try to use the data as is.
> Any reason you prefer isValid over operator bool?

Mostly in case this is used internally inside the class. I hate seeing:

```
if (*this)
```

I would rather see:
```
if (isValid())
```




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

https://reviews.llvm.org/D67666