Use unique pointers in CXFA_FMParse

Use unique pointers in CXFA_FMParse

Replaced CXFA_FMParse's returns to be unique pointers to prevent leaks.

Committed: https://pdfium.googlesource.com/pdfium/+/eb9625c04876c45862fb30e48a4fd6d5998db0e9

[npm, 2016-11-24 20:53:44]

The CQ bit was checked by npm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 20:53:46]

Dry run: 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-24 21:00:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 21:00:16]

Dry run: Try jobs failed on following builders:
  win_xfa_rel on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_xfa_rel/bui...)

[npm, 2016-11-24 21:06:31]

The CQ bit was checked by npm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 21:06:42]

Dry run: 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-24 21:17:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 21:17:32]

Dry run: This issue passed the CQ dry run.

[npm, 2016-11-24 21:19:52]

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

[npm, 2016-11-24 21:19:52]

PTAL

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

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:596: e.reset();
e.reset() is the same as e = nullptr?

[Tom Sepez, 2016-11-28 18:09:12]

Just some tidying as you see fit.

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

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_expr...
xfa/fxfa/fm2js/xfa_expression.cpp:84: CXFA_FMExpression* e =
m_pExpressions.at(i).get();
nit: or just const auto& e = m_pExpressions[i]; since operator-> works on this
as if it were a real pointer.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_expr...
xfa/fxfa/fm2js/xfa_expression.cpp:212: for (size_t i = 0; i <
m_ExpressionList.size(); ++i)
nit: or just 

  for (const auto& expr : m_ExpressionList)
    expr->ToJavaScript(javascript);

you could do the same thing in the other loops, but your "last item" check would
become something like 

   if (expr == m_ExpressionList.back())

which may read a little clearer that the 1+1 == size().

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

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_expr...
xfa/fxfa/fm2js/xfa_expression.h:49:
std::vector<std::unique_ptr<CXFA_FMExpression>>&& pExpressions);
Maybe std::unique_ptr<std::vector<std::unique_ptr<CFXA_FMExpression>>> ?
That pattern is a little more common in our code, still there are places where
we snarf the vector contents in the way you're doing here.  Your call.

Though I can see why - avoid checking a lot of null ptrs - hmm, OK.

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

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:19: int32_t CXFA_FMParse::Init(const
CFX_WideStringC& wsFormcalc,
nit: follow-up: since this always returns 0, it can be void.  Do we ever call it
apart form immediately after making one of these?  If not, then move to ctor.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:55: std::unique_ptr<CXFA_FMExpression> e;
nit: can we call this expr or expression?

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:56:
std::vector<std::unique_ptr<CXFA_FMExpression>> expression;
nit: can we call this |expressions| since it's an aggregate?

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:87:
std::vector<std::unique_ptr<CXFA_FMExpression>> expressions;
ditto nits.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:250: std::unique_ptr<CXFA_FMSimpleExpression>
e1(ParseLogicalAndExpression());
nit: ParseLogicalAndExpression() returns unique_ptr in follow-up?  Otherwise e1
= ParseLogicalAndEpression() may be clearer.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:350: line, TOKlt, e1.release(), e2.release());
nit: Ideally, these ctors take unique_ptrs so we can std:move(e1) in a
follow-up.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:596: e.reset();
On 2016/11/24 21:19:52, npm wrote:
> e.reset() is the same as e = nullptr?

