Apologies for the size of the patch, it was very difficult to break down the MIR.h changes. The changes are mostly mechanical so they will hopefully be easy to review. I would appreciate a quick review if possible since this patch is very bitrot prone. Thanks!

Comment on attachment 8545960 [details] [diff] [review] Mark virtual overridden functions as MOZ_OVERRIDE in JS Review of attachment 8545960 [details] [diff] [review]: ----------------------------------------------------------------- r=me on everything but MIR.h, to start. So if I'm skimming right, the *only* reason for the INSTRUCTION_HEADER extra argument nonsense is MPhi. Is that the only black sheep here? If so, I sort of hate to make the complaint, but could you rename INSTRUCTION_HEADER to INSTRUCTION_HEADER_WITHOUT_TYPEPOLICY, remove the two typePolicy methods from it, then have a INSTRUCTION_HEADER(opcode) macro that uses the other, then tacks on the two typePolicy methods with MOZ_OVERRIDE for both? This shouldn't be too crazy a s&r to revert all the INSTRUCTION_HEADER changes, then modify *just* the MPhi one to use the without-typepolicy macro, I hope. (And afterward, it's far clearer who uses each macro, versus with this patch, where it's nigh-impossible to tell who uses the variadic oddity.) Assuming MPhi is the only weird case, r=me to make the change outlined above and land it with the rest of the MIR.h changes. If I missed some other place where single-argument INSTRUCTION_HEADER is necessary, tell me tomo^H^Hday and we can figure out something else. (By the way: it's illegal C++ to pass no arguments where a ... was expected. You have to provide at least one argument, or else it *should* be a compile error, and last I knew was one [or at least a warning] on some platforms. Another reason the current tactic can't quite fly.)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2) > Comment on attachment 8545960 [details] [diff] [review] > Mark virtual overridden functions as MOZ_OVERRIDE in JS > > Review of attachment 8545960 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me on everything but MIR.h, to start. > > So if I'm skimming right, the *only* reason for the INSTRUCTION_HEADER extra > argument nonsense is MPhi. Is that the only black sheep here? Yes! > If so, I sort of hate to make the complaint, but could you rename > INSTRUCTION_HEADER to INSTRUCTION_HEADER_WITHOUT_TYPEPOLICY, remove the two > typePolicy methods from it, then have a INSTRUCTION_HEADER(opcode) macro > that uses the other, then tacks on the two typePolicy methods with > MOZ_OVERRIDE for both? This shouldn't be too crazy a s&r to revert all the > INSTRUCTION_HEADER changes, then modify *just* the MPhi one to use the > without-typepolicy macro, I hope. (And afterward, it's far clearer who uses > each macro, versus with this patch, where it's nigh-impossible to tell who > uses the variadic oddity.) > > Assuming MPhi is the only weird case, r=me to make the change outlined above > and land it with the rest of the MIR.h changes. If I missed some other > place where single-argument INSTRUCTION_HEADER is necessary, tell me > tomo^H^Hday and we can figure out something else. OK, I think I can just do what you're suggesting. > (By the way: it's illegal C++ to pass no arguments where a ... was expected. > You have to provide at least one argument, or else it *should* be a compile > error, and last I knew was one [or at least a warning] on some platforms. > Another reason the current tactic can't quite fly.) Is that true for macros? I'm asking because we're doing this elsewhere and it seems to work fine.

Backed out for bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/48fa02688a56 https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5261755&repo=mozilla-inbound

Win64-only it appears.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #3) > > (By the way: it's illegal C++ to pass no arguments where a ... was expected. > > You have to provide at least one argument, or else it *should* be a compile > > error, and last I knew was one [or at least a warning] on some platforms. > > Another reason the current tactic can't quite fly.) > > Is that true for macros? I'm asking because we're doing this elsewhere and > it seems to work fine. Yes, it is *specifically* true for macros. See C++11/14 [cpp.replace]p4 second sentence: """ If the identifier-list in the macro definition does not end with an ellipsis, the number of arguments (including those arguments consisting of no preprocessing tokens) in an invocation of a function-like macro shall equal the number of parameters in the macro definition. Otherwise, there shall be more arguments in the invocation than there are parameters in the macro definition (excluding the ...). [...] """ Passing no arguments where ... was supplied in a C/C++ function is obviously perfectly fine, else sprintf("foo") wouldn't work.