[Lldb-commits] [PATCH] D33167: Get rid of some uses of StringConvert and reduce some indentation

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think this is a step in the right direction. Besides reducing boilerplate, this will also help us ensure correctness, as we get a constant trickle of bug reports for commands which forgot to set the result status.

The name is not ideal, but it's probably the best we can get. (The ideal solution for me would be to get rid of the duality in the DoExecute function -- e.g. remove the bool return and let the execution state be fully signalled by the result object -- but that's way off from what you were originally trying to do).

That said, I'm not sure I should be the person approving this. :)



================
Comment at: lldb/source/Commands/CommandObjectPlatform.cpp:611
+    if (!platform_sp->CloseFile(fd, error))
+      return result.AppendError(eReturnStatusFailed, error.AsCString());
+
----------------
I believe this should be `"{0}",  error` to avoid hitting problems in case the error contains '{'. (Plus it's 5 chars shorter :) )


================
Comment at: lldb/source/Commands/CommandObjectProcess.cpp:1181
+                                "Failed to send signal {0}: {1}\n", signo,
+                                ST.AsCString());
+
----------------
AsCString unnecessary


https://reviews.llvm.org/D33167