Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2235743003:
Minor cleanup of fpdf_cmaps (Closed)
DescriptionMinor cleanup of fpdf_cmaps
Cleanup some dead code and c-style casts.
Committed: https://pdfium.googlesource.com/pdfium/+/1194561d5d83869edecf6a1f402122a59955f0b7
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add todo #Messages
Total messages: 15 (8 generated)
The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dsinclair@chromium.org changed reviewers: + thestig@chromium.org, weili@chromium.org
PTAL. https://codereview.chromium.org/2235743003/diff/1/core/fpdfapi/fpdf_cmaps/fpd... File core/fpdfapi/fpdf_cmaps/fpdf_cmaps.cpp (right): https://codereview.chromium.org/2235743003/diff/1/core/fpdfapi/fpdf_cmaps/fpd... core/fpdfapi/fpdf_cmaps/fpdf_cmaps.cpp:124: while (1) { This loop only returns, never breaks. So, the while below can never execute.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2235743003/diff/1/core/fpdfapi/fpdf_cmaps/fpd... File core/fpdfapi/fpdf_cmaps/fpdf_cmaps.cpp (right): https://codereview.chromium.org/2235743003/diff/1/core/fpdfapi/fpdf_cmaps/fpd... core/fpdfapi/fpdf_cmaps/fpdf_cmaps.cpp:124: while (1) { On 2016/08/10 14:05:53, dsinclair wrote: > This loop only returns, never breaks. So, the while below can never execute. Looks like a bug to me, mDWordMap should be examined in some way. Maybe add a TODO?
https://codereview.chromium.org/2235743003/diff/1/core/fpdfapi/fpdf_cmaps/fpd... File core/fpdfapi/fpdf_cmaps/fpdf_cmaps.cpp (right): https://codereview.chromium.org/2235743003/diff/1/core/fpdfapi/fpdf_cmaps/fpd... core/fpdfapi/fpdf_cmaps/fpdf_cmaps.cpp:124: while (1) { On 2016/08/10 17:46:44, Wei Li wrote: > On 2016/08/10 14:05:53, dsinclair wrote: > > This loop only returns, never breaks. So, the while below can never execute. > > Looks like a bug to me, mDWordMap should be examined in some way. Maybe add a > TODO? In which case, should I leave the while() below in as, in theory, it will be needed at the point we figure out what this is doing?
https://codereview.chromium.org/2235743003/diff/1/core/fpdfapi/fpdf_cmaps/fpd... File core/fpdfapi/fpdf_cmaps/fpdf_cmaps.cpp (right): https://codereview.chromium.org/2235743003/diff/1/core/fpdfapi/fpdf_cmaps/fpd... core/fpdfapi/fpdf_cmaps/fpdf_cmaps.cpp:124: while (1) { On 2016/08/10 18:47:39, dsinclair wrote: > On 2016/08/10 17:46:44, Wei Li wrote: > > On 2016/08/10 14:05:53, dsinclair wrote: > > > This loop only returns, never breaks. So, the while below can never execute. > > > > Looks like a bug to me, mDWordMap should be examined in some way. Maybe add a > > TODO? > > > In which case, should I leave the while() below in as, in theory, it will be > needed at the point we figure out what this is doing? When getting charcode from CID, I think both pMap->m_WordMap and pMap->m_DWordMap should be checked if present. You can either keep or remove the second loop, but pls add a TODO to figure out this later.
https://codereview.chromium.org/2235743003/diff/1/core/fpdfapi/fpdf_cmaps/fpd... File core/fpdfapi/fpdf_cmaps/fpdf_cmaps.cpp (right): https://codereview.chromium.org/2235743003/diff/1/core/fpdfapi/fpdf_cmaps/fpd... core/fpdfapi/fpdf_cmaps/fpdf_cmaps.cpp:124: while (1) { On 2016/08/10 18:53:09, Wei Li wrote: > On 2016/08/10 18:47:39, dsinclair wrote: > > On 2016/08/10 17:46:44, Wei Li wrote: > > > On 2016/08/10 14:05:53, dsinclair wrote: > > > > This loop only returns, never breaks. So, the while below can never > execute. > > > > > > Looks like a bug to me, mDWordMap should be examined in some way. Maybe add > a > > > TODO? > > > > > > In which case, should I leave the while() below in as, in theory, it will be > > needed at the point we figure out what this is doing? > > When getting charcode from CID, I think both pMap->m_WordMap and > pMap->m_DWordMap should be checked if present. You can either keep or remove the > second loop, but pls add a TODO to figure out this later. Done.
The CQ bit was checked by dsinclair@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from weili@chromium.org Link to the patchset: https://codereview.chromium.org/2235743003/#ps20001 (title: "Add todo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Minor cleanup of fpdf_cmaps Cleanup some dead code and c-style casts. ========== to ========== Minor cleanup of fpdf_cmaps Cleanup some dead code and c-style casts. Committed: https://pdfium.googlesource.com/pdfium/+/1194561d5d83869edecf6a1f402122a59955... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://pdfium.googlesource.com/pdfium/+/1194561d5d83869edecf6a1f402122a59955... |
