[Support] - Introduce zlib::toString(zlib::Status)

[grimar]

Summary

Helper is useful for universal error messages
when using zlib functionality.
I plan to use it in D28105 or following patch for
better uncompression errors diagnostic output.

[davide]

Can you add (unit)tests for this?

lib/Support/Compression.cpp

23–24

isAvailable() returns always true, so, how can this be ever hit?

[grimar]

lib/Support/Compression.cpp

23–24

I can imagine some code like next:

Status S = zlib::StatusUnsupported;
if (zlib::isAvailable() {
  S = ...; //Do something
}

if (S != zlib::StatusOK)
  printError(zlib::toString(S)

So it looks can be useful to cover this enum value too.

[grimar]

lib/Support/Compression.cpp

23–24

Ah, my example will not work currently, I see now.
I am thinking about making toString() to have single implementation for cases when HAVE_LIBZ and !HAVE_LIBZ.
That way example I showed will work and makes sence.

[grimar]

• Added unittest
• Made toString() to be single implementation for cases when ZLib is available/NA

[mgorny]

unittests/Support/CompressionTest.cpp

70

To be honest, I don't think this test takes makes a lot of sense since it's pretty much a duplicate of the function itself. As far as I can see, rather than catching bugs it's going to get people frustrated when they change one of the messages.

[grimar]

unittests/Support/CompressionTest.cpp

70

That is why I did not add it in inital diff.
I was and still unsure if we should or should not test string values like here.

[davide]

unittests/Support/CompressionTest.cpp

70

What I actually meant is to create test scenarios triggering these errors, not copying the function itself.

[davide]

As an alternative, you can merge the patch and the patch where you plan to use this code together (which would make more sense, IMHO).
In fact, I'm still confused why you splitted this in the first place.

[grimar]

In D28515#644772, @davide wrote:

As an alternative, you can merge the patch and the patch where you plan to use this code together (which would make more sense, IMHO).
In fact, I'm still confused why you splitted this in the first place.

I wanted to use it in LLD. That would mean 2 patches anyways, LLD and this one.

Previously Rafael suggested to return Error for methods of this API instead of status.
I think that is good idea, result will be single patch affecting users of this (Decomressor class, something else probably) and changing API itself in a single patch, just like you suggesting.

[davide]

In D28515#645037, @grimar wrote:

In D28515#644772, @davide wrote:

As an alternative, you can merge the patch and the patch where you plan to use this code together (which would make more sense, IMHO).
In fact, I'm still confused why you splitted this in the first place.

I wanted to use it in LLD. That would mean 2 patches anyways, LLD and this one.

Previously Rafael suggested to return Error for methods of this API instead of status.
I think that is good idea, result will be single patch affecting users of this (Decomressor class, something else probably) and changing API itself in a single patch, just like you suggesting.

Yes, that would be better.

[grimar]

D28684 posted instead.

include/llvm/Support/Compression.h

Status uncompress(StringRef InputBuffer, char *UncompressedBuffer,
                   size_t &UncompressedSize);
 
 Status uncompress(StringRef InputBuffer,
                   SmallVectorImpl<char> &UncompressedBuffer,
                   size_t UncompressedSize);
 
 uint32_t crc32(StringRef Buffer);
 
+StringRef toString(Status Stat);
+
 }  // End of namespace zlib
 
 } // End of namespace llvm
 
 #endif
 

lib/Support/Compression.cpp

 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
 #if LLVM_ENABLE_ZLIB == 1 && HAVE_ZLIB_H
 #include <zlib.h>
 #endif
 
 using namespace llvm;
 
 #if LLVM_ENABLE_ZLIB == 1 && HAVE_LIBZ
 static int encodeZlibCompressionLevel(zlib::CompressionLevel Level) {
   switch (Level) {
     case zlib::NoCompression: return 0;
     case zlib::BestSpeedCompression: return 1;
     case zlib::DefaultCompression: return Z_DEFAULT_COMPRESSION;
     case zlib::BestSizeCompression: return 9;
zlib::Status zlib::uncompress(StringRef InputBuffer,
                               size_t UncompressedSize) {
   return zlib::StatusUnsupported;
 }
 uint32_t zlib::crc32(StringRef Buffer) {
   llvm_unreachable("zlib::crc32 is unavailable");
 }
 #endif
 
+StringRef zlib::toString(zlib::Status Stat) {
+  switch (Stat) {
+  case zlib::StatusOK:
+    return "";
+  case zlib::StatusUnsupported:
+    return "zlib is unavailable";
+  case zlib::StatusOutOfMemory:
+    return "there was not enough memory";
+  case zlib::StatusBufferTooShort:
+    return "there was not enough room in the output buffer";
+  case zlib::StatusInvalidArg:
+    return "invalid input parameter";
+  case zlib::StatusInvalidData:
+    return "data was corrupted or incomplete";
+  default:
+    llvm_unreachable("status unknown");
+  }
+}

unittests/Support/CompressionTest.cpp

 TEST(CompressionTest, ZlibCRC32) {
   EXPECT_EQ(
       0x414FA339U,
       zlib::crc32(StringRef("The quick brown fox jumps over the lazy dog")));
 }
 
 #endif
 
+TEST(CompressionTest, ZlibMessages) {
+  EXPECT_EQ("", zlib::toString(zlib::StatusOK));
+  EXPECT_EQ("zlib is unavailable", zlib::toString(zlib::StatusUnsupported));
+  EXPECT_EQ("there was not enough memory",
+            zlib::toString(zlib::StatusOutOfMemory));
+  EXPECT_EQ("there was not enough room in the output buffer",
+            zlib::toString(zlib::StatusBufferTooShort));
+  EXPECT_EQ("invalid input parameter", zlib::toString(zlib::StatusInvalidArg));
+  EXPECT_EQ("data was corrupted or incomplete",
+            zlib::toString(zlib::StatusInvalidData));
+}
 }