Bug 59927 – [SH4] AssemblerLabel does not name a type

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

[thouraya]

Hello,

Updating webkit, I have got the following error : error: AssemblerLabel does not name a type.

The issue is caused by the work done to Use AssemblerLabel throughout Assembler classes.

To fix the problem, I have to change the define order inf MacroAssemblerSH4.h as follow

#include "SH4Assembler.h"
#include "AbstractMacroAssembler.h"

But I'll get a webkit style problem :Alphabetical sorting problem.  [build/include_order] [4]


I have another solution which is to add:

# inculde AbstractMacroAssembler.h in AbstractMacroAssembler.h

What do you suggest as a solution? I'm preparing  a patch.

I think there is the same issue in MIPS platforms.

Regards,
Thouraya

[thouraya]

Created attachment 91912 [details]
fix issue

[WebKit Review Bot]

Attachment 91912 [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/assembler/MacroAssemblerSH4.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


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

[thouraya]

Hello,

Any update ?

Regards.

[Eric Seidel (no email)]

There is a similar fix to this posted for MIPS somewher ein bugzilla.  Why do these headers have an ordering dependency?  That seems like the real bug!

[thouraya]

(In reply to comment #4)
> There is a similar fix to this posted for MIPS somewher ein bugzilla.  Why do these headers have an ordering dependency?  That seems like the real bug!


Hello,

because AbstractMacroAssembler class needs AssemblerLabel struct which is defined in AssemblerBuffer.h file.

Or AssemblerBuffer.h is included in "SH4Assembler.h" and not in AbstractMacroAssembler.h.


Regards,
Thouraya.

[Gavin Barraclough]

Comment on attachment 91912 [details]
fix issue

sorry! - tried to keep everything working :-)

[thouraya]

Hello

The patch was reviewed more than one week ago but it's not yet added to commit queue?


Rgards,
Thouraya.

[Eric Seidel (no email)]

Comment on attachment 91912 [details]
fix issue

You may want to set cq? To convey your desire.  Why do we need to missort the headers like this? Can't We include the needed header in the other header?

[thouraya]

Created attachment 94428 [details]
fix issue

[Oliver Hunt]

Comment on attachment 94428 [details]
fix issue

Why do you need this change?  no other platforms require AssemblerBuffer to be included in this way.

[Oliver Hunt]

(In reply to comment #10)
> (From update of attachment 94428 [details])
> Why do you need this change?  no other platforms require AssemblerBuffer to be included in this way.

Oh i see, eric is making a bad suggestion for the world of jsc.

[thouraya]

Hello,


(In reply to comment #10)
> (From update of attachment 94428 [details])
> Why do you need this change?  no other platforms require AssemblerBuffer to be included in this way.

MIPS requires changes: they suggested the same solution which is to swap the 2 include files and it was accepted in Bug 60283



Regards,
Thouraya.

[Oliver Hunt]

Comment on attachment 91912 [details]
fix issue

Just set cq? on patches f you are unable to commit yourself.   Otherwise reviewers have no indication as to whether they should be using the commit bot or not.

[WebKit Commit Bot]

Comment on attachment 91912 [details]
fix issue

Clearing flags on attachment: 91912

Committed r87076: <http://trac.webkit.org/changeset/87076>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

This is not good. In our project we strive to not create includes that have to be included in a certain order. Each header file is supposed to include everything it depends on. Check-ins like this one go against that basic WebKit design principle.

[Ademar Reis]

(In reply to comment #16)
> This is not good. In our project we strive to not create includes that have to be included in a certain order. Each header file is supposed to include everything it depends on. Check-ins like this one go against that basic WebKit design principle.

Any plans on fixing it any other way? (I don't have access to a SH4 environment to test)

[Darin Adler]

This patch was already landed, so it should be fixed.

I do think someone should fix it another way.

[Eric Seidel (no email)]

There were a flurry of these patches for all the various ports.  This was a recent regression caused by some other Assembler refactor.  I never tracked down the change, but I would suspect that Gavin or Oliver or Geoff would know what change it might be.