Remove some checks for object creation failures. They cannot fail.

Remove some checks for object creation failures. They cannot fail.

Also reduce nesting in if statements, remove CFX_SmartPointer, and
use more unique_ptrs.

R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/6101a5f98b27888f1736ae74982ed4d409d83be0

[Lei Zhang, 2015-10-09 05:22:04]

thestig@chromium.org changed reviewers:
+ tsepez@chromium.org

[Lei Zhang, 2015-10-09 05:22:05]

Well, it started as Create() can't fail, and ended in unique_ptrs gone wild.

[Tom Sepez, 2015-10-09 16:15:22]

On 2015/10/09 05:22:05, Lei Zhang wrote:
> Well, it started as Create() can't fail, and ended in unique_ptrs gone wild.
I know how that goes.

[Tom Sepez, 2015-10-09 16:47:32]

https://codereview.chromium.org/1393303004/diff/60001/core/include/fxcrt/fx_b...
File core/include/fxcrt/fx_basic.h (left):

https://codereview.chromium.org/1393303004/diff/60001/core/include/fxcrt/fx_b...
core/include/fxcrt/fx_basic.h:960: class CFX_SmartPointer {
This gets replaced with a ReleaseDelete, eh?  Its just kinda weird to have a
unique_ptr that doesn't really own the object outright ...

Still, I think its better with this gone.  I'd played with making a
Retain/Release smart pointer some time ago (and applying it to CFX_ string
types), etc.  Is that a better approach?

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (left):

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2696:
std::vector<_SearchTagRecord> patterns(ntags);
Good, _CAPITAL is reserved anyways.  This irks me no end.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:69: return
pObjStream->GetDict()->GetInteger(FX_BSTRC("N"));
I don't think we need the BSTRC() macros, really, but I'm not 100%.  The ctor
for the CFX_ string type will call strlen() on the literal, but I thought that
all our compilers were smart enough to fix that. Something for another day.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:395: if
(!LoadCrossRefV4(CrossRefList[i], XRefStreamList[i], FALSE, i == 0)) {
nit: no braces here.  Yes braces above.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1301: CPDF_StreamAcc*
pObjStream = GetObjectStream((FX_DWORD)m_CrossRef[objnum]);
nit: for another day, every time we seem to use m_CrossRef, there's a cast
involved.  Maybe its type should change?

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1342: CFX_ByteString word =
m_Syntax.GetNextWord(bIsNumber);
nit: someday getnextword should take &bIsNumber rather than non-const ref.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2151: return pArray;
nit: maybe blank line here.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2319: return
(m_WordBuffer[0] == ']') ? pArray.release() : nullptr;
nit: overparenthesized.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2360: if (!GetNextChar(ch))
nit: maybe combine with subsequent if.
nit: then invert the test, and use in condition of while, so that we wind up
with something along the lines of while (...) continue;

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2362: if (ch == 0x0A || ch
== 0x0D)
nit: do we want '\n' and '\r'?  A fascintating philosophical question about
whether the host character set is ascii vs. the document character set.  Hmmm.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2694: for (int i = 0; i <
ntags; ++i) {
nit: can we use an iterator here now that you got a std::vector?

[Lei Zhang, 2015-10-19 08:05:52]

https://codereview.chromium.org/1393303004/diff/60001/core/include/fxcrt/fx_b...
File core/include/fxcrt/fx_basic.h (left):

https://codereview.chromium.org/1393303004/diff/60001/core/include/fxcrt/fx_b...
core/include/fxcrt/fx_basic.h:960: class CFX_SmartPointer {
On 2015/10/09 16:47:31, Tom Sepez wrote:
> This gets replaced with a ReleaseDelete, eh?  Its just kinda weird to have a
> unique_ptr that doesn't really own the object outright ...
> 
> Still, I think its better with this gone.  I'd played with making a
> Retain/Release smart pointer some time ago (and applying it to CFX_ string
> types), etc.  Is that a better approach?
> 

Ya, it's not great, but it beats having to manually call Release() all over the
place. I added a TODO.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (left):

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2696:
std::vector<_SearchTagRecord> patterns(ntags);
On 2015/10/09 16:47:31, Tom Sepez wrote:
> Good, _CAPITAL is reserved anyways.  This irks me no end.

Ack. There's plenty more in other files.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:69: return
pObjStream->GetDict()->GetInteger(FX_BSTRC("N"));
On 2015/10/09 16:47:32, Tom Sepez wrote:
> I don't think we need the BSTRC() macros, really, but I'm not 100%.  The ctor
> for the CFX_ string type will call strlen() on the literal, but I thought that
> all our compilers were smart enough to fix that. Something for another day.

Acknowledged.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:395: if
(!LoadCrossRefV4(CrossRefList[i], XRefStreamList[i], FALSE, i == 0)) {
On 2015/10/09 16:47:31, Tom Sepez wrote:
> nit: no braces here.  Yes braces above.

Done.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1301: CPDF_StreamAcc*
pObjStream = GetObjectStream((FX_DWORD)m_CrossRef[objnum]);
On 2015/10/09 16:47:32, Tom Sepez wrote:
> nit: for another day, every time we seem to use m_CrossRef, there's a cast
> involved.  Maybe its type should change?

Acknowledged.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1342: CFX_ByteString word =
m_Syntax.GetNextWord(bIsNumber);
On 2015/10/09 16:47:32, Tom Sepez wrote:
> nit: someday getnextword should take &bIsNumber rather than non-const ref.

Acknowledged.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2151: return pArray;
On 2015/10/09 16:47:32, Tom Sepez wrote:
> nit: maybe blank line here.

Done.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2319: return
(m_WordBuffer[0] == ']') ? pArray.release() : nullptr;
On 2015/10/09 16:47:32, Tom Sepez wrote:
> nit: overparenthesized.

Done.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2360: if (!GetNextChar(ch))
On 2015/10/09 16:47:32, Tom Sepez wrote:
> nit: maybe combine with subsequent if.
> nit: then invert the test, and use in condition of while, so that we wind up
> with something along the lines of while (...) continue;

Done.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2362: if (ch == 0x0A || ch
== 0x0D)
On 2015/10/09 16:47:32, Tom Sepez wrote:
> nit: do we want '\n' and '\r'?  A fascintating philosophical question about
> whether the host character set is ascii vs. the document character set.  Hmmm.

Meh. I'm just going to leave it as is.

https://codereview.chromium.org/1393303004/diff/60001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2694: for (int i = 0; i <
ntags; ++i) {
On 2015/10/09 16:47:32, Tom Sepez wrote:
> nit: can we use an iterator here now that you got a std::vector?

Annoyingly we still have a "return i" below in the loop, so it's not going to be
that much of a win.

[Tom Sepez, 2015-10-19 16:10:42]

lgtm

[Lei Zhang, 2015-10-19 18:52:31]

Committed patchset #5 (id:80001) manually as
6101a5f98b27888f1736ae74982ed4d409d83be0 (presubmit successful).