Closed Bug 869238 Opened 7 years ago Closed 7 years ago

TestCasting.cpp -Wtype-limit warnings

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: Waldo, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

tbsaunde reported various -Wtype-limit compiler warnings in TestCasting.cpp when compiling with gcc 4.7.  The warnings track back to various constructs in Casting.h that are sometimes vacuous, depending on how things are instantiated.  Add more layers of template instantiation, or more-correct size checking, to fix this.
Attached patch PatchSplinter Review
Attachment #746136 - Flags: review?(nfroyd)
Blocks: buildwarning
Status: NEW → ASSIGNED
Version: unspecified → Trunk
Attachment #746136 - Flags: review?(nfroyd) → review+
Comment on attachment 746136 [details] [diff] [review]
Patch

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

::: mfbt/Casting.h
@@ +41,5 @@
> +
> +template<typename From, typename To>
> +struct UnsignedUnsignedCheck<From, To, FromIsBigger>
> +{
> +  public:

Didn't catch this before, but I think we've said in the past that there's no need for |public:| in structs, as that's already the default.  Fixing up the instances in this patch would be good; fixing the other instances in Casting.h can come along for the ride, if you like.  Might merit a note in STYLE, too.
https://hg.mozilla.org/integration/mozilla-inbound/rev/883e35db4839

public-for-structs was, I think, for |struct S : T| where T would be public-inherited.  Nobody's said anything yet for inside struct or class.  :-)  I know some people like EIBTI and so want public in struct, private in class, even when it's the default.  We could make a rule, but it seems best to do that separately from this, given that we're not consistent either way.
Oh, the issue -- for future reference -- was that MSVC doesn't like sizeof(T) < sizeof(U) in template parameters -- it wants parentheses around it.  No idea what the spec says, seems easy to add 'em, so I just did that.
https://hg.mozilla.org/mozilla-central/rev/883e35db4839
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.