Bug 60380 – Fix compile with GCC 4.6.0

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Dawit A.]

Attempting to compile webkit trunk on my box (ArchLinux x86-64) stops the following error:

Source/JavaScriptCore/runtime/StringPrototype.cpp: In function ‘void* JSC::stringProtoFuncMatch(JSC::ExecState*)’:
Source/JavaScriptCore/runtime/StringPrototype.cpp:631:14: error: variable ‘lastIndex’ set but not used [-Werror=unused-but-set-variable]

The attached patch comments of the offending set but unused variable to prevent this compile breakage.

[Dawit A.]

Created attachment 92599 [details]
proposed patch v1

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/runtime/StringPrototype.cpp:631:  Should have a space between // and comment  [whitespace/comments] [4]
Source/JavaScriptCore/runtime/StringPrototype.cpp:634:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 2 files


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

[Dawit A.]

Created attachment 92600 [details]
proposed patch v2

Fixed the style problems...

[Andreas Kling]

Comment on attachment 92600 [details]
proposed patch v2

If the variable is unused, it should be removed, not commented out.

[Dawit A.]

Created attachment 92614 [details]
proposed patch v3

Removed the set but unused variables instead of commenting them out.

[Alexey Proskuryakov]

Comment on attachment 92614 [details]
proposed patch v3

I saw (at least parts of) this is someone else's patch today, r- since this will conflict with it.

[Alexey Proskuryakov]

Specifically, bug 60370.

[Dawit A.]

(In reply to comment #7)
> Specifically, bug 60370.

Greate, but the patch in that report does not fix the JavascriptCore which was  originally proposed in patch v2.

[Dawit A.]

Created attachment 92663 [details]
proposed patch v4

Removed fixes that have already been applied through bug# 60370.

[Daniel Bates]

Comment on attachment 92663 [details]
proposed patch v4

OK.

[Daniel Bates]

Comment on attachment 92663 [details]
proposed patch v4

r- you're missing a change log.

[Dawit A.]

Created attachment 92698 [details]
proposed patch v5

Added missing changelog information.

[Daniel Bates]

Comment on attachment 92698 [details]
proposed patch v5

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

As far as I can tell you're not a committer. So, I will make these changes for you and land your patch.

> Source/JavaScriptCore/ChangeLog:8
> +        Remove unused local variable from code
> +
> +        Fix compile with gcc 4.6.0
> +        https://bugs.webkit.org/show_bug.cgi?id=60380

This change log entry does not have the right format. The format is bug title, bug url, then description. Also, GCC is an acronym so "gcc" should be "GCC" (although I can see from the diff that we landed change log entries with "gcc").

Also, we use complete sentences.

[Daniel Bates]

Committed r86008: <http://trac.webkit.org/changeset/86008>

[Dawit A.]

(In reply to comment #13)
> (From update of attachment 92698 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92698&action=review
> 
> As far as I can tell you're not a committer. So, I will make these changes for you and land your patch.
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        Remove unused local variable from code
> > +
> > +        Fix compile with gcc 4.6.0
> > +        https://bugs.webkit.org/show_bug.cgi?id=60380
> 
> This change log entry does not have the right format. The format is bug title, bug url, then description. Also, GCC is an acronym so "gcc" should be "GCC" (although I can see from the diff that we landed change log entries with "gcc").

For the record, the changelog message was prepared using 

./Tools/Scripts/prepare-ChangeLog -b 60380 -g <git-commit-number>

So the format is as generated by the script one is supposed to use to generate the changelog...

[Ademar (unprivileged account)]

Revision r86008 cherry-picked into qtwebkit-2.2 with commit 8620c0c <http://gitorious.org/webkit/qtwebkit/commit/8620c0c>

[Dawit A.]

(In reply to comment #16)
> Revision r86008 cherry-picked into qtwebkit-2.2 with commit 8620c0c <http://gitorious.org/webkit/qtwebkit/commit/8620c0c>

Need to cherry pick the patch in bug# 60370 as well. See comment #8.
http://trac.webkit.org/changeset/85973

[Ademar Reis]

(In reply to comment #17)
> (In reply to comment #16)
> > Revision r86008 cherry-picked into qtwebkit-2.2 with commit 8620c0c <http://gitorious.org/webkit/qtwebkit/commit/8620c0c>
> 
> Need to cherry pick the patch in bug# 60370 as well. See comment #8.
> http://trac.webkit.org/changeset/85973

Done, thanks!

[Ademar Reis]

Dawit: bugs should block the master bug until they're cherry-picked. Once they're included in the branch they're removed from the blocking list.

[Dawit A.]

(In reply to comment #19)
> Dawit: bugs should block the master bug until they're cherry-picked. Once they're included in the branch they're removed from the blocking list.

Ahhh.... right. The reason I made this bug block the master bug is because not all the patches from this bug, actually the one from bug 60370, were cherry-picked. Since I did not create the 60370 bug and I could not change it to block master, I simply added a comment on this one and made it the blocker instead.