1026535 - Make the C4099 warning be treated as an error on MSVC

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(no longer active)]

http://msdn.microsoft.com/en-us/library/695x5bes.aspx

MSVC uses different manglings for classes versus structs, which means if you forward declare something as a struct which is declared elsewhere as a class or vice versa you'll get a linker error.

We should really start treating warning C4099 as an error.  This is a huge footgun.

Comment 1

[(no longer active)]

Birunthan, is this something that would interest you by any chance?

Comment 2

[Biru [:poiru]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #1)
> Birunthan, is this something that would interest you by any chance?

Sure, I can take a look.

Comment 3

[Biru [:poiru]]

Attachment: Fix mismatched class/struct tags

Try push: https://tbpl.mozilla.org/?tree=Try&rev=0c25fd2618eb

I'll leave actually making C4099 an error for a subsequent patch due to build bustage on try.

By the way, should we revert bug 780474?

Comment 4

[(no longer active)]

(In reply to Birunthan Mohanathas [:poiru] from comment #3)
> Created attachment 8442506 [details] [diff] [review]
> Fix mismatched class/struct tags
> 
> Try push: https://tbpl.mozilla.org/?tree=Try&rev=0c25fd2618eb
> 
> I'll leave actually making C4099 an error for a subsequent patch due to
> build bustage on try.

Sounds good.

> By the way, should we revert bug 780474?

OMG, yes.  /me goes to hang himself...

Comment 5

[(no longer active)]

FWIW I think we should consider making this warning an error on all of our compilers.

Comment 6

[Biru [:poiru]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf15eb82338

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fcf15eb82338

Comment 8

[Biru [:poiru]]

Attachment: Disable C4099 warning in 3rd party code

Comment 9

[Biru [:poiru]]

Attachment: Re-enable -Wmismatched-tags/C4099 warnings

Try push: https://tbpl.mozilla.org/?tree=Try&rev=bbd190976cd1

With VC10, making C4099 an error first and then disabling it still causes an error. For example, this fails on VC10:

  // cl file.cpp -Wall -we4099 -wd4099
  class Test;
  struct Test {};
  int main() {}

On VC11/VC12, this compiles fine. So, if we want to globally[0] use -we4099, we will first need to fix 3rd party code (namely, libstagefright and protobuf). Or we could just wait until we switch to VC12. Which would you prefer?

[0]: http://dxr.mozilla.org/mozilla-central/source/configure.in#2145

Comment 10

[(no longer active)]

Comment on attachment 8442862 [details] [diff] [review]
Disable C4099 warning in 3rd party code

Review of attachment 8442862 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm, this is a bit sad, but I can live with it.  But if you think there is a chance of getting this stuff fixed in upstream, please file bugs on them?

Comment 11

[(no longer active)]

Comment on attachment 8442866 [details] [diff] [review]
Re-enable -Wmismatched-tags/C4099 warnings

Review of attachment 8442866 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot for driving this through!

Comment 12

[Biru [:poiru]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #11)
> Thanks a lot for driving this through!

Sure thing.

Comment 13

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93be174c5823
https://hg.mozilla.org/integration/mozilla-inbound/rev/896467b67607

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/93be174c5823
https://hg.mozilla.org/mozilla-central/rev/896467b67607

Comment 15

[Daniel Holbert [:dholbert]]

Apologies; I mis-pasted this bug's bug number into my commit message for an unrelated patch. I backed it out & re-landed with correct bug number, but I'm noting it here in case there ends up being a sheriff-merge-tool auto-post here when the faulty commit is merged. (or in case someone ends up on this bug when doing hg archeology from my mistaken commit)

For the record, the cset with mis-pasted bug number was:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/577d7660ea9a
which I backed out in:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0351a28ceb9

(and then re-landed with fixed bug number as https://hg.mozilla.org/integration/mozilla-inbound/rev/dbfa7096402f )