mfbt/lz4.c:310:36: error: 'register' storage class specifier is deprecated and incompatible with C++17 [-Werror,-Wdeprecated-register]

RESOLVED FIXED in Firefox 59

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: decoder, Assigned: sylvestre)

Tracking

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 wontfix, firefox59 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
I got this error with Clang current trunk (r317840):

https://treeherder.mozilla.org/logviewer.html#?job_id=143643775&repo=try&lineNumber=3739

For now I can probably circumvent this with -Wno-deprecated-register but I guess we should eventually fix this properly.
Assignee

Comment 1

2 years ago
I have been working on it directly upstream:
https://github.com/lz4/lz4/pull/417
https://github.com/lz4/lz4/pull/419
Assignee: nobody → sledru
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8927753 - Flags: review?(mh+mozilla)
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8927753 [details]
Bug 1416183 - Silent the -Wdeprecated-register until it is fixed in lz4 upstream

https://reviewboard.mozilla.org/r/199026/#review204414

This is not the first time, and this is probably not the last time we have problems because lz4.c is built as part including it in a C++ file.

How about building it as C once and for all? It should be enough to remove the include in Compression.cpp, add a #include "lz4.h" instead (not in an anonymous namespace, obviously), and add lz4.c to SOURCES. (after bug 1416989 lands)
Attachment #8927753 - Flags: review?(mh+mozilla)
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8928389 [details]
Bug 1416183 - Build LZ4 as C instead of including it as C++.

https://reviewboard.mozilla.org/r/199594/#review204880

Thank you, this is going to make many things easier.

::: mfbt/Compression.cpp:16
(Diff revision 1)
> -using namespace mozilla::Compression;
> -
> -namespace {
> -
>  extern "C" {
> -
> +#include "lz4.h"

Are we sure this change from anonymous namespace to normal C file doesn't export more functions from libxul?

::: mfbt/moz.build
(Diff revision 1)
> -if CONFIG['GNU_CXX']:
> -    SOURCES['Compression.cpp'].flags += ['-Wno-unused-function']
> -    CXXFLAGS += ['-Wno-error=shadow']

Presumably the first one is no longer needed now that we compile lz4.c as a separate file...is the second one not needed for similar reasons?

::: mfbt/moz.build
(Diff revision 1)
> -if CONFIG['_MSC_VER']:
> -    # Error 4804 is "'>' : unsafe use of type 'bool' in operation"
> -    SOURCES['Compression.cpp'].flags += ['-wd4804']

...and I assume we're applying slightly different rules for C and C++ here as well, so we don't need to disable this warning for C?
Attachment #8928389 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #6)
> Comment on attachment 8928389 [details]
> Bug 1416183 - Build LZ4 as C instead of including it as C++.
> 
> https://reviewboard.mozilla.org/r/199594/#review204880
> 
> Thank you, this is going to make many things easier.
> 
> ::: mfbt/Compression.cpp:16
> (Diff revision 1)
> > -using namespace mozilla::Compression;
> > -
> > -namespace {
> > -
> >  extern "C" {
> > -
> > +#include "lz4.h"
> 
> Are we sure this change from anonymous namespace to normal C file doesn't
> export more functions from libxul?

damn, I thought I had looked but now looking again, there's a LZ4LIB_API that sets the visibility to default :(

(there's also an automatic extern "C", we could remote the wrapping in Compression.cpp)

Newer versions of the file actually allow to override that with LZ4LIB_VISIBILITY. Let's go with a lz4.h patch that is compatible with that.

> ::: mfbt/moz.build
> (Diff revision 1)
> > -if CONFIG['GNU_CXX']:
> > -    SOURCES['Compression.cpp'].flags += ['-Wno-unused-function']
> > -    CXXFLAGS += ['-Wno-error=shadow']
> 
> Presumably the first one is no longer needed now that we compile lz4.c as a
> separate file...is the second one not needed for similar reasons?
> 
> ::: mfbt/moz.build
> (Diff revision 1)
> > -if CONFIG['_MSC_VER']:
> > -    # Error 4804 is "'>' : unsafe use of type 'bool' in operation"
> > -    SOURCES['Compression.cpp'].flags += ['-wd4804']
> 
> ...and I assume we're applying slightly different rules for C and C++ here
> as well, so we don't need to disable this warning for C?

I'm not sure why, but I just tried and it builds just fine on all platforms without the flags. Maybe they were wranings that were due to compiling C code as C++.
Comment hidden (mozreview-request)
Attachment #8928389 - Flags: review+ → review?(nfroyd)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8928389 [details]
Bug 1416183 - Build LZ4 as C instead of including it as C++.

https://reviewboard.mozilla.org/r/199594/#review205120
Attachment #8928389 - Flags: review?(nfroyd) → review+

Comment 10

2 years ago
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/6f31b7226512
Build LZ4 as C instead of including it as C++. r=froydnj

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6f31b7226512
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Assignee

Updated

2 years ago
Attachment #8927753 - Attachment is obsolete: true
Won't fix for 58. Let it ride the train.
You need to log in before you can comment on or make changes to this bug.