Closed Bug 1482002 Opened 2 years ago Closed Last year

Compilation error in SVGTest.cpp with clang-3.8

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 --- fixed

People

(Reporter: pbone, Assigned: pbone)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

I get the following compilation error with clang-3.8 on Linux.

 0:15.63 In file included from /home/paul/dev/moz/dont-poison/obj-x86_64-pc-linux-gnu/dom/svg/Unified_cpp_dom_svg6.cpp:128:
 0:15.64 /home/paul/dev/moz/dont-poison/dom/svg/SVGTests.cpp:72:43: error: default initialization of an object of const type 'const nsCaseInsensitiveStringComparator' without a user-provided default constructor
 0:15.64   const nsCaseInsensitiveStringComparator caseInsensitiveComparator;
 0:15.64                                           ^
 0:15.64                                                                    {}
 0:15.65 /home/paul/dev/moz/dont-poison/dom/svg/SVGTests.cpp:159:45: error: default initialization of an object of const type 'const nsCaseInsensitiveStringComparator' without a user-provided default constructor
 0:15.65     const nsCaseInsensitiveStringComparator caseInsensitiveComparator;
 0:15.65                                             ^
 0:15.66                                                                      {}
 0:15.67 2 errors generated.

I guess Bug 1480946 created this regression.
Attachment #8998715 - Flags: review?(longsonr)
Considering you need at least libclang 3.9 for bindgen, I think we should just drop support for clang < 3.9 completely.
Comment on attachment 8998715 [details] [diff] [review]
newbug - add nsCaseInsensitiveStringComparator default constructor


I'll ask bz to review the change, since it's in a different component. but I want to ni longsonr in case they want some input, it looks like their change in Bug 1480946 created the issue.

Thanks.
Flags: needinfo?(longsonr)
Attachment #8998715 - Flags: review?(longsonr) → review?(bzbarsky)
(In reply to Mike Hommey [:glandium] from comment #1)
> Considering you need at least libclang 3.9 for bindgen, I think we should
> just drop support for clang < 3.9 completely.

How does that decision get made?  Right now docs say 3.8 and that's what my system has.

https://developer.mozilla.org/en-US/docs/Mozilla/Using_CXX_in_Mozilla_code
(In reply to Paul Bone [:pbone] from comment #3)
> (In reply to Mike Hommey [:glandium] from comment #1)
> > Considering you need at least libclang 3.9 for bindgen, I think we should
> > just drop support for clang < 3.9 completely.
> 
> How does that decision get made?  Right now docs say 3.8 and that's what my
> system has.
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Using_CXX_in_Mozilla_code

You talk to a build peer sort of person, which glandium and myself qualify as. :)

I think we've wanted to drop clang < 3.9 (or even < 5 or so) for a while; I always thought that it was difficult to determine whether the XCode clang was actually supported under that scheme or not.
(In reply to Nathan Froyd [:froydnj] from comment #4)
> (In reply to Paul Bone [:pbone] from comment #3)
> > (In reply to Mike Hommey [:glandium] from comment #1)
> > > Considering you need at least libclang 3.9 for bindgen, I think we should
> > > just drop support for clang < 3.9 completely.
> > 
> > How does that decision get made?  Right now docs say 3.8 and that's what my
> > system has.
> > 
> > https://developer.mozilla.org/en-US/docs/Mozilla/Using_CXX_in_Mozilla_code
> 
> You talk to a build peer sort of person, which glandium and myself qualify
> as. :)
> 
> I think we've wanted to drop clang < 3.9 (or even < 5 or so) for a while; I
> always thought that it was difficult to determine whether the XCode clang
> was actually supported under that scheme or not.

Filed bug 1482196 for this, we've been seeing a fair amount of user reports of bustage on 3.8.
Comment on attachment 8998715 [details] [diff] [review]
newbug - add nsCaseInsensitiveStringComparator default constructor

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

::: intl/unicharutil/util/nsUnicharUtils.h
@@ +53,4 @@
>  class nsCaseInsensitiveStringComparator : public nsStringComparator
>  {
>  public:
> +  nsCaseInsensitiveStringComparator() {};

I think we want `= default`.
Comment on attachment 8998715 [details] [diff] [review]
newbug - add nsCaseInsensitiveStringComparator default constructor

What comment 6 says, please.  r=me with that.
Attachment #8998715 - Flags: review?(bzbarsky) → review+
(In reply to Eric Rahm [:erahm] from comment #5)
> 
> Filed bug 1482196 for this, we've been seeing a fair amount of user reports
> of bustage on 3.8.

Thanks, I'll follow that and update also.
Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d58330c8fbc
add nsCaseInsensitiveStringComparator default constructor r=bz
Flags: needinfo?(longsonr)
https://hg.mozilla.org/mozilla-central/rev/0d58330c8fbc
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
While Paul's original patch in comment 0 works, the fix in comment 10 does not work for me.

With Apple LLVM version 8.0.0 (clang-800.0.42.1) (3.9.0 according to https://en.wikipedia.org/wiki/Xcode), I still get:

13:46.40 In file included from /Users/Jan/moz/mozilla-central/obj-x86_64-apple-darwin12.2.1-opt/dom/svg/Unified_cpp_dom_svg6.cpp:128:
13:46.40 /Users/Jan/moz/mozilla-central/dom/svg/SVGTests.cpp:72:43: error: default initialization of an object of const type 'const nsCaseInsensitiveStringComparator' without a user-provided default constructor
13:46.40   const nsCaseInsensitiveStringComparator caseInsensitiveComparator;
13:46.40                                           ^
13:46.40                                                                    {}
...etc.

According to https://stackoverflow.com/a/28338123/918910 

    A() = default;

does not actually provide a constructor, it does the opposite by telling the compiler that you don't want to provide one, thus the error. 

If I do

    A() {};

It works.

I believe the "default" in "user-provided default constructor" refers to the user-provided (aka non-default) constructor without any arguments.

Too many things called "default" IMHO.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
reopening is reserved for if/when we back out this patch. Please open another bug.
Status: REOPENED → RESOLVED
Closed: 2 years agoLast year
Flags: needinfo?(jib)
Resolution: --- → FIXED
Flags: needinfo?(jib)
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.