[PATCH] D130506: [Support] Add llvm::compression::{isAvailable,compress,uncompress}

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/Support/Compression.cpp:46
+  case DebugCompressionType::None:
+    assert(false && "expected a compression type");
+    break;
----------------
MaskRay wrote:
> dblaikie wrote:
> > llvm_unreachable should be preferred over assert false ( https://llvm.org/docs/CodingStandards.html#assert-liberally discusses this)
> Thanks for mentioning the doc. For this place I hesitated while thinking about llvm_unreachable but picked assert because the user can call the function with any arguments and P.Format==DebugCompressionType::None reachable. The function is supposed to be unreachable from call sites, though...
In the same way that a user can call a function that takes a non-null pointer with a null pointer, resulting in an assertion - that's OK. It's an invariant of the function that `P.Format != DebugCompressionType::None`, so it's OK/suitable to use unreachable. (using an assert and then having some code "in case the assert fails" is inconsistent - it's defining behavior when an invariant is violated, but that behavior is then untested and unreliable)

But, yeah, guess we can leave this discussion for now while we resolve the larger design discussions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130506