Bug 79083 - Unnecessary preprocessor macro in MainThread.h/cpp
Summary: Unnecessary preprocessor macro in MainThread.h/cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Roland Takacs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-21 00:04 PST by Roland Takacs
Modified: 2012-02-21 10:28 PST (History)
4 users (show)

See Also:


Attachments
patch (1.71 KB, patch)
2012-02-21 00:08 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
patch (2.62 KB, patch)
2012-02-21 01:54 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
patch (2.62 KB, patch)
2012-02-21 05:47 PST, Roland Takacs
no flags Details | Formatted Diff | Diff
patch (1.69 KB, patch)
2012-02-21 10:14 PST, Roland Takacs
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Takacs 2012-02-21 00:04:36 PST
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.
Comment 1 Roland Takacs 2012-02-21 00:08:33 PST
Created attachment 127928 [details]
patch
Comment 2 Patrick R. Gansterer 2012-02-21 00:28:17 PST
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... :-)
Comment 3 Roland Takacs 2012-02-21 01:54:18 PST
Created attachment 127942 [details]
patch
Comment 4 Roland Takacs 2012-02-21 02:19:01 PST
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... :-)
Comment 5 Patrick R. Gansterer 2012-02-21 03:50:54 PST
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?!?
Comment 6 Roland Takacs 2012-02-21 05:47:25 PST
Created attachment 127958 [details]
patch

Sorry, You are right, I meant the other direction.
Comment 7 Patrick R. Gansterer 2012-02-21 07:01:58 PST
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.
Comment 8 Roland Takacs 2012-02-21 10:11:05 PST
(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.
Comment 9 Roland Takacs 2012-02-21 10:14:42 PST
Created attachment 127992 [details]
patch
Comment 10 Patrick R. Gansterer 2012-02-21 10:24:20 PST
Committed r108369: <http://trac.webkit.org/changeset/108369>
Comment 11 Patrick R. Gansterer 2012-02-21 10:28:09 PST
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. :-)