[PATCH] D86892: Improve error handling for SmallVector programming errors.

[Geoffrey Martin-Noble via Phabricator via llvm-commits]

GMNGeoffrey added inline comments.


================
Comment at: llvm/include/llvm/ADT/SmallVector.h:276
+                         std::to_string(this->SizeTypeMax()) + ")";
+#ifdef LLVM_ENABLE_EXCEPTIONS
+    throw std::length_error(Reason);
----------------
MaskRay wrote:
> If you want to use `#ifdef LLVM_ENABLE_EXCEPTIONS`
> 
> ```
> #ifdef LLVM_ENABLE_EXCEPTIONS
> #else
> #endif
> ```
You mean

```
#ifdef LLVM_ENABLE_EXCEPTIONS
  throw std::length_error(Reason);
#else
  report_fatal_error(Reason);
#endif
```

?

Why? Shouldn't throwing break us out of the control flow here?


================
Comment at: llvm/lib/Support/ErrorHandling.cpp:175
+  (void)::write(2, Reason, strlen(Reason));
+  (void)::write(2, newline, strlen(newline));
   abort();
----------------
MaskRay wrote:
> GMNGeoffrey wrote:
> > MaskRay wrote:
> > > `(void)::write(2, "\n", 1);`
> > The way I had it seems more explicit about what's going on here (same reason OOMMessage is a separate variable, I assume). It's obviously easier to count bytes in newline, but seems better to let the compiler do it. I figured the compiler should optimize it to the same thing anyway, but fiddling with godbolt, it seems that's not actually the case. The `char[]` is still allocated. This actually applies to the `OOMMessage` variable too. Switching it to `const char*` (or `auto`, which deduces this) makes the compiled instructions the same, and the source code more explicit, I think. https://godbolt.org/z/q5KzhK
> `const char OOMMessage[] = ...`
> 
> * clang performs constant evaluation of strlen while emitting LLVM IR.
> 
> `const char *OOMMessage = ...`
> 
> * clang does not evaluate the length.
> * SimplifyLibCalls.cpp called by instcombine can perform constant evaluation of strlen.
> 
> `char OOMMessage[] = ...`
> 
> * clang does evaluate the length.
> * SimplifyLibCalls.cpp called by instcombine cannot optimize the call.
> * There is a runtime string copy and a runtime call of strlen.
> 
Hmmm adding `const` doesn't seem to change anything: https://godbolt.org/z/19aYME


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86892