Bug 227436 – Remove "function declared ‘static’ but never defined" build warnings since r278971.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Joonghun Park]

Static functions should be put in cpp file so that it is visible only within the file, and header file should include extern functions
with single implementation, not static functions with different impls for each cpp files usually.

[Joonghun Park]

Created attachment 432366 [details]
Patch

[Michael Catanzaro]

*** Bug 227450 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

Comment on attachment 432366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432366&action=review

[5/491] Building CXX object Source/JavaScriptCore/CMakeFi...edSources/unified-sources/UnifiedSource-f2e18ffc-27.cpp.o
In file included from ../../Source/JavaScriptCore/runtime/LiteralParser.cpp:39,
                 from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-f2e18ffc-27.cpp:5:
JavaScriptCore/DerivedSources/KeywordLookup.h:87:27: warning: ‘bool JSC::cannotBeIdentPartOrEscapeStart(LChar)’ declared ‘static’ but never defined [-Wunused-function]
   87 | static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar);
      |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
JavaScriptCore/DerivedSources/KeywordLookup.h:88:27: warning: ‘bool JSC::cannotBeIdentPartOrEscapeStart(UChar)’ declared ‘static’ but never defined [-Wunused-function]
   88 | static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(UChar);
      |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

These warnings were introduced in r278971 "[JSC] Optimize JSON.parse with small data by changing Identifier pool mechanism." Problem is KeywordLookup.h does not define these functions and expects the translation unit that it is #included in to do so. Previously that was only Lexer.cpp. Problem is it's now also #included from LiteralParser.cpp, which does not define cannotBeIdentPartOrEscapeStart(LChar) or cannotBeIdentPartOrEscapeStart(UChar).

> Source/JavaScriptCore/parser/Lexer.cpp:788
> -static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar c)
> +ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar c)

Problem is using ALWAYS_INLINE for a function that's not defined in the header doesn't make sense. We need to either:

 (a) Also remove ALWAYS_INLINE, or
 (b) Move the definitions of these functions to KeywordLookup.h (which is generated by KeywordLookupGenerator.py)

[Yusuke Suzuki]

Comment on attachment 432366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432366&action=review

>> Source/JavaScriptCore/parser/Lexer.cpp:788
>> +ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar c)
> 
> Problem is using ALWAYS_INLINE for a function that's not defined in the header doesn't make sense. We need to either:
> 
>  (a) Also remove ALWAYS_INLINE, or
>  (b) Move the definitions of these functions to KeywordLookup.h (which is generated by KeywordLookupGenerator.py)

Please do not remove ALWAYS_INLINE. They are *super* critical performance-sensitive function, and removing that can cause severe perf impact on several benchmarks.

[Michael Catanzaro]

Comment on attachment 432366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432366&action=review

>>> Source/JavaScriptCore/parser/Lexer.cpp:788
>>> +ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar c)
>> 
>> Problem is using ALWAYS_INLINE for a function that's not defined in the header doesn't make sense. We need to either:
>> 
>>  (a) Also remove ALWAYS_INLINE, or
>>  (b) Move the definitions of these functions to KeywordLookup.h (which is generated by KeywordLookupGenerator.py)
> 
> Please do not remove ALWAYS_INLINE. They are *super* critical performance-sensitive function, and removing that can cause severe perf impact on several benchmarks.

Let's do (b) then.

[Michael Catanzaro]

Hi Joonghun, let me know if you want to submit another patch. Otherwise, I can try.

[Joonghun Park]

(In reply to Michael Catanzaro from comment #6)
> Hi Joonghun, let me know if you want to submit another patch. Otherwise, I
> can try.

Hi Michael, I will do the (b) in the next patchset.
Please review that change:)

[Joonghun Park]

