Bug 51873 – [Symbian] RVCT fails to compile [U|I]nt[8|16|32]Array.h

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Laszlo Gombos]

compile    : WebCore\bindings\js\JSDOMWindowCustom.cpp
   "Y:/webkit/WebCore/html/canvas/Int16Array.h", line 41: Error:  #1001: class member designated by a using-declaration must be visible in a direct base class
         using TypedArrayBase<short>::set;
               ^
   "Y:/webkit/WebCore/html/canvas/Int8Array.h", line 42: Error:  #1001: class member designated by a using-declaration must be visible in a direct base class
         using TypedArrayBase<signed char>::set;
               ^
   "Y:/webkit/WebCore/html/canvas/Uint16Array.h", line 42: Error:  #1001: class member designated by a using-declaration must be visible in a direct base class
         using TypedArrayBase<unsigned short>::set;
               ^
   "Y:/webkit/WebCore/html/canvas/Uint32Array.h", line 42: Error:  #1001: class member designated by a using-declaration must be visible in a direct base class
         using TypedArrayBase<unsigned int>::set;
               ^
Change introduced by bug 47187 (http://trac.webkit.org/changeset/69122) is probably not a solution and it might have to be reverted. At the moment BLOB and 3D_CANVAS are the only two features that require compilation of these files so disabling these features are a temporary workaround. However it is expected that more and more WebCore feature will rely on these files.

[Janne Koskinen]

Haven't tested but from previous experience the using declaration won't compile on WINSCW either.

[Laszlo Gombos]

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.

[qi]

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.

[WebKit Review Bot]

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.

[qi]

Created attachment 79203 [details]
patch2

Fixed the "Tab" issue.

[Laszlo Gombos]

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).

[Darin Adler]

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.

[Daniel Bates]

What version of RVCT are you using?

[qi]

(In reply to comment #8)
> What version of RVCT are you using?

RVCT 4, but it also happen in RVCT 2.2

[qi]

Created attachment 79957 [details]
patch3

Based on comments to change a little bit.

[WebKit Review Bot]

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.

[qi]

Created attachment 79958 [details]
patch4

fix style issue.

[WebKit Review Bot]

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.

[qi]

Created attachment 79960 [details]
patch5

Sorry, again.

[Laszlo Gombos]

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.

[qi]

Created attachment 79964 [details]
patch6

Little change based on comments. :)

[qi]

Created attachment 79967 [details]
patch7

Modify ChangeLog.

[Keith Kyzivat]

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.

[Keith Kyzivat]

(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.

[Laszlo Gombos]

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 ?

[qi]

Created attachment 80054 [details]
patch8

Comments and style change.

[Laszlo Gombos]

Comment on attachment 80054 [details]
patch8

LGTM now, r+.

[WebKit Commit Bot]

Comment on attachment 80054 [details]
patch8

Clearing flags on attachment: 80054

Committed r76592: <http://trac.webkit.org/changeset/76592>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.