Last Comment Bug 873486 - MOZ_OVERRIDE in method definition breaks compilation for C++11
: MOZ_OVERRIDE in method definition breaks compilation for C++11
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla24
Assigned To: Juan Gomez [:_AtilA_] (CET/CEST)
:
:
Mentors:
Depends on:
Blocks: 862324
  Show dependency treegraph
 
Reported: 2013-05-17 08:12 PDT by Juan Gomez [:_AtilA_] (CET/CEST)
Modified: 2013-05-21 10:50 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Removes MOZ_OVERRIDE from method definition. (555 bytes, patch)
2013-05-17 08:12 PDT, Juan Gomez [:_AtilA_] (CET/CEST)
matt.woodrow: review+
Details | Diff | Splinter Review
Removes vir-specifier MOZ_OVERRIDE in method definition (953 bytes, patch)
2013-05-20 07:15 PDT, Juan Gomez [:_AtilA_] (CET/CEST)
no flags Details | Diff | Splinter Review
patch for checkin (same as r+'d patch, now with patch headers) (915 bytes, patch)
2013-05-20 07:37 PDT, Juan Gomez [:_AtilA_] (CET/CEST)
no flags Details | Diff | Splinter Review

Description Juan Gomez [:_AtilA_] (CET/CEST) 2013-05-17 08:12:11 PDT
Created attachment 751042 [details] [diff] [review]
Removes MOZ_OVERRIDE from method definition.

C++11 specs says that the virt-specifiers ("override" and "final") shall only appear in the declaration of a virtual member function. But TextureHostOGL.h has a MOZ_OVERRIDE (so override) after SetBuffer(..) declaracion, causing gcc-4.7 with C++11 to break compliation.
Comment 1 Daniel Holbert [:dholbert] 2013-05-20 01:43:57 PDT
(In reply to Juan Gomez [:_AtilA_] from comment #0)
> C++11 specs says that the virt-specifiers ("override" and "final") shall
> only appear in the declaration of a virtual member function. But
> TextureHostOGL.h has a MOZ_OVERRIDE (so override) after SetBuffer(..)
> declaracion, causing gcc-4.7 with C++11 to break compliation.

To be clear, this is TextureHostOGL.cpp (not .h), and the faulty MOZ_OVERRIDE in question appears after the method *definition* (not the declaration), and that's the problem.

(correct me if I'm wrong)
Comment 2 Daniel Holbert [:dholbert] 2013-05-20 02:01:32 PDT
Juan: could you attach an updated version of the patch with a commit message (including r=mattwoodrow) and with the "user" patch-header included so that the patch is correctly attributed to you?

See this page for how to do that, if you're not sure how:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

After that, you should be able to mark this bug as "checkin-needed", and then someone can come along and trivially import & land the patch on your behalf.
Comment 3 Juan Gomez [:_AtilA_] (CET/CEST) 2013-05-20 07:15:41 PDT
Created attachment 751681 [details] [diff] [review]
Removes vir-specifier MOZ_OVERRIDE in method definition
Comment 4 Juan Gomez [:_AtilA_] (CET/CEST) 2013-05-20 07:16:44 PDT
(In reply to Daniel Holbert [:dholbert] from comment #1)
> (In reply to Juan Gomez [:_AtilA_] from comment #0)
> > C++11 specs says that the virt-specifiers ("override" and "final") shall
> > only appear in the declaration of a virtual member function. But
> > TextureHostOGL.h has a MOZ_OVERRIDE (so override) after SetBuffer(..)
> > declaracion, causing gcc-4.7 with C++11 to break compliation.
> 
> To be clear, this is TextureHostOGL.cpp (not .h), and the faulty
> MOZ_OVERRIDE in question appears after the method *definition* (not the
> declaration), and that's the problem.
> 
> (correct me if I'm wrong)

Oh! I confused things a little, but you are right!.
I submitted a new patch using git forma-patch, it has author information and a verbose commit message. I read this format is importable by Mercurial, if not, I'll try hg.
Comment 5 Daniel Holbert [:dholbert] 2013-05-20 07:33:45 PDT
Comment on attachment 751042 [details] [diff] [review]
Removes MOZ_OVERRIDE from method definition.

[un-obsoleting the original r+'d patch]

In cases like this one where you've got a r+'d patch and you're attaching an updated version for someone else to checkin, it's best *not* to obsolete the r+'d patch, to make it more immediately-clear that the patch has indeed been reviewed.

(And then perhaps add "(for checkin)" to the description field for the actual ready-to-land patch, to make it clear which version is intended for checkin)
Comment 6 Juan Gomez [:_AtilA_] (CET/CEST) 2013-05-20 07:37:31 PDT
Created attachment 751689 [details] [diff] [review]
patch for checkin (same as r+'d patch, now with patch headers)

Well formatted commit message.
Comment 7 Ryan VanderMeulen [:RyanVM] 2013-05-20 08:50:49 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc64829a0cba
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-05-21 10:50:16 PDT
https://hg.mozilla.org/mozilla-central/rev/dc64829a0cba

Note You need to log in before you can comment on or make changes to this bug.