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
Created attachment 91912 [details] fix issue
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.
Hello, Any update ? Regards.
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!
(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.
Comment on attachment 91912 [details] fix issue sorry! - tried to keep everything working :-)
Hello The patch was reviewed more than one week ago but it's not yet added to commit queue? Rgards, Thouraya.
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?
Created attachment 94428 [details] fix issue
Comment on attachment 94428 [details] fix issue Why do you need this change? no other platforms require AssemblerBuffer to be included in this way.
(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.
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.
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.
Comment on attachment 91912 [details] fix issue Clearing flags on attachment: 91912 Committed r87076: <http://trac.webkit.org/changeset/87076>
All reviewed patches have been landed. Closing bug.
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.
(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)
This patch was already landed, so it should be fixed. I do think someone should fix it another way.
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.