Bug 171554 – JSC C API should expose GC marking constraints and weak references

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

[Filip Pizlo]

This lets you *almost* integrate JSC's GC with a foreign GC. It lets you implement lots of exotic reference types, way beyond weak references.

[Filip Pizlo]

Created attachment 308829 [details]
the patch

[Build Bot]

Attachment 308829 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/API/tests/testapi.c:1167:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.cpp:79:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/API/JSWeakPrivate.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
Total errors found: 4 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 308829 [details]
the patch

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

r=me

> Source/JavaScriptCore/API/JSMarkingConstraintPrivate.h:47
> +JS_EXPORT void JSContextGroupRegisterMarkingConstraint(JSContextGroupRef, JSMarkingConstraint, void *userData);

Usually we would do this as a pair of add/remove APIs: JSContextGroupAddMarkingConstraint, JSContextGroupRemoveMarkingConstraint.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/216078/webkit

[Ryan Haddad]

(In reply to Filip Pizlo from comment #4)
> Landed in https://trac.webkit.org/changeset/216078/webkit

This change broke the Windows build:

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/1133

[Filip Pizlo]

(In reply to Ryan Haddad from comment #5)
> (In reply to Filip Pizlo from comment #4)
> > Landed in https://trac.webkit.org/changeset/216078/webkit
> 
> This change broke the Windows build:
> 
> https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/
> 1133

Hopefully fixed in r216085

[Filip Pizlo]

And fixed for real in https://trac.webkit.org/changeset/216086/webkit

[Don Olmstead]

There's still a problem with the Windows builds. The declaration of JSSynchronousGarbageCollectForDebugging in testapi.c should have an extern "C" in front of it.

[Don Olmstead]

Created attachment 308855 [details]
Windows build fix

[Filip Pizlo]

(In reply to Don Olmstead from comment #9)
> Created attachment 308855 [details]
> Windows build fix

That's weird that you need "extern "C"" in a C file.  It may be that your fix does not work on Mac, in which case the right fix is to #ifdef __cplusplus...

[Don Olmstead]

Created attachment 308862 [details]
Windows build fix

Add __cplusplus guard

[Filip Pizlo]

(In reply to Don Olmstead from comment #11)
> Created attachment 308862 [details]
> Windows build fix
> 
> Add __cplusplus guard

Thank you for doing this!

[Don Olmstead]

Reopening so hopefully the patch will land

[Don Olmstead]

(In reply to Filip Pizlo from comment #12)
> (In reply to Don Olmstead from comment #11)
> > Created attachment 308862 [details]
> > Windows build fix
> > 
> > Add __cplusplus guard
> 
> Thank you for doing this!

No problem!

[WebKit Commit Bot]

Comment on attachment 308862 [details]
Windows build fix

Clearing flags on attachment: 308862

Committed r216109: <http://trac.webkit.org/changeset/216109>