Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2023173002:
Fix some signed/unsigned comparisons in xfa_fm2jscontext.cpp (Closed)
|
Created:
4 years, 6 months ago by Oliver Chang Modified:
4 years, 6 months ago CC:
pdfium-reviews_googlegroups.com Base URL:
https://pdfium.googlesource.com/pdfium.git@master Target Ref:
refs/heads/master Project:
pdfium Visibility:
Public. |
DescriptionFix some signed/unsigned comparisons in xfa_fm2jscontext.cpp
Looks like this causes compile warnings on ARM where wchar_t is unsigned.
Committed: https://pdfium.googlesource.com/pdfium/+/f6be145f54bf44810974e43e9554c756c9730bb6
Patch Set 1 #
Total comments: 7
Patch Set 2 : more cleanup #
Total comments: 2
Patch Set 3 : static_assert #Messages
Total messages: 13 (5 generated)
ochang@chromium.org changed reviewers: + dsinclair@chromium.org, thestig@chromium.org
PTAL. Build failures from attempting to roll:
../../third_party/pdfium/xfa/fxfa/fm2js/xfa_fm2jscontext.cpp: In function 'void
{anonymous}::AlternateDateTimeSymbols(CFX_WideString&, const CFX_WideString&,
const uint8_t*)':
../../third_party/pdfium/xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:397:53: error:
comparison between signed and unsigned integer expressions
[-Werror=sign-compare]
if (!bInConstRange && (n = wc - L'A') >= 0 && n <= (L'a' - L'A')) {
^
../../third_party/pdfium/xfa/fxfa/fm2js/xfa_fm2jscontext.cpp: In static member
function 'static void CXFA_FM2JSContext::EncodeURL(const CFX_ByteStringC&,
CFX_ByteTextBuf&)':
../../third_party/pdfium/xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:4233:17: error:
comparison between signed and unsigned integer expressions
[-Werror=sign-compare]
while (ch >= iRadix) {
Description was changed from ========== Fix some signed/unsigned comparisons in xfa_fm2jscontext.cpp Looks like this compile warnings on ARM where wchar_t is unsigned. ========== to ========== Fix some signed/unsigned comparisons in xfa_fm2jscontext.cpp Looks like this causes compile warnings on ARM where wchar_t is unsigned. ==========
https://codereview.chromium.org/2023173002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right): https://codereview.chromium.org/2023173002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:398: if (!bInConstRange && (n = wc - L'A') >= 0 && n <= (L'a' - L'A')) { How about we get rid of |n| and do: if (... wc >= L'A' && wc <= L'a') { uint8_t nAlt = pAltTable[wc - L'A']; https://codereview.chromium.org/2023173002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:399: int32_t nAlt = static_cast<int32_t>(pAltTable[n]); Just make |nAlt| a uint8_t? https://codereview.chromium.org/2023173002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:399: int32_t nAlt = static_cast<int32_t>(pAltTable[n]); BTW, both of the possible tables, |g_sAltTable_Date| and |g_sAltTable_Time|, seem to have too many entries. Am I right? https://codereview.chromium.org/2023173002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:4232: FX_WCHAR iRadix = 16; const?
https://codereview.chromium.org/2023173002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right): https://codereview.chromium.org/2023173002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:398: if (!bInConstRange && (n = wc - L'A') >= 0 && n <= (L'a' - L'A')) { On 2016/06/01 05:27:04, Lei Zhang wrote: > How about we get rid of |n| and do: > > if (... wc >= L'A' && wc <= L'a') { > uint8_t nAlt = pAltTable[wc - L'A']; Done. https://codereview.chromium.org/2023173002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:399: int32_t nAlt = static_cast<int32_t>(pAltTable[n]); On 2016/06/01 05:27:04, Lei Zhang wrote: > Just make |nAlt| a uint8_t? Done. Also removed the unused entries (now 33 entries each for both). https://codereview.chromium.org/2023173002/diff/1/xfa/fxfa/fm2js/xfa_fm2jscon... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:4232: FX_WCHAR iRadix = 16; On 2016/06/01 05:27:04, Lei Zhang wrote: > const? Done.
lgtm https://codereview.chromium.org/2023173002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j... File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right): https://codereview.chromium.org/2023173002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:361: const uint8_t g_sAltTable_Date[] = { Maybe static_assert(FX_ArraySize(g_sAltTable_Date) == L'a' - L'A" + 1) ? <- hope I did that right.
https://codereview.chromium.org/2023173002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j... File xfa/fxfa/fm2js/xfa_fm2jscontext.cpp (right): https://codereview.chromium.org/2023173002/diff/20001/xfa/fxfa/fm2js/xfa_fm2j... xfa/fxfa/fm2js/xfa_fm2jscontext.cpp:361: const uint8_t g_sAltTable_Date[] = { On 2016/06/01 18:01:35, Lei Zhang wrote: > Maybe static_assert(FX_ArraySize(g_sAltTable_Date) == L'a' - L'A" + 1) ? <- hope > I did that right. Done.
The CQ bit was checked by ochang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2023173002/#ps40001 (title: "static_assert")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023173002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023173002/40001
lgtm++
Message was sent while issue was closed.
Description was changed from ========== Fix some signed/unsigned comparisons in xfa_fm2jscontext.cpp Looks like this causes compile warnings on ARM where wchar_t is unsigned. ========== to ========== Fix some signed/unsigned comparisons in xfa_fm2jscontext.cpp Looks like this causes compile warnings on ARM where wchar_t is unsigned. Committed: https://pdfium.googlesource.com/pdfium/+/f6be145f54bf44810974e43e9554c756c973... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://pdfium.googlesource.com/pdfium/+/f6be145f54bf44810974e43e9554c756c973... |
