Summary: | [Symbian] RVCT fails to compile [U|I]nt[8|16|32]Array.h | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Laszlo Gombos <laszlo.gombos> | ||||||||||||||||||
Component: | Platform | Assignee: | qi <qi.2.zhang> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | commit-queue, cshu, darin, dbates, ddkilzer, dino, hausmann, kamaji, koshuin, laszlo.gombos, s.mathur, webkit.review.bot | ||||||||||||||||||
Priority: | P3 | Keywords: | Qt, QtTriaged | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||
OS: | S60 3rd edition | ||||||||||||||||||||
Attachments: |
|
Description
Laszlo Gombos
2011-01-04 07:51:36 PST
Haven't tested but from previous experience the using declaration won't compile on WINSCW either. There is a related problem introduced in http://trac.webkit.org/changeset/74775. Using declaration in WebCore/rendering/RenderInline.h seems to be ignored by RVCT, which results in the following build error: "Y:\webkit\Source\WebCore\rendering\RenderInline.cpp", line 245: Error: #410 -D: protected function "WebCore::RenderBoxModelObject::setContinuation" is not a ccessible through a "WebCore::RenderBlock" pointer or object middleBlock->setContinuation(clone); ^ Y:\webkit\Source\WebCore\rendering\RenderInline.cpp: 0 warnings, 1 error Temporary workaround is to make setContinuation() public in RenderBoxModelObject.h. Created attachment 79200 [details] patch In RVCT 4, only Array(comment #1) has the compile issue, RenderInline (comment #2) is OK. In RVCT 2.2, RenderInline has the same issue as Comment #2 talked about. This fix only fixed RVCT 4 issue, since RVCT 4 will replace RVCT 2.2 soon. And I test this patch in RVCT 2.2 enviroment, it didn't generate any problem. Attachment 79200 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/html/canvas/Int32Array.h:45: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/html/canvas/Int8Array.h:47: Tab found; better to use spaces [whitespace/tab] [1]
Total errors found: 2 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 79203 [details]
patch2
Fixed the "Tab" issue.
Adding Darin and Daniel to get their input. An other option would be - which would make the code less cluttered but perhaps less efficient is to always take the RVCT path for all ports (and remove the other path). Comment on attachment 79203 [details]
patch2
At a certain point, dealing where certain C++ features don’t work, it’s unpleasant for the project to have to dumb down the code for that compiler.
I do think it would be slightly better to do this without the #if and with a comment explaining that we can’t use “using” here due to a bug in the RVCT compiler.
What version of RVCT are you using? (In reply to comment #8) > What version of RVCT are you using? RVCT 4, but it also happen in RVCT 2.2 Created attachment 79957 [details]
patch3
Based on comments to change a little bit.
Attachment 79957 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/html/canvas/Int32Array.h:42: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebCore/html/canvas/Int8Array.h:44: Tab found; better to use spaces [whitespace/tab] [1]
Total errors found: 2 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 79958 [details]
patch4
fix style issue.
Attachment 79958 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/html/canvas/Int32Array.h:42: Tab found; better to use spaces [whitespace/tab] [1]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 79960 [details]
patch5
Sorry, again.
Qi, I think we should use something more explanatory as a comment - I like Darin's wording here // Can’t use “using” here due to a bug in the RVCT compiler. In the ChangeLog please mention that there are no new tests as there is no new functionality introduced. Created attachment 79964 [details]
patch6
Little change based on comments. :)
Created attachment 79967 [details]
patch7
Modify ChangeLog.
I think the setter functions should be inside a platform conditionalized section -- #if COMPILER(RVCT) // Can’t use “using” here due to a bug in the RVCT compiler. void set(unsigned index, double value) { return IntegralTypedArrayBase<int>::set(index, value); } void set(TypedArrayBase<int>* array, unsigned offset, ExceptionCode& ec) { return TypedArrayBase<int>::set(array, offset, ec); } #else using TypedArrayBase<int>::set; using IntegralTypedArrayBase<int>::set; #endif .. Also, I think non-ascii characters are against style guidelines. (though if they are, it obviously isn't caught by check-webkit-style) I know some compilers don't like non-ascii characters. (In reply to comment #18) > I think the setter functions should be inside a platform conditionalized NVM -- Laszlo jogged my memory -- we want to get rid of #if COMPILER(x) code as much as we can (for maintenance reasons)... However, non-ascii characters I believe are still an issue. Comment on attachment 79967 [details]
patch7
Almost there.
I think the ChangeLog should explain why the change is made and that we're not special-casing for RVCT.
You can also replace the non-ascii quotation marks with ascii onces, although the try-bots seems to be ok.
Is there any reason you changed the order of the two functions ?
Created attachment 80054 [details]
patch8
Comments and style change.
Comment on attachment 80054 [details]
patch8
LGTM now, r+.
Comment on attachment 80054 [details] patch8 Clearing flags on attachment: 80054 Committed r76592: <http://trac.webkit.org/changeset/76592> All reviewed patches have been landed. Closing bug. |