Bug 79083 – Unnecessary preprocessor macro in MainThread.h/cpp

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

[Roland Takacs]

Patrick R. Gansterer mentioned at https://bugs.webkit.org/show_bug.cgi?id=73309 that I wrote "#if PLATFORM(WINDOWS)" insted of "#if PLATFORM(WIN)".

I fixed this bug.

[Roland Takacs]

Created attachment 127928 [details]
patch

[Patrick R. Gansterer]

Comment on attachment 127928 [details]
patch

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

> Source/JavaScriptCore/wtf/MainThread.cpp:284
> +#elif PLATFORM(MAC) || PLATFORM(WIN)

IMHO you need an additional line in the exports file too now, when you do not inline the code. But maybe you that, only let the EWS generate the mangeled name... :-)

[Roland Takacs]

Created attachment 127942 [details]
patch

[Roland Takacs]

Thanks to show me what cause the error ;)

(In reply to comment #2)
> (From update of attachment 127928 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127928&action=review
> 
> > Source/JavaScriptCore/wtf/MainThread.cpp:284
> > +#elif PLATFORM(MAC) || PLATFORM(WIN)
> 
> IMHO you need an additional line in the exports file too now, when you do not inline the code. But maybe you that, only let the EWS generate the mangeled name... :-)

[Patrick R. Gansterer]

Comment on attachment 127942 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        There is "PLATFORM(WINDOWS)" insted of "PLATFORM(WIN)".

not the other direction?!?

[Roland Takacs]

Created attachment 127958 [details]
patch

Sorry, You are right, I meant the other direction.

[Patrick R. Gansterer]

Comment on attachment 127958 [details]
patch

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

sorry for not asking earlier, but why can't we simple remove the PLATFORM(WINDOWS)? IMHO nobody needs it.

> Source/JavaScriptCore/ChangeLog:3
> +        [Qt] Parallel GC bugfix

how is _this_ patch related to Qt? which bug does it fix?

> Source/JavaScriptCore/ChangeLog:10
> +        Futhermore the decoration name of isMainThreadOrGCThread
> +        was inserted to the JavaScriptCore.def.

Please explain why.

[Roland Takacs]

(In reply to comment #7)
> (From update of attachment 127958 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127958&action=review
> 
> sorry for not asking earlier, but why can't we simple remove the PLATFORM(WINDOWS)? IMHO nobody needs it.

You are right, it is really not necessary to check it.

> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        [Qt] Parallel GC bugfix
> 
> how is _this_ patch related to Qt? which bug does it fix?
>

The name was really wrong. I renamed it.
 
> > Source/JavaScriptCore/ChangeLog:10
> > +        Futhermore the decoration name of isMainThreadOrGCThread
> > +        was inserted to the JavaScriptCore.def.
> 
> Please explain why.

When I changed 'WINDOWS' to 'WIN' and I uploaded the patch, it failed. Secondly, I added the isMainThreadOrGCThreads mangled name to the JavaScriptCore.def, and it works.

I will upload the patch soon without "PLATFORM(WINDOWS)" preprocessor macros.

[Roland Takacs]

Created attachment 127992 [details]
patch

[Patrick R. Gansterer]

Committed r108369: <http://trac.webkit.org/changeset/108369>

[Patrick R. Gansterer]

Comment on attachment 127992 [details]
patch

THX for the patch.

(In reply to comment #8)
> > > Source/JavaScriptCore/ChangeLog:10
> > > +        Futhermore the decoration name of isMainThreadOrGCThread
> > > +        was inserted to the JavaScriptCore.def.
> > 
> > Please explain why.
> 
> When I changed 'WINDOWS' to 'WIN' and I uploaded the patch, it failed. Secondly, I added the isMainThreadOrGCThreads mangled name to the JavaScriptCore.def, and it works.

The ChangeLog should usually explain the what and why. When browsing the history you usually don't look into the bug, so any relevant information should go into ChangeLog. :-)