Page MenuHomePhabricator

Add format_provider for the Error class
ClosedPublic

Authored by labath on Jan 10 2017, 8:30 AM.

Details

Summary

The formatter supports the same options as the string-like classes, i.e. the
ability to truncate the displayed string. I don't anticipate it would be much
used, but it seems consistent.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 83813.Jan 10 2017, 8:30 AM
labath retitled this revision from to Add format_provider for the Error class.
labath updated this object.
labath added reviewers: zturner, clayborg.
labath added a subscriber: lldb-commits.
clayborg edited edge metadata.Jan 10 2017, 11:36 AM

See inlined comment.

include/lldb/Core/Error.h
308–309 ↗(On Diff #83813)

Do we want to prefix this output with "error: " followed by the error string?

zturner added inline comments.Jan 10 2017, 11:41 AM
include/lldb/Core/Error.h
308–309 ↗(On Diff #83813)

If we prefix it by something, then perhaps we should incldue the error category as well. Is it a Windows error, a generic error, a posix error?

(Note that in the future I still would like to kill lldb::Error and replace it with llvm::Error which is much safer / more extensible, but no objection to this kind of change going in in the interim.)

I wouldn't put just "error" as it is quite superfluous ("No such file." sounds like an error even without that prefix). And then you don't have the option to remove it if you need it for some reason (unless you add more format options...). I'd keep it without it, as it is easy to add it manually if you need it.

Adding the error category sounds like a better idea. I am fine without it, but I can add it if you want to.

If we're going to be changing stuff, I'd consider changing the success case to print "Success". (But I'd also change the AsCString method to keep things consistent).

I think we should keep the AsCString and the formatted output identical, to minimize surprises. So if we want to add category, I think it should go into the AsCString as well.

Lets start with this. I would by default just emit the error string and then let users opt in via additional format modifiers to show the "error: " prefix and add the category.

You can't add anything extra to the AsCString() since it returns a "const char *". You can't return a StringRef because it isn't backed by anything. You could return a std::string.

My vote would be to leave AsCString() alone and have it just return a pointer to the string buffer that it owns, and let the formatv stuff do the extra formatting.

zturner edited edge metadata.Jan 10 2017, 2:43 PM

Technically it's backed by the error instance, so you could return a StringRef as long as you said that its lifetime is tied to the lifetime of the Error instance. Not super unreasonable, but also not high priority. I would vote for getting rid of c strings wherever possible, but in this case I think a std::string would be better. On the other hand, as mentioned it's kind of orthogonal to this change, so for now I wouldn't worry about it at all and just leave AsCString alone as Greg said.

You can't add anything extra to the AsCString() since it returns a "const char *". You can't return a StringRef because it isn't backed by anything. You could return a std::string.

My vote would be to leave AsCString() alone and have it just return a pointer to the string buffer that it owns, and let the formatv stuff do the extra formatting.

The way I would add it is to add whatever text we want directly to the m_string variable that backs the returned const char *. Basically, I believe that if a function has a "natural" string conversion function (which AsCString is) then the default formatv conversion should be just that (obviously the formatv conversion can do other fancy stuff with the format modifiers, but that's another story).

I believe we have converged to keeping this patch as is. If I don't hear any comments, I am going to land this tomorrow.

clayborg accepted this revision.Jan 11 2017, 9:49 AM
clayborg edited edge metadata.

Looks fine.

This revision is now accepted and ready to land.Jan 11 2017, 9:49 AM
This revision was automatically updated to reflect the committed changes.