Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2469833002:
Clean up CPDF_SyntaxParser a little bit (Closed)
DescriptionClean up CPDF_SyntaxParser a little bit
- Added a private method to read a character.
- Added enum for parsing status.
- Deleted unused method.
Committed: https://pdfium.googlesource.com/pdfium/+/7f3a8c3c317b291b44521a6a0c4dd192ad2d5966
Patch Set 1 #Patch Set 2 : Replace C-cast with static_casts #
Total comments: 6
Patch Set 3 : Use enum class #Patch Set 4 : Rebase #Patch Set 5 : Manual rebase? #
Total comments: 12
Patch Set 6 : Comments #
Total comments: 1
Messages
Total messages: 31 (22 generated)
The CQ bit was checked by npm@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...
The CQ bit was checked by npm@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...
Description was changed from ========== Clean up CPDF_SyntaxParser a little bit - Added a private method to read a character. - Added enum for parsing status. - Deleted unused method. ========== to ========== Clean up CPDF_SyntaxParser a little bit - Added a private method to read a character. - Added enum for parsing status. - Deleted unused method. ==========
npm@chromium.org changed reviewers: + dsinclair@chromium.org, thestig@chromium.org, weili@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2469833002/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_syntax_parser.cpp (right): https://codereview.chromium.org/2469833002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:34: enum SyntaxParser_ReadStatus { Make this an enum class then you can drop the SyntaxParser_Read from the name prefixes below as they have to be fully qualified. Rename to just ReadStatus? Then the usage would be ReadStatus::Normal, ReadStatus::Backlash which reads nicely. https://codereview.chromium.org/2469833002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:68: FX_BOOL CPDF_SyntaxParser::ReadChar(FX_FILESIZE read_pos, uint32_t read_size) { Should this be ReadChars? It will potentially read more then a single character, yea? https://codereview.chromium.org/2469833002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:280: status = SyntaxParser_ReadNormal; Move this before the if() as it happens in both branches?
PTAL https://codereview.chromium.org/2469833002/diff/20001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_syntax_parser.cpp (right): https://codereview.chromium.org/2469833002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:34: enum SyntaxParser_ReadStatus { On 2016/11/01 19:14:42, dsinclair wrote: > Make this an enum class then you can drop the SyntaxParser_Read from the name > prefixes below as they have to be fully qualified. > > Rename to just ReadStatus? Then the usage would be ReadStatus::Normal, > ReadStatus::Backlash which reads nicely. Done. https://codereview.chromium.org/2469833002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:68: FX_BOOL CPDF_SyntaxParser::ReadChar(FX_FILESIZE read_pos, uint32_t read_size) { On 2016/11/01 19:14:42, dsinclair wrote: > Should this be ReadChars? It will potentially read more then a single character, > yea? But it still represents a single char, as it is called in from GetNextChar and GetCharAtBackward https://codereview.chromium.org/2469833002/diff/20001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:280: status = SyntaxParser_ReadNormal; On 2016/11/01 19:14:42, dsinclair wrote: > Move this before the if() as it happens in both branches? Done.
ping
The CQ bit was checked by npm@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/android/builds/...)
The CQ bit was checked by npm@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/android/builds/...) linux_asan_lsan on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_asan_lsan...) linux_no_v8 on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_no_v8/bui...) win on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/win/builds/2673)
The CQ bit was checked by npm@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_syntax_parser.cpp (right): https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:29: struct SearchTagRecord { No longer needed? https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:83: if (m_BufOffset >= pos || This same check is in 3 places. https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:87: if (static_cast<FX_FILESIZE>(read_size) > m_FileLen) std::min() https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:103: static_cast<FX_FILESIZE>(m_BufOffset + m_BufSize) <= pos) { For later, since it's pre-existing - I'm paranoid about integer overflows. https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:643: objnum == static_cast<uint32_t>(m_MetadataObjnum) Can we just make |m_MetadataObjnum| unsigned? https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:791: static_cast<size_t>(static_cast<FX_FILESIZE>(m_BufSize) > m_FileLen Something like: std::min(m_BufSize, static_cast<uint32_t>(m_FileLen);
PTAL https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_syntax_parser.cpp (right): https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:29: struct SearchTagRecord { On 2016/11/03 08:11:27, Lei Zhang wrote: > No longer needed? Done. https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:83: if (m_BufOffset >= pos || On 2016/11/03 08:11:27, Lei Zhang wrote: > This same check is in 3 places. Replaced with inline method. Only found it in 2 places. https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:87: if (static_cast<FX_FILESIZE>(read_size) > m_FileLen) On 2016/11/03 08:11:27, Lei Zhang wrote: > std::min() Done. https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:103: static_cast<FX_FILESIZE>(m_BufOffset + m_BufSize) <= pos) { On 2016/11/03 08:11:27, Lei Zhang wrote: > For later, since it's pre-existing - I'm paranoid about integer overflows. Agreed. Might need to add more checks to operations done by parser. https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:643: objnum == static_cast<uint32_t>(m_MetadataObjnum) On 2016/11/03 08:11:27, Lei Zhang wrote: > Can we just make |m_MetadataObjnum| unsigned? Done. https://codereview.chromium.org/2469833002/diff/70001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.cpp:791: static_cast<size_t>(static_cast<FX_FILESIZE>(m_BufSize) > m_FileLen On 2016/11/03 08:11:27, Lei Zhang wrote: > Something like: > > std::min(m_BufSize, static_cast<uint32_t>(m_FileLen); Done.
lgtm https://codereview.chromium.org/2469833002/diff/90001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_syntax_parser.h (right): https://codereview.chromium.org/2469833002/diff/90001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_syntax_parser.h:84: inline bool CheckPosition(FX_FILESIZE pos) { *shrug* I'm in the "let the compiler figure it out" camp.
The CQ bit was checked by npm@chromium.org
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 ========== Clean up CPDF_SyntaxParser a little bit - Added a private method to read a character. - Added enum for parsing status. - Deleted unused method. ========== to ========== Clean up CPDF_SyntaxParser a little bit - Added a private method to read a character. - Added enum for parsing status. - Deleted unused method. Committed: https://pdfium.googlesource.com/pdfium/+/7f3a8c3c317b291b44521a6a0c4dd192ad2d... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90001) as https://pdfium.googlesource.com/pdfium/+/7f3a8c3c317b291b44521a6a0c4dd192ad2d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
