[PATCH] D32865: [llvm-dwarfdump] - Print an error message if section decompression failed.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Thanks for review Adrian ! My answers inline.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:313
 
+  Error maybeDecompress(const object::SectionRef &Sec, StringRef Name,
+                        StringRef &Data);
----------------
aprantl wrote:
> I would remove the "maybe" here.
It can be a bit confusing as "decompress" looks like we always do that but this method do nothing if content is uncompressed.
Looking in other LLVM code, maybeDoSomething looks to be used naming for such purposes. 
I can't say I like it though, but not sure what can be some different good naming for functionality like "decompess data only if it is compressed".

One way would be to rename it to "decompress" but wrap in check like:
```
 if (Decompressor::isCompressed(Sec)) {
     if (auto Err = decompress(Section, name, data)) {
        errs() << "error: failed to decompress '" + name + "', " +
                    toString(std::move(Err))
             << '\n';
      continue;
}
```
but that would expose Decompressor outside decompress() method and looks not better IMO.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:948
+
+  SmallString<32> Out;
+  if (auto Err = Decompressor->decompress(Out))
----------------
aprantl wrote:
> Is a section likely to be < 32 bytes?
I don't think so. But that is a part of original code, I just moved this line and would not do any changes
in this patch for it.

32 probably can be a good size estimation for testcases we have around, though not sure it is worth to rely on them.

One more thing I just noticed. 'Out' is used for decompress() call which has next interface:
```
Error Decompressor::decompress(SmallString<32> &Out)
```
so changing it needs update of API. I think it is probably reasonable to make it be more generic like
```
Error Decompressor::decompress(SmallVectorImpl<char> &Out)
```


https://reviews.llvm.org/D32865