Resolve compilation error with G++ 4.9.

Resolve compilation error with G++ 4.9.

Add a check for zero-length keys to avoid hitting the equivalent of |""[1]|.
BUG=https://code.google.com/p/pdfium/issues/detail?id=58
R=jun_fang@foxitsoftware.com

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

[Tom Sepez, 2014-10-21 19:57:04]

tsepez@chromium.org changed reviewers:
+ bo_xu@foxitsoftware.com, jun_fang@foxitsoftware.com

[Tom Sepez, 2014-10-21 19:57:04]

Bo, Jun, please review.

[Tom Sepez, 2014-10-29 17:23:34]

On 2014/10/21 19:57:04, Tom Sepez wrote:
> Bo, Jun, please review.

Ping?

[jun_fang, 2014-10-29 18:21:50]

https://codereview.chromium.org/665223003/diff/1/core/src/fpdfapi/fpdf_parser...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/665223003/diff/1/core/src/fpdfapi/fpdf_parser...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2188: if (key.IsEmpty()) {
IsEmpty is checked at Ln2168. If key is not empty before Ln2187, it shall not be
empty after the function of PDF_NameDecode. Do you think it's necessary to add
another check here?

[Tom Sepez, 2014-10-29 18:27:11]

https://codereview.chromium.org/665223003/diff/1/core/src/fpdfapi/fpdf_parser...
> core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2188: if (key.IsEmpty()) {
> IsEmpty is checked at Ln2168. If key is not empty before Ln2187, it shall not
be
> empty after the function of PDF_NameDecode. Do you think it's necessary to add
> another check here?

The problem is that the compiler can't know this because PDF_NameDecode is
external, and gives the error in the linked bug.

[Tom Sepez, 2014-10-29 18:28:03]

> The problem is that the compiler can't know this because PDF_NameDecode is
> external, and gives the error in the linked bug.
i.e. My hope by adding the code was to let it know this so that the error is
avoided.

[Tom Sepez, 2014-10-29 18:49:42]

https://codereview.chromium.org/665223003/diff/1/core/src/fpdfapi/fpdf_parser...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/665223003/diff/1/core/src/fpdfapi/fpdf_parser...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2200: if (key.GetLength() ==
1) {
Maybe instead we just check here for key.GetLength() == 0?

[jun_fang, 2014-10-29 18:58:29]

https://codereview.chromium.org/665223003/diff/1/core/src/fpdfapi/fpdf_parser...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/665223003/diff/1/core/src/fpdfapi/fpdf_parser...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2200: if (key.GetLength() ==
1) {
On 2014/10/29 18:49:41, Tom Sepez wrote:
> Maybe instead we just check here for key.GetLength() == 0?

I am checking the code here. I think that the current logic may not be correct.
It's easy to understand that we add to use addvalue when nKeys >= 32. When nKeys
< 32, we only needs to call SetAt(). For example, assume key.GetLength() = 1 and
nKeys = 33 here. I think Addvalue shall be called rather SetAt. So my suggestion
for change is :

if (key.GetLength() >= 1) {
    if (nKeys < 32) {
       pDict->SetAt(CFX_ByteStringC(((FX_LPCSTR)key) + 1, key.GetLength() - 1),
pObj);
     } else {
       pDict->AddValue(CFX_ByteStringC(((FX_LPCSTR)key) + 1, key.GetLength() -
1), pObj);
}

[Tom Sepez, 2014-10-29 20:26:55]

On 2014/10/29 18:58:29, jun_fang wrote:
>
https://codereview.chromium.org/665223003/diff/1/core/src/fpdfapi/fpdf_parser...
> File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):
> 
>
https://codereview.chromium.org/665223003/diff/1/core/src/fpdfapi/fpdf_parser...
> core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:2200: if (key.GetLength()
==
> 1) {
> On 2014/10/29 18:49:41, Tom Sepez wrote:
> > Maybe instead we just check here for key.GetLength() == 0?
> 
> I am checking the code here. I think that the current logic may not be
correct.
> It's easy to understand that we add to use addvalue when nKeys >= 32. When
nKeys
> < 32, we only needs to call SetAt(). For example, assume key.GetLength() = 1
and
> nKeys = 33 here. I think Addvalue shall be called rather SetAt. So my
suggestion
> for change is :
> 
> if (key.GetLength() >= 1) {
>     if (nKeys < 32) {
>        pDict->SetAt(CFX_ByteStringC(((FX_LPCSTR)key) + 1, key.GetLength() -
1),
> pObj);
>      } else {
>        pDict->AddValue(CFX_ByteStringC(((FX_LPCSTR)key) + 1, key.GetLength() -
> 1), pObj);
> }

Can you explain what the intended difference between SetAt and AddValue might
be?  I'm trying to infer it from the code, but its still a little hazy.  Thanks.

Otherwise, what you propose seems reasonable.  Is the number 32 special for some
reason?  Does it occur in other places?  Should we have a #define for it?

[Tom Sepez, 2014-10-29 22:04:00]

Sooo .... looking at this, one gets the feeling that at one time, we may have
simply done SetAt(), which checks for duplicates and empty slots for re-use,
except that the underlying representation is O(N) for insert due to duplicate
detection, and someone noticed that above N = 32 it gets slow. Then rather than
fixing the underlying representation, they bounded the badness and instead went
to straight inserts, skipping the check for duplicates. But doesn't doing so
violate the PDF spec in that dictionaries aren't supposed to have more than one
element with the same key?  

We should land this patch, though.  I think you are right in capturing the
intent.  LGTM.

As a follow-up, I'm going to see what it would take to make your Dictionaries
based upon std::map.

[jun_fang, 2014-10-29 22:16:26]

On 2014/10/29 22:04:00, Tom Sepez wrote:
> Sooo .... looking at this, one gets the feeling that at one time, we may have
> simply done SetAt(), which checks for duplicates and empty slots for re-use,
> except that the underlying representation is O(N) for insert due to duplicate
> detection, and someone noticed that above N = 32 it gets slow. Then rather
than
> fixing the underlying representation, they bounded the badness and instead
went
> to straight inserts, skipping the check for duplicates. But doesn't doing so
> violate the PDF spec in that dictionaries aren't supposed to have more than
one
> element with the same key?  
> 
> We should land this patch, though.  I think you are right in capturing the
> intent.  LGTM.
> 
> As a follow-up, I'm going to see what it would take to make your Dictionaries
> based upon std::map.

OK. LGTM for resolving the compilation error. We can check the related code as a
follow-up.

[Tom Sepez, 2014-10-29 22:31:24]

Committed patchset #2 (id:20001) manually as
f3a5d3ddf7b492e5e8f64cb2ee98a53b5520499d (presubmit successful).