Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1481503002:
Use c++ style casts in CPWL_Color::CPWL_Color() (Closed)
|
Created:
5 years ago by jun_fang Modified:
5 years ago CC:
pdfium-reviews_googlegroups.com Base URL:
https://pdfium.googlesource.com/pdfium.git@xfa Target Ref:
refs/heads/xfa Visibility:
Public. |
DescriptionUse c++ style casts in CPWL_Color::CPWL_Color()
BUG=pdfium:281
R=tsepez@chromium.org
Committed: https://pdfium.googlesource.com/pdfium/+/cd0e00a305479505b9c759e86bc8ebfd4ecdc4f4
Patch Set 1 #
Messages
Total messages: 11 (4 generated)
Description was changed from ========== Fix a crasher in CPWL_Color::CPWL_Color() BUG=pdfium:281 ========== to ========== Fix a crasher in CPWL_Color::CPWL_Color() It was caused by an invalid type casting. BUG=pdfium:281 ==========
jun_fang@foxitsoftware.com changed reviewers: + kai_jing@foxitsoftware.com, thestig@chromium.org, tsepez@chromium.org
On 2015/11/25 10:21:46, jun_fang wrote: > mailto:jun_fang@foxitsoftware.com changed reviewers: > + mailto:kai_jing@foxitsoftware.com, mailto:thestig@chromium.org, mailto:tsepez@chromium.org Hi Tom and Lei, Please review this CL. You will find that it still causes a crasher even this CL is applied. That's another bug fixed in https://codereview.chromium.org/1465423002/.
Ok, but this doesn't change the behaviour of the code. static_cast is preferred only because it will cause a compilation error if the cast is illegal (e.g between unrelated types). That this compiles without having to change the types means that the cast is legal and the c-style cast would be equivalent in this one case. You can land this, but change the title to something like "use c++ style casts in ..." since it doesn't actually fix anything.
On 2015/11/25 17:44:57, Tom Sepez wrote: > Ok, but this doesn't change the behaviour of the code. static_cast is preferred > only because it will cause a compilation error if the cast is illegal (e.g > between unrelated types). That this compiles without having to change the types > means that the cast is legal and the c-style cast would be equivalent in this > one case. > > You can land this, but change the title to something like "use c++ style casts > in ..." since it doesn't actually fix anything. static_cast can avoid the crasher reported in pdfium:281. For some reason, m_mapXMLNodeToParseContext.GetValueAt(pXMLNode) will return a invalid address. it will cause a crasher. After static_cast is used, pStyle is null. It will be processed by the later error handling. In this way, the test pdf file can be rendered. However, some texts are missing in the generated images. I have raised another function bug to track it.
On 2015/11/27 08:55:49, jun_fang wrote: > On 2015/11/25 17:44:57, Tom Sepez wrote: > > Ok, but this doesn't change the behaviour of the code. static_cast is > preferred > > only because it will cause a compilation error if the cast is illegal (e.g > > between unrelated types). That this compiles without having to change the > types > > means that the cast is legal and the c-style cast would be equivalent in this > > one case. > > > > You can land this, but change the title to something like "use c++ style casts > > in ..." since it doesn't actually fix anything. > > static_cast can avoid the crasher reported in pdfium:281. For some reason, > m_mapXMLNodeToParseContext.GetValueAt(pXMLNode) will return a invalid address. > it will cause a crasher. After static_cast is used, pStyle is null. > It will be processed by the later error handling. In this way, the test pdf > file can be rendered. However, some texts are missing in the generated images. > I have raised another function bug to track it. Unfortunately, what you're describing here doesn't make sense according to the rules of C++ and shouldn't be possible. There is some other issue going on that you're going to need to track down ...
Description was changed from ========== Fix a crasher in CPWL_Color::CPWL_Color() It was caused by an invalid type casting. BUG=pdfium:281 ========== to ========== Use c++ style casts in CPWL_Color::CPWL_Color() BUG=pdfium:281 ==========
On 2015/11/27 16:57:52, Tom Sepez wrote: > On 2015/11/27 08:55:49, jun_fang wrote: > > On 2015/11/25 17:44:57, Tom Sepez wrote: > > > Ok, but this doesn't change the behaviour of the code. static_cast is > > preferred > > > only because it will cause a compilation error if the cast is illegal (e.g > > > between unrelated types). That this compiles without having to change the > > types > > > means that the cast is legal and the c-style cast would be equivalent in > this > > > one case. > > > > > > You can land this, but change the title to something like "use c++ style > casts > > > in ..." since it doesn't actually fix anything. > > > > static_cast can avoid the crasher reported in pdfium:281. For some reason, > > m_mapXMLNodeToParseContext.GetValueAt(pXMLNode) will return a invalid address. > > it will cause a crasher. After static_cast is used, pStyle is null. > > It will be processed by the later error handling. In this way, the test pdf > > file can be rendered. However, some texts are missing in the generated images. > > I have raised another function bug to track it. > > Unfortunately, what you're describing here doesn't make sense according to the > rules of C++ and shouldn't be possible. There is some other issue going on that > you're going to need to track down ... Hi Tom, Update the tile as you suggested.
lgtm
Description was changed from ========== Use c++ style casts in CPWL_Color::CPWL_Color() BUG=pdfium:281 ========== to ========== Use c++ style casts in CPWL_Color::CPWL_Color() BUG=pdfium:281 R=tsepez@chromium.org Committed: https://pdfium.googlesource.com/pdfium/+/cd0e00a305479505b9c759e86bc8ebfd4ecd... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as cd0e00a305479505b9c759e86bc8ebfd4ecdc4f4 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
