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

George Rimar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri May 5 09:29:09 PDT 2017


grimar added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFContext.h:313
 
+  Error maybeDecompress(const object::SectionRef &Sec, StringRef Name,
+                        StringRef &Data);
----------------
aprantl wrote:
> grimar wrote:
> > 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.
> Okay, thanks for the explanation it wasn't clear to me that it is also expected to be used with uncompressed input. Could you please add a doxygen comment to the function explaining that it decompresses the section or otherwise returns its contents verbatim?
> 
r302249. Hope it looks ok :]


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:948
+
+  SmallString<32> Out;
+  if (auto Err = Decompressor->decompress(Out))
----------------
aprantl wrote:
> grimar wrote:
> > 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)
> > ```
> Yes, I agree. I would personally prefer a std::vector& here, because it is highly unlikely to ever use the "small" case and the small vector then just adds extra overhead.
I'll try to revisit this place and Decompressor API on next week.


Repository:
  rL LLVM

https://reviews.llvm.org/D32865





More information about the llvm-commits mailing list