Closed
Bug 1416183
Opened 7 years ago
Closed 7 years ago
mfbt/lz4.c:310:36: error: 'register' storage class specifier is deprecated and incompatible with C++17 [-Werror,-Wdeprecated-register]
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: decoder, Assigned: Sylvestre)
References
Details
Attachments
(1 file, 1 obsolete file)
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•7 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•7 years ago
|
Attachment #8927753 -
Flags: review?(mh+mozilla)
Comment hidden (mozreview-request) |
Comment 4•7 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•7 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+
Comment 7•7 years ago
|
||
(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) |
Updated•7 years ago
|
Attachment #8928389 -
Flags: review+ → review?(nfroyd)
Comment 9•7 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•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6f31b7226512
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Assignee | ||
Updated•7 years ago
|
Attachment #8927753 -
Attachment is obsolete: true
Comment 12•7 years ago
|
||
Won't fix for 58. Let it ride the train.
Assignee | ||
Updated•5 years ago
|
Type: enhancement → defect
You need to log in
before you can comment on or make changes to this bug.
Description
•