Page MenuHomePhabricator

Add LLDB_LOG_ERROR (?)
ClosedPublic

Authored by labath on Jan 17 2018, 7:04 AM.

Details

Summary

The difference between this and regular LLDB_LOG is that this one clears
the error object unconditionally. This was inspired by the
ObjectFileELF bug (r322664), where the error object was being cleared
only if logging was enabled.

Of course, one could argue that since our logging is optional, it does
not really qualify as "handling" the error and we should leave it up to
the programmer to explicitly clear it if he really wants to.

It's not really clear to me which position is better. What do you think?

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jan 17 2018, 7:04 AM

I think this is fine, but I'll defer to Zachary.

davide accepted this revision.Jan 17 2018, 9:01 AM

Forgot to click accept.

This revision is now accepted and ready to land.Jan 17 2018, 9:01 AM
clayborg accepted this revision.Jan 17 2018, 10:04 AM

This seems fine as a bit of functionality but I'm worried about the name. There's nothing in "LLDB_LOG_ERROR" that indicates that the error gets cleared, but that's actually a pretty important piece of its business. Before this propagates further is there a better name we can come up with that's not too horrible to type or scan but conveys this? LLDB_LOG_AND_CLEAR_ERROR is not too bad, though if there's a better single word that conveys logging & handling that would be nicer. But I can't think of one off the top of my head.

In a way it's kind of built into the semantics of llvm::Error that this is the only way it could possibly work, since it's a move only type. If you do this, for example:

Error E = doSomething();
LLDB_LOG_ERROR(E);

you'd get a compilation failure. The only way to make it compile would be to do this:

Error E = doSomething();
LLDB_LOG_ERROR(std::move(E));

And since you've written std::move(E) you already can't use it again anyway. And if you had written it as one line:

LLDB_LOG_ERROR(doSomething());

Then you weren't holding onto the error anyway and it shouldn't matter whether or not it was cleared.

So I'm not sold on the idea of lengthening the name, because you couldn't have used the Error anyway after calling this.

Seems important to me that a name tells what it does without having to know the implementation details of what it acts on. Particularly for folks new to the code, having to know one less thing to understand how something that's important to the logic flow of the program behaves will make comprehending the code easier. It's only a little bump to understanding, but the fewer of those we have the better, IMO.

The behavior of llvm::Error is one more quirk that you need to know about, but I don't think the logging machinery is the place that should teach you that. If you're working with llvm::Error, in all likelyhook you've already had to learn about this behavior anyway, in which case seeing the std::move should be enough of a signal to explain what happens. So, I'd prefer to stick to a shorter name here.

It looks like we haven't come up with a better name, and there's new code in D42468 which would benefit a lot from this functionality, so I'm going to land this. I don't expect hundreds of uses of this macro appearing any time soon, so it should be easy enough to change it if we find something better.

Closed by commit rL323753: Add LLDB_LOG_ERROR macro (authored by labath, committed by ). · Explain WhyJan 30 2018, 4:21 AM
This revision was automatically updated to reflect the committed changes.