right.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:611:
std::vector<std::unique_ptr<CXFA_FMSimpleExpression>> pArray;
nit: probably should just be |array|, no longer a pointer so no p Prefix. Or
just |expressions| since |array| may collide.  That's the downside of the
so-called hungarian notation -- change type, change a lot of names.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:811: std::unique_ptr<CXFA_FMExpression> e =
nullptr;
nit:  assignment not required, default ctor gives you a null one.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:1030: std::unique_ptr<CXFA_FMExpression> e;
nit: maybe move and combine at line 1033.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_prog...
File xfa/fxfa/fm2js/xfa_program.cpp (right):

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_prog...
xfa/fxfa/fm2js/xfa_program.cpp:24: if (!m_pErrorInfo.message.IsEmpty()) {
nit: no {}

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_prog...
xfa/fxfa/fm2js/xfa_program.cpp:33: 1, true, FX_WSTRC(L""), nullptr,
std::move(expressions));
nit: I think you can just pass L"" here, the ctor should be implicit to avoid
writing this all over the place.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_simp...
File xfa/fxfa/fm2js/xfa_simpleexpression.cpp (right):

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_simp...
xfa/fxfa/fm2js/xfa_simpleexpression.cpp:591: CXFA_FMSimpleExpression* e =
m_Arguments.at(0).get();
nit: same comments as before, also above loops.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_simp...
File xfa/fxfa/fm2js/xfa_simpleexpression_unittest.cpp (right):

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_simp...
xfa/fxfa/fm2js/xfa_simpleexpression_unittest.cpp:16:
std::unique_ptr<CXFA_FMIdentifierExpression> exp =
nit: I'd write auto exp = ...
since we already have the full type information in the MakeUnique<>.
I'm less of a fan of auto when that information isn't immediately obvious ...

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_simp...
xfa/fxfa/fm2js/xfa_simpleexpression_unittest.cpp:18: CFX_WideStringC(L"sign"));
nit: Can we just pass L"sign" (implicit ctor)?

[Tom Sepez, 2016-11-28 18:09:20]

lgtm

[npm, 2016-11-28 20:04:40]

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

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_expr...
xfa/fxfa/fm2js/xfa_expression.cpp:84: CXFA_FMExpression* e =
m_pExpressions.at(i).get();
On 2016/11/28 18:09:11, Tom Sepez wrote:
> nit: or just const auto& e = m_pExpressions[i]; since operator-> works on this
> as if it were a real pointer.

Done.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_expr...
xfa/fxfa/fm2js/xfa_expression.cpp:212: for (size_t i = 0; i <
m_ExpressionList.size(); ++i)
On 2016/11/28 18:09:11, Tom Sepez wrote:
> nit: or just 
> 
>   for (const auto& expr : m_ExpressionList)
>     expr->ToJavaScript(javascript);
> 
> you could do the same thing in the other loops, but your "last item" check
would
> become something like 
> 
>    if (expr == m_ExpressionList.back())
> 
> which may read a little clearer that the 1+1 == size().
> 

Done.

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

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_expr...
xfa/fxfa/fm2js/xfa_expression.h:49:
std::vector<std::unique_ptr<CXFA_FMExpression>>&& pExpressions);
On 2016/11/28 18:09:11, Tom Sepez wrote:
> Maybe std::unique_ptr<std::vector<std::unique_ptr<CFXA_FMExpression>>> ?
> That pattern is a little more common in our code, still there are places where
> we snarf the vector contents in the way you're doing here.  Your call.
> 
> Though I can see why - avoid checking a lot of null ptrs - hmm, OK.

This way was suggested by Dan for a previous CL:
https://codereview.chromium.org/2528543003/#ps20001. Not adding the extra
std::unique_ptr layer looks a bit cleaner to me. Maybe we could change some of
the std::unique_ptr<std::vector usages to not use unique pointer when there is
no need to do so?

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

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:19: int32_t CXFA_FMParse::Init(const
CFX_WideStringC& wsFormcalc,
On 2016/11/28 18:09:11, Tom Sepez wrote:
> nit: follow-up: since this always returns 0, it can be void.  Do we ever call
it
> apart form immediately after making one of these?  If not, then move to ctor.

Acknowledged.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:55: std::unique_ptr<CXFA_FMExpression> e;
On 2016/11/28 18:09:12, Tom Sepez wrote:
> nit: can we call this expr or expression?

Done.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:56:
std::vector<std::unique_ptr<CXFA_FMExpression>> expression;
On 2016/11/28 18:09:12, Tom Sepez wrote:
> nit: can we call this |expressions| since it's an aggregate?

Done.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:87:
std::vector<std::unique_ptr<CXFA_FMExpression>> expressions;
On 2016/11/28 18:09:11, Tom Sepez wrote:
> ditto nits.

Done.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:250: std::unique_ptr<CXFA_FMSimpleExpression>
e1(ParseLogicalAndExpression());
On 2016/11/28 18:09:12, Tom Sepez wrote:
> nit: ParseLogicalAndExpression() returns unique_ptr in follow-up?  Otherwise
e1
> = ParseLogicalAndEpression() may be clearer. 

Already returns unique_ptr. Changing to e1
= ParseLogicalAndEpression() here and similarly in other places.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:350: line, TOKlt, e1.release(), e2.release());
On 2016/11/28 18:09:12, Tom Sepez wrote:
> nit: Ideally, these ctors take unique_ptrs so we can std:move(e1) in a
> follow-up.

