Closed Bug 827521 Opened 7 years ago Closed 7 years ago

SVGPreserveAspectRatio.h:60:16: error: use of overloaded operator '<' is ambiguous (with operand types 'uint16_t' (aka 'unsigned short') and 'mozilla::SVGAlign') and more, w/ Clang 3.0

Categories

(Core :: SVG, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: graememcc, Assigned: graememcc)

References

Details

(Whiteboard: [workaround: use GCC or build a newer clang, w/ steps at http://mzl.la/VGKIwd ])

Attachments

(1 file, 1 obsolete file)

Attached patch fix (obsolete) — Splinter Review
Stock clang 3.0 on Ubuntu fails on a trunk clobber build. Bisect blames 0b4ce0db7ecb from bug 826961.

/home/graememcc/moz/trunk/debug_work/src/image/decoders/icon/gtk/nsIconChannel.cpp
In file included from /home/graememcc/moz/trunk/debug_work/src/image/src/SVGDocumentWrapper.cpp:30:
In file included from /home/graememcc/moz/trunk/debug_work/src/content/svg/content/src/nsSVGSVGElement.h:19:
/home/graememcc/moz/trunk/debug_work/src/content/svg/content/src/SVGPreserveAspectRatio.h:60:16: error: use of overloaded operator '<' is ambiguous (with operand types 'uint16_t' (aka 'unsigned short') and 'mozilla::SVGAlign')
    if (aAlign < SVG_PRESERVEASPECTRATIO_NONE ||
        ~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/graememcc/moz/trunk/debug_work/src/content/svg/content/src/SVGPreserveAspectRatio.h:60:16: note: built-in candidate operator<(int, float)
...(followed by many other candidates)

This is followed by another 7 or so similar errors in that file. SVGAnimatedPreserveAspectRatio.cpp also throws up a pile of similar errors.

However, my manually compiled Clang 3.2 has no complaints about this code. Thus I won't be offended if the attached patch gets r- and the bug WONTFIXed, given (a) we don't seem to have any official requirements regarding Clang support on Linux and (b) the patch uglifies the code, sprinkling casts all over the place.
Attachment #698870 - Flags: review?(longsonr)
Summary: /SVGPreserveAspectRatio.h:60:16: error: use of overloaded operator '<' is ambiguous (with operand types 'uint16_t' (aka 'unsigned short') and 'mozilla::SVGAlign') and more → SVGPreserveAspectRatio.h:60:16: error: use of overloaded operator '<' is ambiguous (with operand types 'uint16_t' (aka 'unsigned short') and 'mozilla::SVGAlign') and more
If clang is moving away from warning about this, I think it'd be better to leave the code as-is (implicitly casting where necessary) rather than adding these explicit casts.

(Then people using "old" clang can just build without the --enable-warnings-as-errors mozconfig option and they should be fine.)
Summary: SVGPreserveAspectRatio.h:60:16: error: use of overloaded operator '<' is ambiguous (with operand types 'uint16_t' (aka 'unsigned short') and 'mozilla::SVGAlign') and more → SVGPreserveAspectRatio.h:60:16: error: use of overloaded operator '<' is ambiguous (with operand types 'uint16_t' (aka 'unsigned short') and 'mozilla::SVGAlign') and more, in w/ --enable-warnings-as-errors build w/ Clang 3.0
Summary: SVGPreserveAspectRatio.h:60:16: error: use of overloaded operator '<' is ambiguous (with operand types 'uint16_t' (aka 'unsigned short') and 'mozilla::SVGAlign') and more, in w/ --enable-warnings-as-errors build w/ Clang 3.0 → SVGPreserveAspectRatio.h:60:16: error: use of overloaded operator '<' is ambiguous (with operand types 'uint16_t' (aka 'unsigned short') and 'mozilla::SVGAlign') and more, in --enable-warnings-as-errors build w/ Clang 3.0
Comment on attachment 698870 [details] [diff] [review]
fix


This is horrible. There must be a better way than this.
Attachment #698870 - Flags: review?(longsonr) → review-
Would an explicit conversion operator on the enums work?
> SVGPreserveAspectRatio.h:60:16: error: use of overloaded operator '<' is
> ambiguous (with operand types 'uint16_t' (aka 'unsigned short') and
> 'mozilla::SVGAlign')
[...]
> SVGPreserveAspectRatio.h:60:16: note: built-in candidate operator<(int, float)

Wait -- why would it be trying to convert SVGAlign to a _float_ for the conversion, of all things?  It's backed by a uint8_t, and we're comparing it to a uint16_t.

I wonder if this is a compiler bug (which would explain why Clang 3.2 doesn't warn about it -- maybe it's fixed there).
Because clang is listing all possible "candidates"?
> ...(followed by many other candidates)
Anyway, I'm in favor of WONTFIX'ing. The purpose of converting to enum was reducing casts, not increasing.
Just got another report of this on IRC.

I wonder if we should disable this warning (or add an explicit command-line flag to warn about it instead of honoring it for FAIL_ON_WARNINGS)?
(In reply to Daniel Holbert [:dholbert] from comment #6)
> I wonder if we should disable this warning (or add an explicit command-line
> flag to warn about it instead of honoring it for FAIL_ON_WARNINGS)?

Just kidding -- this is a full-on compile error, not a warning that we're treating as an error.  So I don't think we can disable it with a flag.
(In reply to Daniel Holbert [:dholbert] from comment #7)
> Just kidding -- this is a full-on compile error, not a warning that we're
> treating as an error.  So I don't think we can disable it with a flag.

??? The bug summary says "in --enable-warnings-as-errors build".
(In reply to Masatoshi Kimura [:emk] from comment #8)
> Maybe this clang bug?
> http://llvm.org/bugs/show_bug.cgi?id=9993

Looks like it, yeah.

(In reply to Masatoshi Kimura [:emk] from comment #9)
> ??? The bug summary says "in --enable-warnings-as-errors build".

That wasn't originally there -- I (mistakenly) added that in comment 1.  Thanks for reminding me -- removed it now.
Summary: SVGPreserveAspectRatio.h:60:16: error: use of overloaded operator '<' is ambiguous (with operand types 'uint16_t' (aka 'unsigned short') and 'mozilla::SVGAlign') and more, in --enable-warnings-as-errors build w/ Clang 3.0 → SVGPreserveAspectRatio.h:60:16: error: use of overloaded operator '<' is ambiguous (with operand types 'uint16_t' (aka 'unsigned short') and 'mozilla::SVGAlign') and more, w/ Clang 3.0
How widespread is this Daniel? We could just back out bug 826961.
(In reply to Robert Longson from comment #11)
> How widespread is this Daniel?

I've only heard 2 reports so far. (comment 0 & comment 6)

> We could just back out bug 826961.

If we get a ton more reports, that might be prudent... but otherwise I'd opt for leaving bug 826961 in. I don't like the idea of backing out useful cleanup work because of a compiler bug in a non-default compiler, and w/ the bug being fixed in newer versions.
Whiteboard: [workaround: use GCC or build a newer clang, w/ steps at http://mzl.la/VGKIwd ]
Attached patch fix?Splinter Review
> Would an explicit conversion operator on the enums work?
AIUI conversion operators can only be defined for classes/structs.

This slightly less awful patch isolates the manual casts to the header file, and uses those values for the bounds checking etc. The cast in SVGAnimatedPreserveAspectRatio::GetBaseValueString remains to preserve the meaning of the comparison there.
Attachment #698870 - Attachment is obsolete: true
Comment on attachment 700008 [details] [diff] [review]
fix?

Much better.
Attachment #700008 - Flags: review+
Comment on attachment 700008 [details] [diff] [review]
fix?

Drive-by suggestion:

>+const uint16_t SVG_ALIGN_MIN = SVG_PRESERVEASPECTRATIO_NONE;
[...]
>+const uint16_t SVG_MEETORSLICE_MIN = SVG_MEETORSLICE_MEET;

In both cases, this is a somewhat confusing value for a "MIN" value, because these enum types actually have an _UNKNOWN enum value that is lower than this purported "min".

MXR reference:
17 enum SVGAlign MOZ_ENUM_TYPE(uint8_t) {
18   SVG_PRESERVEASPECTRATIO_UNKNOWN = 0,
19   SVG_PRESERVEASPECTRATIO_NONE = 1,
...and...
32 enum SVGMeetOrSlice MOZ_ENUM_TYPE(uint8_t) {
33   SVG_MEETORSLICE_UNKNOWN = 0,
34   SVG_MEETORSLICE_MEET = 1,
https://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGPreserveAspectRatio.h?mark=18-19,33-34#16

Maybe add a comment above these declarations, saying something like:
// Min & max enum values for valid <align> parameter
// (excluding "_UNKNOWN" because it's a sentinel rather than an actual value)

(...and the same with s/align/meetOrSlice/)
and/or perhaps change them from MIN to MIN_VALID and MAX to MAX_VALID
http://hg.mozilla.org/integration/mozilla-inbound/rev/1ceb45fd14c5
Assignee: nobody → graememcc_firefox
Status: NEW → ASSIGNED
Flags: in-testsuite-
Duplicate of this bug: 829272
https://hg.mozilla.org/mozilla-central/rev/1ceb45fd14c5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.