Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1686153004:
Add a class to better represent PS operations. (Closed)
DescriptionAdd a class to better represent PS operations.
R=tsepez@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/f5f1399f1af3c5869bf6857a125552d4834c19da
Patch Set 1 #
Total comments: 13
Patch Set 2 : address comments #
Total comments: 7
Patch Set 3 : #Patch Set 4 : FX_PI #Messages
Total messages: 15 (3 generated)
thestig@chromium.org changed reviewers: + tsepez@chromium.org
lgtm https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... File core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp (right): https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:170: if (static_cast<int>(pEngine->Pop())) Ok, just curious, so 0.9 is false ... https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:202: const struct PDF_PSOpName { nit: put this up top in namespace { ??? https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:205: } PDF_PSOpNames[] = {{"add", PSOP_ADD}, {"sub", PSOP_SUB}, This should be a std::map<char*, PSOP>. How to make one without static initializers ... https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:245: CPDF_PSProc* pProc = new CPDF_PSProc; nit: if pProc is the unique ptr itself, we can just std::move it later on? https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:253: for (size_t i = 0; i < FX_ArraySize(PDF_PSOpNames); ++i) { Do range-based for loops work for POD arrays? for (const auto& opnamep : PDF_PSOpNames) just curious. https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:262: FX_FLOAT pd = FX_atof(word); nit: local not needed, maybe looks better if we have a unique_ptr pConst to std::move?
https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... File core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp (right): https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:205: } PDF_PSOpNames[] = {{"add", PSOP_ADD}, {"sub", PSOP_SUB}, On 2016/02/11 01:44:54, Tom Sepez wrote: > This should be a std::map<char*, PSOP>. How to make one without static > initializers ... Actually, it probably ought to be a map from CFX_ByteStringC ... And do initializer lists help here? I think so.
https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... File core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp (right): https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:170: if (static_cast<int>(pEngine->Pop())) On 2016/02/11 01:44:55, Tom Sepez wrote: > Ok, just curious, so 0.9 is false ... PSOP_TRUE pushes in 1, and PSOP_FALSE pushes in 0. https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:202: const struct PDF_PSOpName { On 2016/02/11 01:44:54, Tom Sepez wrote: > nit: put this up top in namespace { ??? There is no anonymous namespace. I added one. https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:205: } PDF_PSOpNames[] = {{"add", PSOP_ADD}, {"sub", PSOP_SUB}, On 2016/02/11 01:44:54, Tom Sepez wrote: > This should be a std::map<char*, PSOP>. How to make one without static > initializers ... Initialize one on first use? Or put them in order by |name| and do binary search? I'll punt on this for now. https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:245: CPDF_PSProc* pProc = new CPDF_PSProc; On 2016/02/11 01:44:54, Tom Sepez wrote: > nit: if pProc is the unique ptr itself, we can just std::move it later on? Done. https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:253: for (size_t i = 0; i < FX_ArraySize(PDF_PSOpNames); ++i) { On 2016/02/11 01:44:54, Tom Sepez wrote: > Do range-based for loops work for POD arrays? > for (const auto& opnamep : PDF_PSOpNames) > > just curious. It works. https://codereview.chromium.org/1686153004/diff/1/core/src/fpdfapi/fpdf_page/... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:262: FX_FLOAT pd = FX_atof(word); On 2016/02/11 01:44:54, Tom Sepez wrote: > nit: local not needed, maybe looks better if we have a unique_ptr pConst to > std::move? Done.
https://codereview.chromium.org/1686153004/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp (right): https://codereview.chromium.org/1686153004/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:73: class CPDF_PSOP { Thinking about this on the way home, are you sure this isn't overkill compared to struct CPDF_PSOP { Type type; PDF_PSOP op union { float f; PSProc * p; } } and just having an array of these, no pointers, mallocs, virtual fn calls, etc? https://codereview.chromium.org/1686153004/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:502: static FX_FLOAT PDF_Interpolate(FX_FLOAT x, Now that we're in namespace {, don't need static. https://codereview.chromium.org/1686153004/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:509: static FX_DWORD _GetBits32(const uint8_t* pData, int bitpos, int nbits) { here too https://codereview.chromium.org/1686153004/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:517: typedef struct { nit: as long as we're here, no typedef needed and line 521 too.
Or even:
struct CPDF_PSOP {
Type type;
union {
PDF_PSOP op;
float f;
PSProc * p;
}
}
https://codereview.chromium.org/1686153004/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp (left): https://codereview.chromium.org/1686153004/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:217: #define PI 3.1415926535897932384626433832795f Also this (from Bruce) https://randomascii.wordpress.com/2014/06/26/please-calculate-this-circles-ci...
I've gone a bit less overboard in patch set 3. https://codereview.chromium.org/1686153004/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp (left): https://codereview.chromium.org/1686153004/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:217: #define PI 3.1415926535897932384626433832795f On 2016/02/11 18:18:54, Tom Sepez wrote: > Also this (from Bruce) > https://randomascii.wordpress.com/2014/06/26/please-calculate-this-circles-ci... I added "#define _USE_MATH_DEFINES" and refuse to become a barista.
https://codereview.chromium.org/1686153004/diff/20001/core/src/fpdfapi/fpdf_p... File core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp (left): https://codereview.chromium.org/1686153004/diff/20001/core/src/fpdfapi/fpdf_p... core/src/fpdfapi/fpdf_page/fpdf_page_func.cpp:217: #define PI 3.1415926535897932384626433832795f On 2016/02/12 02:38:44, Lei Zhang wrote: > On 2016/02/11 18:18:54, Tom Sepez wrote: > > Also this (from Bruce) > > > https://randomascii.wordpress.com/2014/06/26/please-calculate-this-circles-ci... > > I added "#define _USE_MATH_DEFINES" and refuse to become a barista. BTW, there's also FX_PI, FWLTHEME_PI, and PWL_PI.
Description was changed from ========== Add classes to better represent PS operations. ========== to ========== Add a class to better represent PS operations. ==========
> BTW, there's also FX_PI, FWLTHEME_PI, and PWL_PI. I vote in favor of FX_PI, using the #define, despite the NO! in the linked article.
LGTM otherwise.
Description was changed from ========== Add a class to better represent PS operations. ========== to ========== Add a class to better represent PS operations. R=tsepez@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/f5f1399f1af3c5869bf6857a125552d4834c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as f5f1399f1af3c5869bf6857a125552d4834c19da (presubmit successful). |
