[PATCH] D124804: [Object][DX] Parse DXContainer Parts

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: llvm/lib/Object/DXContainer.cpp:34
+template <typename T>
+static Error readValue(StringRef Buffer, const char *P, T &Val) {
+  // Don't read before the beginning or past the end of the file
----------------
beanz wrote:
> kuhar wrote:
> > Can you give `P` a more descriptive name and/or add a function comment?
> > Also, why not return `Expected<T>`
> Mostly just because the usage pattern for `Error` is cleaner in this case, but also it avoids a copy and is in-line able.
> 
> With Expected the code would be:
> ```
> auto ExVal = readValue(…);
> if (!ExVal)
>   return ExVal.takeError();
> Val = *ExVal;
> ```
> 
> Which is a bit less clean to me.
I'd disagree here. Let's look at a callsite and the code that follows:
```
    uint32_t PartOffset;
    if (Error Err = readValue(Data.getBuffer(), Current, PartOffset))
      return Err;
    Current += sizeof(uint32_t);
    if (PartOffset + sizeof(dxbc::PartHeader) > Data.getBufferSize())
      return parseFailed("Part offset points beyond boundary of the file");
    PartOffsets.push_back(PartOffset);
```

In general, I would not say it's immediately that PartOffset is an output parameter. Without seeing the definition, we don't know if passing `PartOffset` to `readValue` will lead to uninitialized reads or not. I find it much more idiomatic to use return values to return values. With `Expected<T>`, the callsite and the surrounding code would look like this:

```
    Expected<uint32_t> PartOffset = readValue(Data.getBuffer(), Current);
    if (Error Err = PartOffset.takeError())
      return Err;
    Current += sizeof(uint32_t);
    if (*PartOffset + sizeof(dxbc::PartHeader) > Data.getBufferSize())
      return parseFailed("Part offset points beyond boundary of the file");
    PartOffsets.push_back(*PartOffset);
```

Here it's very clear to me what is being returned and I would not worry about any uninitialized values, even without checking the implementation of `readValue`. And overall, the number of lines stays the same.

I also typically don't prefix/suffix expected values with anything special, I don't think it improves readability when you can see the variable type. This is also what the google style guide does: https://abseil.io/tips/181.

To be absolutely clear, this is not a big deal IMO either way and feel free to stick with the current API if you strongly prefer it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124804