This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by sammccall on Jul 8 2020, 12:56 PM.

Details

Summary

This is considerably terser than the makeStringError and friends, and
avoids verbosity cliffs that discourage adding log information.

It follows the syntax used in log/elog/vlog/dlog that have been successful.

The main caveats are:

  • it's strictly out-of-place in logger.h, though kind of fits thematically and in implementation
  • it claims the "error" identifier, which seems a bit too opinionated to put higher up in llvm

I've updated some users of StringError mostly at random - there are lots
more mechanical changes but I'd like to get this reviewed before making
them all.

Diff Detail

Event Timeline

sammccall created this revision.Jul 8 2020, 12:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2020, 12:56 PM
sammccall updated this revision to Diff 276531.Jul 8 2020, 1:00 PM

Move a few comments around.

I'm not a huge fan of using error, maybe createError, its used in many other parts of llvm, typically with static linkage.
Its definitely out of place in Logger, but there is no where else better for it to live short of a new file which seems overkill.

I'm not a huge fan of using error, maybe createError,
Its definitely out of place in Logger, but there is no where else better for it to live short of a new file which seems overkill.

Yeah, the naming is definitely the hardest part to be sure of here.
It's an extremely commonly used function: 75 references to StringError + another 20 or so references to local helpers.
This puts it in the same class as log (95), vlog (50), elog(85).
It's also similar in that it is cross cutting and has the same format string + args shape (so wrapping affects it similarly).

its used in many other parts of llvm, typically with static linkage.

Yeah, there's some precedent, though not overwhelming consensus. Outside clangd I count 10 createErrors, 7 make_string_errors, 4 errors, 1 makeError, 1 makeStringError, and the canonical makeStringError and make_error.

But I think this is just a place where wider LLVM is making different tradeoffs:

  • for good reasons: many errors are diagnostics or don't need to be carefully routed back to a specific request
  • for bad reasons: e.g. compliance with the "verb phrase" policy that's poorly observed and itself seems like a sad accident of history: http://lists.llvm.org/pipermail/llvm-dev/2018-May/thread.html#123547
  • llvm::Error itself seems like a pretty strong argument that existing practice doesn't guarantee good ergonomics

I'm not a huge fan of using error, maybe createError,
Its definitely out of place in Logger, but there is no where else better for it to live short of a new file which seems overkill.

Yeah, the naming is definitely the hardest part to be sure of here.
It's an extremely commonly used function: 75 references to StringError + another 20 or so references to local helpers.
This puts it in the same class as log (95), vlog (50), elog(85).
It's also similar in that it is cross cutting and has the same format string + args shape (so wrapping affects it similarly).

while the name error aligns well with elog/vlog, I'm not in favor of it. I think the key point is that error creates an Error object, and this object must be checked before destructing. error name makes it look like a print function, I'd prefer to add a verb to the name, e.g. makeError

its used in many other parts of llvm, typically with static linkage.

Yeah, there's some precedent, though not overwhelming consensus. Outside clangd I count 10 createErrors, 7 make_string_errors, 4 errors, 1 makeError, 1 makeStringError, and the canonical makeStringError and make_error.

But I think this is just a place where wider LLVM is making different tradeoffs:

  • for good reasons: many errors are diagnostics or don't need to be carefully routed back to a specific request
  • for bad reasons: e.g. compliance with the "verb phrase" policy that's poorly observed and itself seems like a sad accident of history: http://lists.llvm.org/pipermail/llvm-dev/2018-May/thread.html#123547
  • llvm::Error itself seems like a pretty strong argument that existing practice doesn't guarantee good ergonomics

it would be nicer if we could offer this across llvm-projects, but agree that logger.cc seems to be the best place to place it so far.

Honestly, I really dislike names in the style of makeError/createError.
My favorite name for this would be Error (i.e. a constructor) but that would involve redesigning it (which is a good idea, but an impossible yak-shave).
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?

I think the key point is that error creates an Error object, and this object must be checked before destructing. error name makes it look like a print function, I'd prefer to add a verb to the name, e.g. makeError

This is exactly backwards IMO - a function used for its value (like this one) doesn't need a verb, a function used for its side-effects (like log) does.
https://swift.org/documentation/api-design-guidelines/#strive-for-fluent-usage seems pretty well-thought-out to me.
LLVM guidelines don't reflect this, but I haven't been able to find anyone making a positive case for them beyond legacy and inertia.

Honestly, I really dislike names in the style of makeError/createError.

Sorry, I should give some reasons here:

  • not discoverable (error is marginally better)
  • emphasis on allocation rather than how you're using the error (error is much better)
  • forms part of a cluster of hard to remember names createX vs makeX vs make_x (error is much better)
  • longer, both in characters and words (error is better)

It's just an aesthetic irritation but really this is just an aesthetic cleanup.

hokein added a comment.Jul 9 2020, 4:57 AM

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.

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

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).

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...

hokein accepted this revision.Jul 9 2020, 11:56 PM

anyway, I'm fine with this change, feel free to submit it as-is (or do any refinements), don't really want the naming to block this.

This revision is now accepted and ready to land.Jul 9 2020, 11:56 PM
This revision was landed with ongoing or failed builds.Sep 14 2020, 1:53 AM
This revision was automatically updated to reflect the committed changes.