IndexedDB: ParseMethodAndError takes a leveldb::Status instance.

IndexedDB: ParseMethodAndError takes a leveldb::Status instance.

ParseMethodAndError took a string as first param, but all callers started with a
Status object from which they derived the string. Simplified impl by just
passing in the Status object.

Committed: https://crrev.com/5d9f56995bdb98020897b0ade57e27c424b56fd9
Cr-Commit-Position: refs/heads/master@{#308465}

[cmumford, 2014-12-15 21:44:25]

cmumford@chromium.org changed reviewers:
+ dgrogan@chromium.org

[cmumford, 2014-12-15 21:44:26]

Just a change for code cleanup - no bug fixing.

[dgrogan, 2014-12-15 21:55:30]

lgtm

just nits

https://codereview.chromium.org/805043006/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/805043006/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium.cc:414: std::string str =
status.ToString();
Change the variable name so status_string, or something more descriptive than
"str".

Also, since you call c_str() on this everywhere that it's used, maybe you want
to change this to a char*?

[cmumford, 2014-12-15 22:26:11]

https://codereview.chromium.org/805043006/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/805043006/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium.cc:414: std::string str =
status.ToString();
On 2014/12/15 21:55:30, dgrogan wrote:
> Change the variable name so status_string, or something more descriptive than
> "str".
> 
> Also, since you call c_str() on this everywhere that it's used, maybe you want
> to change this to a char*?

I renamed it to "status_string". I couldn't make it a char* because it would
point to a temporary stack result (ToString's return value). Let me know if I
misunderstood your request.

[dgrogan, 2014-12-15 22:31:48]

https://codereview.chromium.org/805043006/diff/1/third_party/leveldatabase/en...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/805043006/diff/1/third_party/leveldatabase/en...
third_party/leveldatabase/env_chromium.cc:414: std::string str =
status.ToString();
On 2014/12/15 22:26:11, cmumford wrote:
> On 2014/12/15 21:55:30, dgrogan wrote:
> > Change the variable name so status_string, or something more descriptive
than
> > "str".
> > 
> > Also, since you call c_str() on this everywhere that it's used, maybe you
want
> > to change this to a char*?
> 
> I renamed it to "status_string". I couldn't make it a char* because it would
> point to a temporary stack result (ToString's return value). Let me know if I
> misunderstood your request.

I think you understood it but now I'm confused. Won't ToString's return value
exist for the duration of this function call? As in, I could understand not
wanting to return the char* from this function, but just using it internally
should be fine.

In any case, this is trivial, so feel free to submit.

[cmumford, 2014-12-15 23:05:38]

On 2014/12/15 22:31:48, dgrogan wrote:
>
https://codereview.chromium.org/805043006/diff/1/third_party/leveldatabase/en...
> File third_party/leveldatabase/env_chromium.cc (right):
> 
>
https://codereview.chromium.org/805043006/diff/1/third_party/leveldatabase/en...
> third_party/leveldatabase/env_chromium.cc:414: std::string str =
> status.ToString();
> On 2014/12/15 22:26:11, cmumford wrote:
> > On 2014/12/15 21:55:30, dgrogan wrote:
> > > Change the variable name so status_string, or something more descriptive
> than
> > > "str".
> > > 
> > > Also, since you call c_str() on this everywhere that it's used, maybe you
> want
> > > to change this to a char*?
> > 
> > I renamed it to "status_string". I couldn't make it a char* because it would
> > point to a temporary stack result (ToString's return value). Let me know if
I
> > misunderstood your request.
> 
> I think you understood it but now I'm confused. Won't ToString's return value
> exist for the duration of this function call? As in, I could understand not
> wanting to return the char* from this function, but just using it internally
> should be fine.
> 
> In any case, this is trivial, so feel free to submit.

I am pressing the commit button.

just to complete the discussion thread this code:

const char* const status_string = status.ToString().c_str();

Will return back a pointer to a string in heap owned by a temporary variable
which was deleted just after operator= returns. So status_string is immediately
invalid. This was valid before because we were calling:

ParseMethodAndError(s.ToString().c_str(), ...)

And the compiler would ensure that the temp string returned by ToString() would
live on the stack until ParseMethodAndError returned.

Pretty good discussion at
http://stackoverflow.com/questions/17902045/what-is-a-lifetime-of-a-function-...

[cmumford, 2014-12-15 23:05:44]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2014-12-15 23:07:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/805043006/20001

[commit-bot: I haz the power, 2014-12-16 00:05:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-16 00:05:57]

Patchset 2 (id:??) landed as
https://crrev.com/5d9f56995bdb98020897b0ade57e27c424b56fd9
Cr-Commit-Position: refs/heads/master@{#308465}

[dgrogan, 2014-12-18 00:28:36]

On 2014/12/15 23:05:38, cmumford wrote:
> On 2014/12/15 22:31:48, dgrogan wrote:
> >
>
https://codereview.chromium.org/805043006/diff/1/third_party/leveldatabase/en...
> > File third_party/leveldatabase/env_chromium.cc (right):
> > 
> >
>
https://codereview.chromium.org/805043006/diff/1/third_party/leveldatabase/en...
> > third_party/leveldatabase/env_chromium.cc:414: std::string str =
> > status.ToString();
> > On 2014/12/15 22:26:11, cmumford wrote:
> > > On 2014/12/15 21:55:30, dgrogan wrote:
> > > > Change the variable name so status_string, or something more descriptive
> > than
> > > > "str".
> > > > 
> > > > Also, since you call c_str() on this everywhere that it's used, maybe
you
> > want
> > > > to change this to a char*?
> > > 
> > > I renamed it to "status_string". I couldn't make it a char* because it
would
> > > point to a temporary stack result (ToString's return value). Let me know
if
> I
> > > misunderstood your request.
> > 
> > I think you understood it but now I'm confused. Won't ToString's return
value
> > exist for the duration of this function call? As in, I could understand not
> > wanting to return the char* from this function, but just using it internally
> > should be fine.
> > 
> > In any case, this is trivial, so feel free to submit.
> 
> I am pressing the commit button.
> 
> just to complete the discussion thread this code:
> 
> const char* const status_string = status.ToString().c_str();
> 
> Will return back a pointer to a string in heap owned by a temporary variable
> which was deleted just after operator= returns.

Yes, you are quite right. How have I written so much C++ without knowing when
the temporary is deleted...