More unique_ptrs in CXFA_FMParse::ParseForeachExpression and avoid leaks

More unique_ptrs in CXFA_FMParse::ParseForeachExpression and avoid leak

Commit a31098417852bdf13e693a6e0913e0706cf94098 accidentally caused a
leak. Replaced pointers with unique_ptr to prevent this from happening.

BUG=664891
Committed: https://pdfium.googlesource.com/pdfium/+/4e847e36fe013bcfddf71c79221887a308b9fadb

[npm, 2016-11-23 19:18:32]

Description was changed from

==========
More unique_ptrs in CXFA_FMParse::ParseForeachExpression and avoid leaks

BUG=664891
==========

to

==========
More unique_ptrs in CXFA_FMParse::ParseForeachExpression and avoid leak

Commit a31098417852bdf13e693a6e0913e0706cf94098 accidentally caused a
leak. Replaced pointers with unique_ptr to prevent this from happening.

BUG=664891
==========

[npm, 2016-11-23 19:18:32]

npm@chromium.org changed reviewers:
+ dsinclair@chromium.org, thestig@chromium.org, tsepez@chromium.org

[npm, 2016-11-23 19:18:39]

ptal

[npm, 2016-11-23 19:27:04]

Updating after discussing with Dan. Will ping when ready.

[npm, 2016-11-23 19:35:21]

Ok, ptal

[dsinclair, 2016-11-23 19:38:46]

lg to me, Tom, this look OK to you?

https://codereview.chromium.org/2528543003/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
File xfa/fxfa/fm2js/xfa_fmparse.cpp (right):

https://codereview.chromium.org/2528543003/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:1017:
pAccessors.push_back(std::unique_ptr<CXFA_FMSimpleExpression>(s));
pAccessors.push_back(pdfium::WrapUnique<CXFA_FMSimpleExpression>(s));

Don't need {}'s

https://codereview.chromium.org/2528543003/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:1021: } else {
If you flip the condtions this can become:

if (m_pToken->m_type != TOKcomma)
  break;
NextToken();

[Tom Sepez, 2016-11-23 20:21:55]

https://codereview.chromium.org/2528543003/diff/20001/xfa/fxfa/fm2js/xfa_expr...
File xfa/fxfa/fm2js/xfa_expression.cpp (right):

https://codereview.chromium.org/2528543003/diff/20001/xfa/fxfa/fm2js/xfa_expr...
xfa/fxfa/fm2js/xfa_expression.cpp:517: m_pAccessors(std::move(pAccessors)),
do we need std::move here?

https://codereview.chromium.org/2528543003/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
File xfa/fxfa/fm2js/xfa_fmparse.cpp (right):

https://codereview.chromium.org/2528543003/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:1015: CXFA_FMSimpleExpression* s =
ParseSimpleExpression();
Things get cleaner if ParseSimpleExpression returned unique_ptrs.

[npm, 2016-11-23 20:40:27]

https://codereview.chromium.org/2528543003/diff/20001/xfa/fxfa/fm2js/xfa_expr...
File xfa/fxfa/fm2js/xfa_expression.cpp (right):

https://codereview.chromium.org/2528543003/diff/20001/xfa/fxfa/fm2js/xfa_expr...
xfa/fxfa/fm2js/xfa_expression.cpp:517: m_pAccessors(std::move(pAccessors)),
On 2016/11/23 20:21:55, Tom Sepez wrote:
> do we need std::move here?

Yes, m_pAccessors(pAccessors) does not compile.

https://codereview.chromium.org/2528543003/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
File xfa/fxfa/fm2js/xfa_fmparse.cpp (right):

https://codereview.chromium.org/2528543003/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:1015: CXFA_FMSimpleExpression* s =
ParseSimpleExpression();
On 2016/11/23 20:21:55, Tom Sepez wrote:
> Things get cleaner if ParseSimpleExpression returned unique_ptrs.

There is a long chain down. Is it ok if I do it in a followup?

https://codereview.chromium.org/2528543003/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:1017:
pAccessors.push_back(std::unique_ptr<CXFA_FMSimpleExpression>(s));
On 2016/11/23 19:38:46, dsinclair wrote:
> pAccessors.push_back(pdfium::WrapUnique<CXFA_FMSimpleExpression>(s));
> 
> Don't need {}'s

Done.

https://codereview.chromium.org/2528543003/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:1021: } else {
On 2016/11/23 19:38:46, dsinclair wrote:
> If you flip the condtions this can become:
> 
> if (m_pToken->m_type != TOKcomma)
>   break;
> NextToken();

Done.

[Tom Sepez, 2016-11-23 22:24:08]

lgtm

[npm, 2016-11-23 22:34:34]

The CQ bit was checked by npm@chromium.org

[commit-bot: I haz the power, 2016-11-23 22:34:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 22:47:35]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1479940474251240,
"parent_rev": "9fd0c630ea2225fc544949d88d1bf63acc43112e", "commit_rev":
"4e847e36fe013bcfddf71c79221887a308b9fadb"}

[commit-bot: I haz the power, 2016-11-23 22:47:38]

Description was changed from

==========
More unique_ptrs in CXFA_FMParse::ParseForeachExpression and avoid leak

Commit a31098417852bdf13e693a6e0913e0706cf94098 accidentally caused a
leak. Replaced pointers with unique_ptr to prevent this from happening.

BUG=664891
==========

to

==========
More unique_ptrs in CXFA_FMParse::ParseForeachExpression and avoid leak

Commit a31098417852bdf13e693a6e0913e0706cf94098 accidentally caused a
leak. Replaced pointers with unique_ptr to prevent this from happening.

BUG=664891

Committed:
https://pdfium.googlesource.com/pdfium/+/4e847e36fe013bcfddf71c79221887a308b9...
==========

[commit-bot: I haz the power, 2016-11-23 22:47:38]

Committed patchset #3 (id:40001) as
https://pdfium.googlesource.com/pdfium/+/4e847e36fe013bcfddf71c79221887a308b9...