Bug 224616 - Refactor inline functions out of HashMapImpl.h into HashMapImplInlines.h.
Summary: Refactor inline functions out of HashMapImpl.h into HashMapImplInlines.h.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks: 224610
  Show dependency treegraph
 
Reported: 2021-04-15 11:19 PDT by Mark Lam
Modified: 2021-04-17 18:19 PDT (History)
16 users (show)

See Also:


Attachments
proposed patch. (69.57 KB, patch)
2021-04-15 11:49 PDT, Mark Lam
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2021-04-15 11:19:46 PDT
Also do the same for clients of HashMapImpl that require similar refactoring.  This fixes the #include of JSCJSValueInlines.h in HashMapImpl.h, as well as makes it easier to use other inline functions from other classes in the implementation of HashMapImpl's inline functions in the future.
Comment 1 Radar WebKit Bug Importer 2021-04-15 11:20:14 PDT
<rdar://problem/76713709>
Comment 2 Mark Lam 2021-04-15 11:49:18 PDT
Created attachment 426121 [details]
proposed patch.
Comment 3 Yusuke Suzuki 2021-04-15 11:55:44 PDT
Comment on attachment 426121 [details]
proposed patch.

r=me
Comment 4 Darin Adler 2021-04-15 12:08:32 PDT
Comment on attachment 426121 [details]
proposed patch.

Never noticed when this first was introduced: Seems unfortunate that WTF has WTF::HashMap and JavaScriptCore has unrelated JSC::HashMapImpl. Might be worth thinking about future name changes to clarify the relationship/distinction
Comment 5 Mark Lam 2021-04-15 12:20:34 PDT
Thanks for the review.  Landed in r276039: <http://trac.webkit.org/r276039>.
Comment 6 Michael Catanzaro 2021-04-16 14:42:25 PDT
New warnings:

[1215/5456] Building CXX object Source/JavaScriptCore/CMa...edSources/unified-sources/UnifiedSource-f0a787a9-11.cpp.o
In file included from ../../Source/JavaScriptCore/runtime/JSMap.h:28,
                 from ../../Source/JavaScriptCore/bytecode/SpeculatedType.cpp:39,
                 from JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-f0a787a9-11.cpp:5:
../../Source/JavaScriptCore/runtime/HashMapImpl.h:233:27: warning: ‘bool JSC::areKeysEqual(JSC::JSGlobalObject*, JSC::JSValue, JSC::JSValue)’ declared ‘static’ but never defined [-Wunused-function]
  233 | ALWAYS_INLINE static bool areKeysEqual(JSGlobalObject*, JSValue, JSValue);
      |                           ^~~~~~~~~~~~
../../Source/JavaScriptCore/runtime/HashMapImpl.h:238:31: warning: ‘uint32_t JSC::wangsInt64Hash(uint64_t)’ declared ‘static’ but never defined [-Wunused-function]
  238 | static ALWAYS_INLINE uint32_t wangsInt64Hash(uint64_t key);
      |

I think we could simply remove the declarations of these functions from HashMapImpl.h. If they're needed, better include HashMapImplInlines.h, yes?
Comment 7 Mark Lam 2021-04-16 14:50:05 PDT
(In reply to Michael Catanzaro from comment #6)
> New warnings:
> 
> [1215/5456] Building CXX object
> Source/JavaScriptCore/CMa...edSources/unified-sources/UnifiedSource-f0a787a9-
> 11.cpp.o
> In file included from ../../Source/JavaScriptCore/runtime/JSMap.h:28,
>                  from
> ../../Source/JavaScriptCore/bytecode/SpeculatedType.cpp:39,
>                  from
> JavaScriptCore/DerivedSources/unified-sources/UnifiedSource-f0a787a9-11.cpp:
> 5:
> ../../Source/JavaScriptCore/runtime/HashMapImpl.h:233:27: warning: ‘bool
> JSC::areKeysEqual(JSC::JSGlobalObject*, JSC::JSValue, JSC::JSValue)’
> declared ‘static’ but never defined [-Wunused-function]
>   233 | ALWAYS_INLINE static bool areKeysEqual(JSGlobalObject*, JSValue,
> JSValue);
>       |                           ^~~~~~~~~~~~
> ../../Source/JavaScriptCore/runtime/HashMapImpl.h:238:31: warning: ‘uint32_t
> JSC::wangsInt64Hash(uint64_t)’ declared ‘static’ but never defined
> [-Wunused-function]
>   238 | static ALWAYS_INLINE uint32_t wangsInt64Hash(uint64_t key);
>       |
> 
> I think we could simply remove the declarations of these functions from
> HashMapImpl.h. If they're needed, better include HashMapImplInlines.h, yes?

Looking at this again, I think these 2 functions are only used by clients in HeapMapImplInlines.h.  So, yes, I think we can simply remove these declarations from HashMapImpl.h.
Comment 8 Yusuke Suzuki 2021-04-17 18:19:53 PDT
Committed r276217 (236699@main): <https://commits.webkit.org/236699@main>