Clean up misc nits found while fixing bugs.

Clean up misc nits found while fixing bugs.

Some bits regressed in commit 338805f.

R=tsepez@chromium.org

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

[Lei Zhang, 2015-12-29 03:34:34]

Patchset #1 (id:1) has been deleted

[Lei Zhang, 2015-12-29 03:37:02]

Description was changed from

==========
Cleanup misc nits found while fixing a bug.
==========

to

==========
Cleanup misc nits found while fixing bugs.
==========

[Lei Zhang, 2016-01-07 07:05:15]

Description was changed from

==========
Cleanup misc nits found while fixing bugs.
==========

to

==========
Clean up misc nits found while fixing bugs.
==========

[Lei Zhang, 2016-01-07 07:05:16]

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

[Lei Zhang, 2016-01-07 07:06:48]

Patchset #1 (id:20001) has been deleted

[Tom Sepez, 2016-01-07 17:27:02]

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
File core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp (right):

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:294: std::vector<uint8_t>*
buffer) {
nit: does RVO allow us to return this as a result without penalty rather than
passing in an address?

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:298: for (int i = 0; i < 2; ++i)
nit: worth unrolling this loop into two calls?

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:313: if (bFirstObject) {
nit: maybe 
  uint8_t byte = bFirstObject ? 0xFF : 0;
  buffer.AppendByte(byte);
  buffer.AppendByte(byte);

(doublecheck what type |byte| should have).

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:353: int32_t
OutPutIndex(CFX_FileBufferArchive* pFile, FX_FILESIZE offset) {
nit: OutputIndex

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:354: FXSYS_assert(pFile);
nit: not needed.

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:357: if
(pFile->AppendByte(FX_GETBYTEOFFSET56(offset)) < 0) {
nit: {} not required.

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:13: #define
REQUIRE_PARAMS(count)  \
nit: bleh on hiding an early return.

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:434: struct _FX_BSTR {
Down the road. I've thought that the real CFX_ByteStringC with all its methods
should inherit from a POD type like this, to allow this sort of thing. Thoughts?

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:478: if (abbr.GetLength() ==
table[i + 1].m_Size &&
Is it too much to ask to make a struct containing a pair of values rather than
the [n] [n+1] trick used here?

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp (left):

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:1012:
m_pSingleStream->LoadAllData(pForm->m_pFormStream, FALSE);
Are we sure one of these didn't mean to pass TRUE?

[Lei Zhang, 2016-01-08 02:54:31]

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
File core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp (right):

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:294: std::vector<uint8_t>*
buffer) {
On 2016/01/07 17:27:02, Tom Sepez wrote:
> nit: does RVO allow us to return this as a result without penalty rather than
> passing in an address?

Done.

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:298: for (int i = 0; i < 2; ++i)
On 2016/01/07 17:27:02, Tom Sepez wrote:
> nit: worth unrolling this loop into two calls?

Let the compiler figure it out?

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:313: if (bFirstObject) {
On 2016/01/07 17:27:02, Tom Sepez wrote:
> nit: maybe 
>   uint8_t byte = bFirstObject ? 0xFF : 0;
>   buffer.AppendByte(byte);
>   buffer.AppendByte(byte);
> 
> (doublecheck what type |byte| should have).

Done.

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:353: int32_t
OutPutIndex(CFX_FileBufferArchive* pFile, FX_FILESIZE offset) {
On 2016/01/07 17:27:02, Tom Sepez wrote:
> nit: OutputIndex

Done.

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:354: FXSYS_assert(pFile);
On 2016/01/07 17:27:02, Tom Sepez wrote:
> nit: not needed.

Done.

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:357: if
(pFile->AppendByte(FX_GETBYTEOFFSET56(offset)) < 0) {
On 2016/01/07 17:27:02, Tom Sepez wrote:
> nit: {} not required.

Done.

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp (right):

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:13: #define
REQUIRE_PARAMS(count)  \
On 2016/01/07 17:27:02, Tom Sepez wrote:
> nit: bleh on hiding an early return.

Gone.

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:434: struct _FX_BSTR {
On 2016/01/07 17:27:02, Tom Sepez wrote:
> Down the road. I've thought that the real CFX_ByteStringC with all its methods
> should inherit from a POD type like this, to allow this sort of thing.
Thoughts?

Sounds ok to me. Or maybe Blink's String class will take over in Chromium some
day and we'll switch to that too.

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser.cpp:478: if (abbr.GetLength() ==
table[i + 1].m_Size &&
On 2016/01/07 17:27:02, Tom Sepez wrote:
> Is it too much to ask to make a struct containing a pair of values rather than
> the [n] [n+1] trick used here? 

Done.

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_p...
File core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp (left):

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_p...
core/src/fpdfapi/fpdf_page/fpdf_page_parser_old.cpp:1012:
m_pSingleStream->LoadAllData(pForm->m_pFormStream, FALSE);
On 2016/01/07 17:27:02, Tom Sepez wrote:
> Are we sure one of these didn't mean to pass TRUE?

Dunno what Foxit had in mind when they changed this. It used to be just a single
statement.

[Tom Sepez, 2016-01-08 17:01:45]

lgtm

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
File core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp (right):

https://codereview.chromium.org/1544963002/diff/40001/core/src/fpdfapi/fpdf_e...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:298: for (int i = 0; i < 2; ++i)
On 2016/01/08 02:54:30, Lei Zhang wrote:
> On 2016/01/07 17:27:02, Tom Sepez wrote:
> > nit: worth unrolling this loop into two calls?
> 
> Let the compiler figure it out?
Nah, nothing to do with efficiency, I thought it read cleaner.  NO matter.

https://codereview.chromium.org/1544963002/diff/100001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp (right):

https://codereview.chromium.org/1544963002/diff/100001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:967: }
good catch.  Did we break this or has it always been wrong?

[Lei Zhang, 2016-01-08 20:15:39]

https://codereview.chromium.org/1544963002/diff/100001/core/src/fpdfapi/fpdf_...
File core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp (right):

https://codereview.chromium.org/1544963002/diff/100001/core/src/fpdfapi/fpdf_...
core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:967: }
On 2016/01/08 17:01:45, Tom Sepez wrote:
> good catch.  Did we break this or has it always been wrong?

No idea. I just run:

git cl lint 2>&1 | grep -v C-style|grep -v non-const

and fix the easier ones along the way.

[Tom Sepez, 2016-01-08 20:31:33]

On 2016/01/08 20:15:39, Lei Zhang wrote:
>
https://codereview.chromium.org/1544963002/diff/100001/core/src/fpdfapi/fpdf_...
> File core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp (right):
> 
>
https://codereview.chromium.org/1544963002/diff/100001/core/src/fpdfapi/fpdf_...
> core/src/fpdfapi/fpdf_edit/fpdf_edit_create.cpp:967: }
> On 2016/01/08 17:01:45, Tom Sepez wrote:
> > good catch.  Did we break this or has it always been wrong?
> 
> No idea. I just run:
> 
> git cl lint 2>&1 | grep -v C-style|grep -v non-const
> 
> and fix the easier ones along the way.

broken at 338805f.

[Tom Sepez, 2016-01-08 20:32:41]

> broken at 338805f.
Can we add this to the description?

[Lei Zhang, 2016-01-08 21:51:59]

On 2016/01/08 20:32:41, Tom Sepez wrote:
> > broken at 338805f.
> Can we add this to the description?

Sure.

[Lei Zhang, 2016-01-08 21:52:50]

Description was changed from

==========
Clean up misc nits found while fixing bugs.
==========

to

==========
Clean up misc nits found while fixing bugs.

Some bits regressed in commit 338805f.
==========

[Lei Zhang, 2016-01-08 21:53:13]

Description was changed from

==========
Clean up misc nits found while fixing bugs.

Some bits regressed in commit 338805f.
==========

to

==========
Clean up misc nits found while fixing bugs.

Some bits regressed in commit 338805f.

R=tsepez@chromium.org

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

[Lei Zhang, 2016-01-08 21:53:14]

Committed patchset #5 (id:120001) manually as
c5a8933f260ca1034fde902058442984e55c0f7a (presubmit successful).