(In reply to Joonghun Park from comment #7)
> (In reply to Michael Catanzaro from comment #6)
> > Hi Joonghun, let me know if you want to submit another patch. Otherwise, I
> > can try.
> 
> Hi Michael, I will do the (b) in the next patchset.
> Please review that change:)

Hi, Michael.

While I was trying to move cannotBeIdentPartOrEscapeStart,
I found that the depedent functions and variables seemed not appropriate to move to KeywordLookup.h.

For example, if we move isSingleCharacterIdentPart(UChar c),
then we need to move isIdentPart(CharacterType c) and typesOfLatin1Characters[256] array too, and it seems that typesOfLatin1Characters  should be used only in Lexer.cpp translation unit.

And if we only move the declaration of isIdentStart(CharacterType c),
then it goes to the same place we are currently in.

So I looked around some articles, 
1) https://stackoverflow.com/questions/5057021/why-are-c-inline-functions-in-the-header
2) https://en.cppreference.com/w/cpp/language/inline

and it seems that inline function's definition doesn't have to be placed in header file as long as all the definitions are identical(that's why usually inline function's definition is put in header, to keep it identical for all cpp files including the header file).

So I wonder about 2 options below.

a) If it's ok to keep the inline definitions in cpp files as it is now,
and just remove static keyword(as the current patchset does).


b) Or maybe simply we can keep 'static inline' keywords as it is now,
and just add empty definitions in LiteralParser.cpp as below.

static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar)
{
    return false;
}

static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(UChar)
{
    return false;
}

I confirmed that doing this removed the build warnings,
warning: ‘bool JSC::cannotBeIdentPartOrEscapeStart()’ declared ‘static’ but never defined [-Wunused-function],
but the cons is that it seems those functions has no meaning in LiteralParser.cpp.

[Michael Catanzaro]

(In reply to Joonghun Park from comment #8)
> So I wonder about 2 options below.
> 
> a) If it's ok to keep the inline definitions in cpp files as it is now,
> and just remove static keyword(as the current patchset does).

I don't know. Under your proposal, these functions would not be defined in LiteralParser.cpp at all. I would avoid this.

> b) Or maybe simply we can keep 'static inline' keywords as it is now,
> and just add empty definitions in LiteralParser.cpp as below.
> 
> static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar)
> {
>     return false;
> }
> 
> static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(UChar)
> {
>     return false;
> }

(b) would definitely be OK. I would make it RELEASE_ASSERT_NOT_REACHED() rather than return false. That should be perfectly safe.

[Joonghun Park]

(In reply to Michael Catanzaro from comment #9)
> (In reply to Joonghun Park from comment #8)
> > So I wonder about 2 options below.
> > 
> > a) If it's ok to keep the inline definitions in cpp files as it is now,
> > and just remove static keyword(as the current patchset does).
> 
> I don't know. Under your proposal, these functions would not be defined in
> LiteralParser.cpp at all. I would avoid this.
> 
> > b) Or maybe simply we can keep 'static inline' keywords as it is now,
> > and just add empty definitions in LiteralParser.cpp as below.
> > 
> > static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(LChar)
> > {
> >     return false;
> > }
> > 
> > static ALWAYS_INLINE bool cannotBeIdentPartOrEscapeStart(UChar)
> > {
> >     return false;
> > }
> 
> (b) would definitely be OK. I would make it RELEASE_ASSERT_NOT_REACHED()
> rather than return false. That should be perfectly safe.

Thank you for your review. I will upload a patchset with (b).

[Joonghun Park]

Created attachment 432484 [details]
Patch

[Michael Catanzaro]

Comment on attachment 432484 [details]
Patch

Maybe Yusuke will have a better proposal for how to reorganize the code to avoid this, but it seems like a pretty good solution for now. Thanks Joonghun!

[Joonghun Park]

(In reply to Michael Catanzaro from comment #12)
> Comment on attachment 432484 [details]
> Patch
> 
> Maybe Yusuke will have a better proposal for how to reorganize the code to
> avoid this, but it seems like a pretty good solution for now. Thanks
> Joonghun!

I got it:) Thank you for your help, Michael. I will land this for now.

[EWS]

Committed r279393 (239258@main): <https://commits.webkit.org/239258@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432484 [details].

[Radar WebKit Bug Importer]

<rdar://problem/79939462>