[PATCH] D83419: [clangd] Add error() function for creating formatv-style llvm::Errors. NFC

Thu Jul 9 08:34:46 PDT 2020

sammccall added a comment.

In D83419#2141433 <https://reviews.llvm.org/D83419#2141433>, @hokein wrote:

> > Sorry, I should give some reasons here:
>
> These are sensible reasons. My only (not a strong) concern is that "error" is quite special, we need to be careful to choose it -- note that there is an `error` function in glibc which is used for error-reporting.

Yeah. This is also true of `log` which is is a c standard library function. I've seen slightly funny diagnostics when the header is missing, but no worse than that. It's not possible to actually call the wrong function here - neither of the first two parameters of glibc `error()` can be a string.

> maybe there are other better names (`stringError`?) other than `createError`/`makeError`.

Maybe, it's hard to know what's important enough to include in the name. Neither the fact that the implementation stores a string, or that we're allocating an object seem to qualify to me.
Maybe something to do with the formatting like `errorV` or `fmtError` or something but they seem pretty ugly. The difficulty of being part of the `log` family, which doesn't have suffixes...

>> I guess makeError is still better than the status quo, but not enough to feel motivated to clean this up right now. Maybe someone else wants to pick this up?
> 
> unless you strongly insist on `error` name, I'm happy to help with (looks like it's just a low-hanging rename fruit). I think this is a nice cleanup, we should make it happen (either using `error` or other names).

Sure. I'm probably letting the perfect be the enemy of the good here, still sad about the json::Value::asInt() -> json::Value::getAsInt() thing...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83419