Page MenuHomePhabricator

Get rid of some uses of StringConvert and reduce some indentation
Needs RevisionPublic

Authored by zturner on May 13 2017, 10:58 PM.



The goal here is to eventually delete the StringConvert class, as the llvm APIs for doing string to number conversion have a cleaner syntax.

In order to reduce code churn, since I was in this code anyway, I changed the deeply nested indentation of the code blocks I was touching to use early outs. This led me to get annoyed by the fact that every time we want to return something, it has to be 3 or 4 lines of code to open a scope, set the correct status on the error, set the message, eventually return, and then add another line of code to close the scope. We should be able to do all this in one line to further reduce boilerplate goo. So I added some code to Status to do this. Whereas before we would have to write this:

if (!succeed) {
  result.AppendError("some error");
  return result.Succeeded();

which is 5 lines of code, now we only have to write:

if (!success)
  return result.AppendError(eStatusFailed, "some error");

which is only 2 lines.

There are more occurrences of StringConvert to change, but I wanted to get the feedback out of the way first before I go and do the rest.

Diff Detail

Event Timeline

zturner created this revision.May 13 2017, 10:58 PM

It's not that perfectly clear to me that SetStatus() returns Succeeded(), however I don't have a proposal for a better name. SetStatusReturnSucceeded() or similar are long.

labath edited edge metadata.May 15 2017, 5:58 AM

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


I believe this should be "{0}", error to avoid hitting problems in case the error contains '{'. (Plus it's 5 chars shorter :) )


AsCString unnecessary

jingham requested changes to this revision.May 15 2017, 11:10 AM
jingham added a subscriber: jingham.

This looks fine. Like Kamil, I think it would help to document your new interfaces.

Going away from StringConvert, you are switching from an API that gives a value on fail to one that doesn't change the input value on error. You mostly handle the failures right away, so in that case it's fine to just declare them. But anywhere it's not immediately clear you are directly returning, you should probably initialize the variables to the fail value.


Can you add a comment saying what these functions return? It's clear now that the code is in the header file, but that doesn't describe a contract just a current state, and anyway, will keep it clear if the code gets moved to the implementation file.


"ST" is not how we name variables in lldb. Besides the capitalization, which is wrong, we used to call all Error variables "error" which gave its use a bit of consistency. You didn't change the variable names when you renamed them to "error" which is probably why calling this one "error" gave you pause. But we should maintain consistency for now, and that will make it easier to go rename them to whatever is appropriate when we get around to that.


I don't think it does any harm here. But your switching from an API that sets a fail value to one that doesn't, so you're adding the possibility of uninitialized variable access. Probably good as you are making this transition to initialize to the fail value.

This revision now requires changes to proceed.May 15 2017, 11:10 AM
labath resigned from this revision.Nov 26 2018, 2:55 AM
labath added a subscriber: labath.