[Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

This revision was automatically updated to reflect the committed changes. Closed by commit rL291759: Add format_provider for the Error class (authored by labath). Changed prior to commit: https://reviews.llvm.org/D28519?vs=83813&id=84098#toc Repository: rL LLVM https://reviews.llvm.org/D285

[Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks fine. https://reviews.llvm.org/D28519 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/l

Re: [Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

Official Lgtm On Wed, Jan 11, 2017 at 4:48 AM Pavel Labath via Phabricator < revi...@reviews.llvm.org> wrote: > labath added a comment. > > In https://reviews.llvm.org/D28519#641938, @clayborg wrote: > > > You can't add anything extra to the AsCString() since it returns a > "const char *". You can

[Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

labath added a comment. In https://reviews.llvm.org/D28519#641938, @clayborg wrote: > 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

[Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

zturner added a comment. 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 wherev

[Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

clayborg added a comment. 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 st

[Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

clayborg added a comment. 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. https://reviews.llvm.org/D28519 ___ lldb-com

[Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

labath added a comment. 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. https://reviews.llvm.org/D28519 ___ lldb-co

[Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

labath added a comment. 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 i

[Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

zturner added inline comments. Comment at: include/lldb/Core/Error.h:308-309 + llvm::StringRef Options) { +llvm::format_provider::format(error.AsCString(), OS, + Options); + } clayborg wro

[Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

clayborg added a comment. See inlined comment. Comment at: include/lldb/Core/Error.h:308-309 + llvm::StringRef Options) { +llvm::format_provider::format(error.AsCString(), OS, + Options); + } ---

[Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

labath created this revision. labath added reviewers: zturner, clayborg. labath added a subscriber: lldb-commits. Herald added a subscriber: mgorny. 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