Acknowledged.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:611:
std::vector<std::unique_ptr<CXFA_FMSimpleExpression>> pArray;
On 2016/11/28 18:09:12, Tom Sepez wrote:
> nit: probably should just be |array|, no longer a pointer so no p Prefix. Or
> just |expressions| since |array| may collide.  That's the downside of the
> so-called hungarian notation -- change type, change a lot of names.

Done.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:811: std::unique_ptr<CXFA_FMExpression> e =
nullptr;
On 2016/11/28 18:09:11, Tom Sepez wrote:
> nit:  assignment not required, default ctor gives you a null one.

Done.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_fmpa...
xfa/fxfa/fm2js/xfa_fmparse.cpp:1030: std::unique_ptr<CXFA_FMExpression> e;
On 2016/11/28 18:09:12, Tom Sepez wrote:
> nit: maybe move and combine at line 1033.

Done.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_prog...
File xfa/fxfa/fm2js/xfa_program.cpp (right):

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_prog...
xfa/fxfa/fm2js/xfa_program.cpp:24: if (!m_pErrorInfo.message.IsEmpty()) {
On 2016/11/28 18:09:12, Tom Sepez wrote:
> nit: no {}

Done.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_prog...
xfa/fxfa/fm2js/xfa_program.cpp:33: 1, true, FX_WSTRC(L""), nullptr,
std::move(expressions));
On 2016/11/28 18:09:12, Tom Sepez wrote:
> nit: I think you can just pass L"" here, the ctor should be implicit to avoid
> writing this all over the place.

Done.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_simp...
File xfa/fxfa/fm2js/xfa_simpleexpression.cpp (right):

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_simp...
xfa/fxfa/fm2js/xfa_simpleexpression.cpp:541: if (i < 32 && (methodPara & (0x01
<< i)) > 0) {
Leaving i in this for loop for now because it is being used non-trivially.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_simp...
File xfa/fxfa/fm2js/xfa_simpleexpression_unittest.cpp (right):

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_simp...
xfa/fxfa/fm2js/xfa_simpleexpression_unittest.cpp:16:
std::unique_ptr<CXFA_FMIdentifierExpression> exp =
On 2016/11/28 18:09:12, Tom Sepez wrote:
> nit: I'd write auto exp = ...
> since we already have the full type information in the MakeUnique<>.
> I'm less of a fan of auto when that information isn't immediately obvious ...

Done.

https://codereview.chromium.org/2530933002/diff/20001/xfa/fxfa/fm2js/xfa_simp...
xfa/fxfa/fm2js/xfa_simpleexpression_unittest.cpp:18: CFX_WideStringC(L"sign"));
On 2016/11/28 18:09:12, Tom Sepez wrote:
> nit: Can we just pass L"sign" (implicit ctor)?

Done.

[Tom Sepez, 2016-11-28 20:15:07]

++LGTM

[npm, 2016-11-28 20:16:24]

The CQ bit was checked by npm@chromium.org

[commit-bot: I haz the power, 2016-11-28 20:16:30]

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-28 20:31:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480364184687610,
"parent_rev": "c92e26defaa636b0d2ec9b9de0f88c6153ad7ed3", "commit_rev":
"eb9625c04876c45862fb30e48a4fd6d5998db0e9"}

[commit-bot: I haz the power, 2016-11-28 20:31:16]

Description was changed from

==========
Use unique pointers in CXFA_FMParse

Replaced CXFA_FMParse's returns to be unique pointers to prevent leaks.
==========

to

==========
Use unique pointers in CXFA_FMParse

Replaced CXFA_FMParse's returns to be unique pointers to prevent leaks.

Committed:
https://pdfium.googlesource.com/pdfium/+/eb9625c04876c45862fb30e48a4fd6d5998d...
==========

[commit-bot: I haz the power, 2016-11-28 20:31:17]

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