Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1077083003:
Fix the noisiest variable shadowing warnings in pdfium. (Closed)
|
Created:
5 years, 8 months ago by brucedawson Modified:
5 years, 8 months ago Reviewers:
Tom Sepez CC:
pdfium-reviews_googlegroups.com Base URL:
https://pdfium.googlesource.com/pdfium.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionFix the noisiest variable shadowing warnings in pdfium.
Three functions in fx_coordinates.h account for 60% of the warnings
when building with VS 2015, due to variable shadowing. Renaming the
function parameters is safe, resolves the warnings, and reduces
confusion.
R=tsepez@chromium.org
BUG=440500
Committed: https://pdfium.googlesource.com/pdfium/+/9c7b0940569ee5eb1794e8db4e47ecaf3a64315d
Patch Set 1 #
Total comments: 22
Patch Set 2 : Put statements on separate lines. #Messages
Total messages: 9 (0 generated)
Take a look when Rietveld starts working again? It's a simple patch to reduce the pdfium VS 2015 warning noise by 60%.
https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... File core/include/fxcrt/fx_coordinates.h (right): https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:137: FXT_PSV::x = newx, FXT_PSV::y = newy; Can we get rid of the FXT_PSV:: qualifiers throughout here now that the shadowing is gone? nit: one statement per line, lose the comma.
https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... File core/include/fxcrt/fx_coordinates.h (right): https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:135: void Set(baseType newx, baseType newy) why do we even need this method? Isn't there one in the base class that does the same thing?
https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... File core/include/fxcrt/fx_coordinates.h (right): https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:135: void Set(baseType newx, baseType newy) A quick check shows that it is needed. fpdf_text_int.cpp(1925) (and possibly other locations) fails to compile if it is commented out. https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:137: FXT_PSV::x = newx, FXT_PSV::y = newy; I started doing that initially, and I'd be happy to do it. I only stopped because the other methods use FXT_PSV:: as well, so changing it would be inconsistent, and changing it throughout the class seemed like excessive churn. Thoughts?
On 2015/04/10 21:29:51, brucedawson wrote: > https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... > File core/include/fxcrt/fx_coordinates.h (right): > > https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... > core/include/fxcrt/fx_coordinates.h:135: void Set(baseType newx, baseType newy) > A quick check shows that it is needed. fpdf_text_int.cpp(1925) (and possibly > other locations) fails to compile if it is commented out. > > https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... > core/include/fxcrt/fx_coordinates.h:137: FXT_PSV::x = newx, FXT_PSV::y = newy; > I started doing that initially, and I'd be happy to do it. I only stopped > because the other methods use FXT_PSV:: as well, so changing it would be > inconsistent, and changing it throughout the class seemed like excessive churn. > > Thoughts? Land it as is. There's so much else wrong with this file.
For example: https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... File core/include/fxcrt/fx_coordinates.h (right): https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:12: template<class baseType> class CFX_PSVTemplate; I don't think there's a need to forward declare any of these. https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:14: template<class baseType> class CFX_PRLTemplate; I didn't see an implementation of CFX_PRLTemplate anywhere. https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:16: template<class baseType> class CFX_ETemplate; CFX_ETemplate appears unused. https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:17: template<class baseType> class CFX_ATemplate; CFX_ATemplate appears unused. https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:18: template<class baseType> class CFX_RRTemplate; CFX_RRTemplate appears unused. https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:25: typedef CFX_PSVTemplate<baseType> FXT_POINT; FXT_POINT appears unused, shadowed in subclass. maybe used once subclass loses it def'n/ https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:26: typedef CFX_PSVTemplate<baseType> FXT_SIZE; FXT_SIZE ditto https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:27: void Set(baseType x, baseType y) Same issue with shadowing parameters. Then we can lose the PSV:: https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:71: friend FX_BOOL operator == (const FXT_PSV &obj1, const FXT_PSV &obj2) why "friend"? x, y are public. I think they wanted this to be a function, not a class method, since it is the two-arg form? I'm confused. https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:118: typedef CFX_PSVTemplate<FX_INT32> CFX_Size; I'm deeply offended by anyone who thinks a size contains x,y instead of w,h. https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:122: typedef CFX_PSVTemplate<FX_INT32> * FX_LPPOINT; can we use the typdef's above here to save some typing and ensure consistency. https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:126: #define CFX_FloatPoint CFX_PointF should be typedef. https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:127: template<class baseType> probably should be typename baseType, since we instantiate it only with things that are intrinsically promotable to float (below). https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:132: typedef CFX_PSVTemplate<baseType> FXT_POINT; nit: this types are shadows? https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:515: friend FX_BOOL operator == (const FXT_RECT &rc1, const FXT_RECT &rc2) ditto on friend. https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:612: class CFX_FloatRect : public CFX_Object And yet this isn't a template. https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:867: FX_FLOAT GetA() const |a| is public and we don't need this, or make |a| protected below. https://codereview.chromium.org/1077083003/diff/1/core/include/fxcrt/fx_coord... core/include/fxcrt/fx_coordinates.h:904: #define CFX_AffineMatrix CFX_Matrix typedef.
Sounds good. Magic four letters needed?
On 2015/04/10 21:34:15, brucedawson wrote: > Sounds good. Magic four letters needed? LGTM
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as 9c7b0940569ee5eb1794e8db4e47ecaf3a64315d (presubmit successful). |
