RVCT compile error in LiteralParser.cpp

[Kwang Yul Seo]

RVCT emits erros in compiling LiteralParser.cpp: 

"..\JavaScriptCore\runtime\LiteralParser.cpp", line 140: Error:  #304: no instance of function template "JSC::isSafeStringCharacter" matches the argument list
            argument types are: (const UChar)
          while (m_ptr < m_end && isSafeStringCharacter<mode>(*m_ptr))
                                  ^
          detected during instantiation of "JSC::LiteralParser::TokenType JSC::LiteralParser::Lexer::lexString<parserMode>(JSC::LiteralParser::Lexer::LiteralParserToken &) [with parserMode=JSC::LiteralParser::StrictJSON]" at line 85
"..\JavaScriptCore\runtime\LiteralParser.cpp", line 140: Error:  #304: no instance of function template "JSC::isSafeStringCharacter" matches the argument list
            argument types are: (const UChar)
          while (m_ptr < m_end && isSafeStringCharacter<mode>(*m_ptr))
                                  ^
          detected during instantiation of "JSC::LiteralParser::TokenType JSC::LiteralParser::Lexer::lexString<parserMode>(JSC::LiteralParser::Lexer::LiteralParserToken &) [with parserMode=JSC::LiteralParser::NonStrictJSON]" at line 86


I am not sure what happens here. RVCT just can't find the instance of isSafeStringCharacter with the argumenmt type const UCHAR.

After I change isSafeStringCharacter to a macro, the problem is fixed.

#if COMPILER(RVCT)
#define isSafeStringCharacter(mode, c) \
    (((c) >= ' ' && (mode == LiteralParser::StrictJSON || (c) <= 0xff) && (c) != '\\' && (c) != '"') || (c) == '\t')
#else

...

#if COMPILER(RVCT)
        while (m_ptr < m_end && isSafeStringCharacter(mode, *m_ptr))
#else

[Kwang Yul Seo]

Created attachment 39738 [details]
RVCT patch

Change isSafeStringCharacter to a macro.

[Kwang Yul Seo]

Created attachment 39739 [details]
RVCT patch with comments

Add comments to code.

[Oliver Hunt]

Comment on attachment 39739 [details]
RVCT patch with comments

A significant part of me wants to just reject this patch on the grounds that you really need to update your compiler -- for instance we basically don't accept patches to support gcc3 for instance.

That said, in its current form there's no question of accepting this patch as it basically results in two copies of the same code, and adds a platform ifdef to code that has no reason to have any ifdefs.  If we must use a macro then we should just replace the function with a macro, not maintain both a macro and function.

[Kwang Yul Seo]

Created attachment 39742 [details]
RVCT patch

I see there is COMPILER(RVCT) in Platform.h, so I assumed that RVCT is suppoprted. What's the official position on this?

I removed the platform ifdefs and replaced the static inline function with macro.

[Kwang Yul Seo]

I forgot to mention the version of RVCT. I use RVCT 4.0.

[Oliver Hunt]

Comment on attachment 39742 [details]
RVCT patch

This looks okay, but i'd like performance tests to confirm that there isn't a regression.  Can you test with run-sunspider on a mac running leopard or snow leopard?

[Kwang Yul Seo]

Yes, sure. I will run the sun-spider test and upload the result here. It will take a few hours because I need to setup the WebKit development environment for my MacBook.

[Oliver Hunt]

(In reply to comment #5)
> I forgot to mention the version of RVCT. I use RVCT 4.0.

I am told that gcc for ARM is actually a good compiler, it would be simpler in the long term to just use that -- by and large GCC is a vastly more complete and correct compiler, and obviously webkit always builds with it.

[Kwang Yul Seo]

Created attachment 39748 [details]
SunSpider results

I found no observable difference after applying the patch.

[Kwang Yul Seo]

I found that the problem is resolved with RVCT option "--gnu --no_parse_templates".

QT port with RVCT exactly does this:

https://bugs.webkit.org/show_bug.cgi?id=27348


Without "--no_parse_templates" option, there are many compile errors because of the RVCT template instatiation issue discussed in:

http://lists.macosforge.org/pipermail/webkit-dev/2008-September/004799.html

I think this patch is not required anymore.

[Eric Seidel (no email)]

Comment on attachment 39742 [details]
RVCT patch

Assuming I'm reading the last comment from Kwang correctly, this patch is no longer needed, and this bug can be closed as WONTFIX.  Is that correct?  Marking r- under that assumption.

[Kwang Yul Seo]

Yes, this patch is not needed anymore.