Closed Bug 1356843 (Wcomma) Opened 3 years ago Closed 3 years ago

Fix and enable clang's -Wcomma warnings

Categories

(Core :: General, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(16 files)

59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
59 bytes, text/x-review-board-request
mchang
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
jorendorff
: review+
Details
59 bytes, text/x-review-board-request
aklotz
: review+
Details
59 bytes, text/x-review-board-request
jhao
: review+
Details
59 bytes, text/x-review-board-request
mossop
: review+
Details
59 bytes, text/x-review-board-request
masayuki
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
spohl
: review+
Details
59 bytes, text/x-review-board-request
mstange
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
59 bytes, text/x-review-board-request
cpeterson
: review+
Details
59 bytes, text/x-review-board-request
glandium
: review+
Details
59 bytes, text/x-review-board-request
dholbert
: review+
Details
1.63 KB, patch
Details | Diff | Splinter Review
clang's new -Wcomma warning warns about suspicious use of the comma operator such as between two statements or to call a function for side effects within an expression.

* -Wcomma warnings in Gecko C++ code:

dom/base/nsGlobalWindow.cpp:9343:55 [-Wcomma] possible misuse of comma operator here
dom/svg/SVGGeometryElement.h:117:21 [-Wcomma] possible misuse of comma operator here
dom/svg/SVGGeometryElement.h:117:41 [-Wcomma] possible misuse of comma operator here
dom/svg/SVGGeometryElement.h:128:14 [-Wcomma] possible misuse of comma operator here
dom/svg/SVGGeometryElement.h:128:23 [-Wcomma] possible misuse of comma operator here
dom/svg/SVGGeometryElement.h:128:40 [-Wcomma] possible misuse of comma operator here
dom/xml/nsXMLFragmentContentSink.cpp:227:50 [-Wcomma] possible misuse of comma operator here
extensions/spellcheck/src/mozSpellChecker.cpp:532:54 [-Wcomma] possible misuse of comma operator here
gfx/layers/Layers.cpp:1944:33 [-Wcomma] possible misuse of comma operator here
gfx/layers/Layers.cpp:1945:33 [-Wcomma] possible misuse of comma operator here
gfx/layers/Layers.cpp:1946:33 [-Wcomma] possible misuse of comma operator here
gfx/layers/Layers.cpp:1949:37 [-Wcomma] possible misuse of comma operator here
gfx/layers/Layers.cpp:1950:37 [-Wcomma] possible misuse of comma operator here
gfx/layers/Layers.cpp:1951:37 [-Wcomma] possible misuse of comma operator here
gfx/layers/Layers.cpp:1952:37 [-Wcomma] possible misuse of comma operator here
gfx/layers/Layers.cpp:1953:37 [-Wcomma] possible misuse of comma operator here
gfx/layers/Layers.cpp:1954:37 [-Wcomma] possible misuse of comma operator here
gfx/layers/Layers.cpp:1955:37 [-Wcomma] possible misuse of comma operator here
gfx/layers/Layers.cpp:1956:37 [-Wcomma] possible misuse of comma operator here
js/src/builtin/MapObject.cpp:786:48 [-Wcomma] possible misuse of comma operator here
js/src/builtin/MapObject.cpp:1371:48 [-Wcomma] possible misuse of comma operator here
js/src/builtin/RegExp.cpp:1267:62 [-Wcomma] possible misuse of comma operator here
js/src/jit/x64/BaseAssembler-x64.h:624:99 [-Wcomma] possible misuse of comma operator here
js/src/jsarray.cpp:2416:27 [-Wcomma] possible misuse of comma operator here
js/src/jscompartment.cpp:120:48 [-Wcomma] possible misuse of comma operator here
js/src/jsstr.cpp:3346:14 [-Wcomma] possible misuse of comma operator here
js/xpconnect/src/XPCWrappedNativeJSOps.cpp:316:71 [-Wcomma] possible misuse of comma operator here
layout/painting/FrameLayerBuilder.cpp:3663:31 [-Wcomma] possible misuse of comma operator here
layout/painting/FrameLayerBuilder.cpp:3673:41 [-Wcomma] possible misuse of comma operator here
layout/painting/nsCSSRenderingBorders.cpp:3336:33 [-Wcomma] possible misuse of comma operator here
layout/painting/nsCSSRenderingBorders.cpp:3336:60 [-Wcomma] possible misuse of comma operator here
layout/painting/nsCSSRenderingBorders.cpp:3337:33 [-Wcomma] possible misuse of comma operator here
layout/painting/nsCSSRenderingBorders.cpp:3337:60 [-Wcomma] possible misuse of comma operator here
layout/style/Declaration.cpp:808:41 [-Wcomma] possible misuse of comma operator here
layout/style/Declaration.cpp:812:42 [-Wcomma] possible misuse of comma operator here
layout/style/FontFaceSet.cpp:1119:60 [-Wcomma] possible misuse of comma operator here
modules/libjar/nsZipArchive.cpp:651:25 [-Wcomma] possible misuse of comma operator here
netwerk/protocol/http/Http2Stream.cpp:436:59 [-Wcomma] possible misuse of comma operator here
toolkit/components/protobuf/src/google/protobuf/stubs/strutil.cc:313:8 [-Wcomma] possible misuse of comma operator here
toolkit/components/startup/nsAppStartup.cpp:436:58 [-Wcomma] possible misuse of comma operator here
toolkit/components/statusfilter/nsBrowserStatusFilter.cpp:165:66 [-Wcomma] possible misuse of comma operator here
toolkit/xre/CreateAppData.cpp:60:51 [-Wcomma] possible misuse of comma operator here
widget/cocoa/nsDeviceContextSpecX.mm:246:26 [-Wcomma] possible misuse of comma operator here
widget/cocoa/nsDeviceContextSpecX.mm:247:32 [-Wcomma] possible misuse of comma operator here
xpcom/io/nsLocalFileUnix.cpp:725:48 [-Wcomma] possible misuse of comma operator here
xpcom/io/nsLocalFileUnix.cpp:1053:39 [-Wcomma] possible misuse of comma operator here
xpfe/appshell/nsXULWindow.cpp:2154:50 [-Wcomma] possible misuse of comma operator here

* -Wcomma warnings in third-party C/C++ code:

db/sqlite3/src/sqlite3.c:28004:12 [-Wcomma] possible misuse of comma operator here
db/sqlite3/src/sqlite3.c:28009:48 [-Wcomma] possible misuse of comma operator here
db/sqlite3/src/sqlite3.c:28009:59 [-Wcomma] possible misuse of comma operator here
db/sqlite3/src/sqlite3.c:28022:14 [-Wcomma] possible misuse of comma operator here
db/sqlite3/src/sqlite3.c:96021:69 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-bentley-ottmann-rectilinear.c:160:14 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-bentley-ottmann-rectilinear.c:171:14 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-bentley-ottmann.c:1130:14 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-bentley-ottmann.c:1145:14 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-botor-scan-converter.c:2033:12 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-image-surface.c:3513:17 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-path-stroke.c:1358:65 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-path.c:475:22 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-path.c:487:22 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-path.c:499:22 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-path.c:504:22 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-path.c:509:22 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-polygon.c:357:8 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-polygon.c:357:17 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-rectangular-scan-converter.c:347:49 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-rectangular-scan-converter.c:353:50 [-Wcomma] possible misuse of comma operator here
gfx/cairo/cairo/src/cairo-traps.c:198:11 [-Wcomma] possible misuse of comma operator here
gfx/harfbuzz/src/hb-common.cc:190:9 [-Wcomma] possible misuse of comma operator here
gfx/harfbuzz/src/hb-ot-layout-gsubgpos-private.hh:345:30 [-Wcomma] possible misuse of comma operator here
gfx/harfbuzz/src/hb-shape-plan.cc:438:26 [-Wcomma] possible misuse of comma operator here
intl/icu/source/common/ushape.cpp:1247:12 [-Wcomma] possible misuse of comma operator here
intl/icu/source/common/ustring.cpp:860:57 [-Wcomma] possible misuse of comma operator here
intl/icu/source/common/ustring.cpp:870:57 [-Wcomma] possible misuse of comma operator here
intl/icu/source/common/ustrtrns.cpp:488:47 [-Wcomma] possible misuse of comma operator here
intl/icu/source/common/ustrtrns.cpp:535:47 [-Wcomma] possible misuse of comma operator here
intl/icu/source/common/ustrtrns.cpp:609:51 [-Wcomma] possible misuse of comma operator here
intl/icu/source/common/ustrtrns.cpp:655:47 [-Wcomma] possible misuse of comma operator here
intl/icu/source/common/ustrtrns.cpp:704:47 [-Wcomma] possible misuse of comma operator here
media/ffvpx/libavcodec/vp8.c:865:46 [-Wcomma] possible misuse of comma operator here
media/ffvpx/libavutil/pixdesc.c:2424:19 [-Wcomma] possible misuse of comma operator here
media/libjpeg/jdcoefct.c:608:21 [-Wcomma] possible misuse of comma operator here
media/libjpeg/jdcoefct.c:608:39 [-Wcomma] possible misuse of comma operator here
media/libpng/pngrutil.c:3559:32 [-Wcomma] possible misuse of comma operator here
media/libpng/pngrutil.c:3580:32 [-Wcomma] possible misuse of comma operator here
media/libpng/pngrutil.c:3580:47 [-Wcomma] possible misuse of comma operator here
media/libpng/pngrutil.c:4069:27 [-Wcomma] possible misuse of comma operator here
media/libpng/pngrutil.c:4121:27 [-Wcomma] possible misuse of comma operator here
media/libpng/pngset.c:1844:25 [-Wcomma] possible misuse of comma operator here
media/libpng/pngset.c:1844:36 [-Wcomma] possible misuse of comma operator here
media/libpng/pngset.c:1851:25 [-Wcomma] possible misuse of comma operator here
media/libpng/pngset.c:1851:36 [-Wcomma] possible misuse of comma operator here
media/libpng/pngset.c:1864:16 [-Wcomma] possible misuse of comma operator here
media/webrtc/trunk/webrtc/modules/audio_coding/neteq/background_noise.h:98:22 [-Wcomma] possible misuse of comma operator here
media/webrtc/trunk/webrtc/modules/desktop_capture/differ_unittest.cc:187:22 [-Wcomma] possible misuse of comma operator here
media/webrtc/trunk/webrtc/voice_engine/channel.cc:1200:39 [-Wcomma] possible misuse of comma operator here
modules/zlib/src/deflate.c:1341:18 [-Wcomma] possible misuse of comma operator here
modules/zlib/src/deflate.c:1958:41 [-Wcomma] possible misuse of comma operator here
modules/zlib/src/trees.c:513:49 [-Wcomma] possible misuse of comma operator here
modules/zlib/src/trees.c:630:20 [-Wcomma] possible misuse of comma operator here
modules/zlib/src/trees.c:716:38 [-Wcomma] possible misuse of comma operator here
modules/zlib/src/trees.c:735:28 [-Wcomma] possible misuse of comma operator here
modules/zlib/src/trees.c:737:26 [-Wcomma] possible misuse of comma operator here
modules/zlib/src/trees.c:739:26 [-Wcomma] possible misuse of comma operator here
modules/zlib/src/trees.c:762:38 [-Wcomma] possible misuse of comma operator here
modules/zlib/src/trees.c:786:28 [-Wcomma] possible misuse of comma operator here
modules/zlib/src/trees.c:788:26 [-Wcomma] possible misuse of comma operator here
modules/zlib/src/trees.c:790:26 [-Wcomma] possible misuse of comma operator here
modules/zlib/src/trees.c:1165:19 [-Wcomma] possible misuse of comma operator here
netwerk/dns/punycode.c:188:12 [-Wcomma] possible misuse of comma operator here
nsprpub/pr/src/misc/praton.c:124:26 [-Wcomma] possible misuse of comma operator here
security/nss/cmd/shlibsign/shlibsign.c:1006:42 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:554:27 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:562:28 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:582:25 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:590:24 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:614:22 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:622:22 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:634:23 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:642:23 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:659:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:660:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:661:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:662:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:663:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:664:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:665:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:666:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:667:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:668:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:669:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:670:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:671:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:672:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:673:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:674:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:675:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:676:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:677:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:678:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:679:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:680:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:681:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:682:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:683:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:684:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:685:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:686:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:687:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:688:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:689:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:690:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:691:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:692:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:693:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:694:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:867:27 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:875:28 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:883:28 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:903:25 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:911:25 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:919:24 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:943:22 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:951:22 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:963:23 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:971:23 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:983:23 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:991:23 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1008:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1009:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1010:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1011:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1012:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1013:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1014:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1015:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1016:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1017:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1018:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1019:49 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1020:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1021:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1022:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1023:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1024:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1025:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1026:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1027:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1028:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1029:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1030:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1031:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1032:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1033:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1034:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1035:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1036:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1037:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1038:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1039:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1040:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1041:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1042:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1043:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1044:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1045:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1046:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1047:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1048:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1049:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1050:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1051:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1052:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1053:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1054:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/camellia.c:1055:51 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/mpi/mpi.c:4108:13 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/mpi/mpi.c:4110:17 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/mpi/mpi.c:4119:13 [-Wcomma] possible misuse of comma operator here
security/nss/lib/freebl/mpi/mpi.c:4121:17 [-Wcomma] possible misuse of comma operator here
toolkit/components/protobuf/src/google/protobuf/stubs/strutil.cc:313:8 [-Wcomma] possible misuse of comma operator here
Comment on attachment 8858585 [details]
Bug 1356843 - Fix -Wcomma warnings in widget/cocoa/nsDeviceContextSpecX.mm.

https://reviewboard.mozilla.org/r/130576/#review133218
Attachment #8858585 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8858586 [details]
Bug 1356843 - Fix -Wcomma warning in xpfe/appshell/nsXULWindow.cpp.

https://reviewboard.mozilla.org/r/130578/#review133220
Attachment #8858586 - Flags: review?(mstange) → review+
Here is a most green Try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3a294174903050db2b9ee51bc28ac4e50e3ae5d&selectedJob=91919499

The JS changes appear to permafail the jit tests on "Windows 7 VM debug" (taskcluster) but not "Windows 7 debug" (buildbot). This appears to be existing bug 1354273 where any change that triggers these jit tests to run will fail (so either an existing bug or a test configuration issue on taskcluster).

I will remove the js review request until bug 1354273 is resolved so I can request review with a clean (green) conscience.
Depends on: 1354273
Comment on attachment 8858581 [details]
Bug 1356843 - Fix -Wcomma warning in netwerk/protocol/http/Http2Stream.cpp.

https://reviewboard.mozilla.org/r/130568/#review133238

Thanks for fixing this.
Attachment #8858581 - Flags: review?(jhao) → review+
Comment on attachment 8858582 [details]
Bug 1356843 - Fix -Wcomma warnings in toolkit/.

https://reviewboard.mozilla.org/r/130570/#review133240
Attachment #8858582 - Flags: review?(dtownsend) → review+
Comment on attachment 8858583 [details]
Bug 1356843 - Fix -Wcomma warning in extensions/spellcheck/src/mozSpellChecker.cpp.

https://reviewboard.mozilla.org/r/130572/#review133268
Attachment #8858583 - Flags: review?(masayuki) → review+
Comment on attachment 8858589 [details]
Bug 1356843 - Enable -Wcomma clang warnings.

https://reviewboard.mozilla.org/r/130584/#review133270

::: gfx/harfbuzz/src/moz.build:77
(Diff revision 1)
> +if CONFIG['CLANG_CXX']:
> +    CXXFLAGS += ['-Wno-comma']

It feels like we should keep those, and push for fixing them upstream.
Attachment #8858589 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8858587 [details]
Bug 1356843 - Fix -Wcomma warnings in layout/.

https://reviewboard.mozilla.org/r/130580/#review133224

r=me with one nit:

::: layout/style/FontFaceSet.cpp:1120
(Diff revision 1)
> -                   val.GetUnit() == eCSSUnit_Font_Format)) {
> +          while (i + 1 < numSrc) {
> +            val = srcArr->Item(i+1);

As long as you're splitting this out, please also fix up the style of (i+1) by adding spaces around the +.
Attachment #8858587 - Flags: review?(dholbert) → review+
Comment on attachment 8858576 [details]
Bug 1356843 - Fix -Wcomma warnings in dom/base/ and dom/xml/.

https://reviewboard.mozilla.org/r/130558/#review133276

I'll review the SVG changes in this patch, but you split the other changes (in dom/base and dom/xml) into a separate patch and have a DOM peer review them.  Having said that, see my suggestion for the dom/base change below.

::: dom/base/nsGlobalWindow.cpp:9336
(Diff revision 1)
>          // XXXbz now that we have mHavePendingClose, is this needed?
>          bool isTab = false;
> -        if (rootWin == AsOuter() ||
> -            !bwin || (bwin->IsTabContentWindow(GetOuterWindowInternal(),
> -                                               &isTab), isTab))
> +        if (bwin)
> +          bwin->IsTabContentWindow(GetOuterWindowInternal(), &isTab);
> +
> +        if (rootWin == AsOuter() || !bwin || isTab)
>            treeOwnerAsWin->Destroy();
>        }

(1) This file's changes should really be reviewed by a DOM peer.  (If this were an exact conversion of the logic, I'd just rubberstamp it -- but it's not an exact conversion. You're swapping around some conditions and calling bwin->IsTabContentWindow more eagerly/early now, which might have performance implications.)

(2) While you're touching this conditional, you should really add braces around its body (especially since it's at the very end of a higher level of nesting, which makes the closing curly-brace on the next line ambiguous-to-humans).

(3) bwin->IsTabContentWindow() is an XPCOM method which returns "nsresult", so technically we should've always been checking it for success before we inspect its outparam.  SO, rather than splitting this out into two expressions, I suspect you really want to just convert the "bwin->IsTabContentWindow(...)," expression to "NS_SUCCEEDED(bwin->IsTabContentWindow(...)) &&"

So I think this should really end up looking something like this:

        if (rootWin == AsOuter() ||
            !bwin ||
            (NS_SUCCEEDED(bwin->IsTabContentWindow(GetOuterWindowInternal(),
                                                   &isTab)) && isTab)) {
          treeOwnerAsWin->Destroy();
        }

But again, you should have a DOM peer sign off on that change [or whatever change you want to make here].

::: dom/svg/SVGGeometryElement.h:117
(Diff revision 1)
> -      mX = x; mY = y, mWidthOrX2 = width, mHeightOrY2 = height;
> +      mX = x; mY = y; mWidthOrX2 = width; mHeightOrY2 = height;
>        mType = RECT;
>      }
>      Rect AsRect() const {
>        MOZ_ASSERT(mType == RECT);
>        return Rect(mX, mY, mWidthOrX2, mHeightOrY2);
>      }
>      bool IsRect() const {
>        return mType == RECT;
>      }
>      void SetLine(Float x1, Float y1, Float x2, Float y2) {
> -      mX = x1, mY = y1, mWidthOrX2 = x2, mHeightOrY2 = y2;
> +      mX = x1; mY = y1; mWidthOrX2 = x2; mHeightOrY2 = y2;

Please wrap these to have one statement per line.
Attachment #8858576 - Flags: review?(dholbert) → review-
Comment on attachment 8858584 [details]
Bug 1356843 - Fix -Wcomma warnings in xpcom/io/nsLocalFileUnix.cpp.

https://reviewboard.mozilla.org/r/130574/#review133346
Attachment #8858584 - Flags: review?(nfroyd) → review+
Comment on attachment 8858577 [details]
Bug 1356843 - Fix -Wcomma warnings in gfx/layers/Layers.cpp.

https://reviewboard.mozilla.org/r/130560/#review133392
Attachment #8858577 - Flags: review?(mchang) → review+
Comment on attachment 8858580 [details]
Bug 1356843 - Fix -Wcomma warning in modules/libjar/nsZipArchive.cpp.

https://reviewboard.mozilla.org/r/130566/#review133434
Attachment #8858580 - Flags: review?(aklotz) → review+
(In reply to Mike Hommey [:glandium] (VAC: Apr 20-May 4) from comment #21)
> ::: gfx/harfbuzz/src/moz.build:77
> (Diff revision 1)
> > +if CONFIG['CLANG_CXX']:
> > +    CXXFLAGS += ['-Wno-comma']
> 
> It feels like we should keep those, and push for fixing them upstream.

OK. I filed a harfbuzz bug upstream: https://github.com/behdad/harfbuzz/issues/471


(In reply to Daniel Holbert [:dholbert] from comment #23)
> Comment on attachment 8858576 [details]
> Bug 1356843 - Fix -Wcomma warnings in dom/.
> 
> https://reviewboard.mozilla.org/r/130558/#review133276
> 
> I'll review the SVG changes in this patch, but you split the other changes
> (in dom/base and dom/xml) into a separate patch and have a DOM peer review
> them.  Having said that, see my suggestion for the dom/base change below.

Thanks. I'll make these changes and ask for a DOM peer review for the dom/base and xml changes.
Comment on attachment 8859018 [details]
Bug 1356843 - Fix -Wcomma warnings in dom/svg/SVGGeomeetryElement.h.

https://reviewboard.mozilla.org/r/131036/#review133626

Thanks! r=me
Attachment #8859018 - Flags: review?(dholbert) → review+
Comment on attachment 8858578 [details]
Bug 1356843 - Fix -Wcomma warning in intl/uconv/util/nsUCSupport.cpp.

https://reviewboard.mozilla.org/r/130562/#review133628

I'm not sure how actively smontagu is watching bugmail -- while I'm here, this part is trivial/obviously-correct-enough that I'm comfortable r+'ing this one.
Attachment #8858578 - Flags: review+
Attachment #8858578 - Flags: review?(smontagu)
Attachment #8858588 - Flags: review?(jseward) → review+
Comment on attachment 8858576 [details]
Bug 1356843 - Fix -Wcomma warnings in dom/base/ and dom/xml/.

https://reviewboard.mozilla.org/r/130558/#review134166

::: dom/xml/nsXMLFragmentContentSink.cpp:228
(Diff revision 3)
>  nsresult
>  nsXMLFragmentContentSink::CloseElement(nsIContent* aContent)
>  {
>    // don't do fancy stuff in nsXMLContentSink
>    if (mPreventScriptExecution &&
> -      (aContent->IsHTMLElement(nsGkAtoms::script),
> +      (aContent->IsHTMLElement(nsGkAtoms::script) ||

Ugh.  So this was a bug introduced in bug 1134280...

We may want to backport this part to branches, because not preventing execution when it should be prevented seems bad!
Attachment #8858576 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #53)
> Ugh.  So this was a bug introduced in bug 1134280...
> 
> We may want to backport this part to branches, because not preventing
> execution when it should be prevented seems bad!

Thanks. I will uplift this to Beta. (RIP Aurora.) Without STR or a test case demonstrating a bug, I doubt release management will approve a backport to the Release channel or ESR.
Attachment #8858588 - Flags: review+ → review?(jseward)
Comment on attachment 8858588 [details]
Bug 1356843 - Fix -Wcomma warning in tools/profiler/lul/LulDwarf.cpp.

restoring r=jseward flag that was stomped on by mozreview
Attachment #8858588 - Flags: review?(jseward) → review+
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd29e77dcca5
Fix -Wcomma warnings in dom/base/ and dom/xml/. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/89fc84197a69
Fix -Wcomma warnings in dom/svg/SVGGeomeetryElement.h. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1d8bafd90ac
Fix -Wcomma warning in extensions/spellcheck/src/mozSpellChecker.cpp. r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd2486567850
Fix -Wcomma warnings in gfx/layers/Layers.cpp. r=mchang
https://hg.mozilla.org/integration/mozilla-inbound/rev/079ecec4a4f4
Fix -Wcomma warning in intl/uconv/util/nsUCSupport.cpp. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/92dcecd49be3
Fix -Wcomma warnings in layout/. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/725c90da85e6
Fix -Wcomma warning in modules/libjar/nsZipArchive.cpp. r=aklotz
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e5eb701aed0
Fix -Wcomma warning in netwerk/protocol/http/Http2Stream.cpp. r=jhao
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba51d0bb0f8b
Fix -Wcomma warnings in toolkit/. r=mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/1842e1612409
Fix -Wcomma warning in tools/profiler/lul/LulDwarf.cpp. r=jseward
https://hg.mozilla.org/integration/mozilla-inbound/rev/858182626955
Fix -Wcomma warnings in widget/cocoa/nsDeviceContextSpecX.mm. r=spohl
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef65aa6f8946
Fix -Wcomma warnings in xpcom/io/nsLocalFileUnix.cpp. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/c835b72c4ccc
Fix -Wcomma warning in xpfe/appshell/nsXULWindow.cpp. r=mstange
Keywords: leave-open
Comment on attachment 8858579 [details]
Bug 1356843 - Fix -Wcomma warnings in js/.

https://reviewboard.mozilla.org/r/130564/#review135004
Attachment #8858579 - Flags: review?(jorendorff) → review+
Keywords: leave-open
Depends on: 1358502
Comment on attachment 8858576 [details]
Bug 1356843 - Fix -Wcomma warnings in dom/base/ and dom/xml/.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1134280 back in 2015
[User impact if declined]: Without this fix, some XML files may contain JavaScript the author intended to run instead of the default DOM JavaScript behavior, but instead both may run.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not really
[Why is the change risky/not risky?]: This change reverts a typo from bug 1134280 back in 2015, restoring the original behavior. However, we don't have a test case or STR demonstrating the unintended behavior. Since AFAIK no one has complained about the regression, we might be able to wait for the fix to ride in 55 instead of uplifting to 54, if rel man is concerned about minimizing risk in Beta 54.
[String changes made/needed]: none
Attachment #8858576 - Flags: approval-mozilla-beta?
Comment on attachment 8858576 [details]
Bug 1356843 - Fix -Wcomma warnings in dom/base/ and dom/xml/.

Thanks for understanding. Like you mentioned in the request, it's been there for a while and no one has complained about the regression. Let's let it ride the train on 55 to minimize the risk. Beta54-.
Attachment #8858576 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
> Since AFAIK no one has complained about the regression

Just to be clear, that regression is a security bug.  It should be evaluated as such for uplift purposes.
Flags: needinfo?(gchang)
Flags: needinfo?(cpeterson)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #65)
> Just to be clear, that regression is a security bug.  It should be evaluated
> as such for uplift purposes.

Gerry, I didn't understand that this fix was a potential security bug. I made a smaller patch here to further reduce regression risk. Please consider this smaller patch for beta and ESR 52 uplift.

[Approval Request Comment]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: bz says this regression is a security bug (comment 65).

User impact if declined: Without this fix, some XML files may contain JavaScript the author intended to run instead of the default DOM JavaScript behavior, but with this bug both may run.

Fix Landed on Version: 55

Risk to taking this patch (and alternatives if risky): This should not be a risky change because it reverts a typo from bug 1134280 back in 2015. However, I don't currently have a test case or STR demonstrating the unintended JavaScript execution.

String or UUID changes made by this patch: None
Flags: needinfo?(cpeterson)
Attachment #8862299 - Flags: approval-mozilla-esr52?
Attachment #8862299 - Flags: approval-mozilla-beta?
Hi Chris,
Does this patch need to be reviewed or be landed in m-c?
Flags: needinfo?(gchang) → needinfo?(cpeterson)
(In reply to Gerry Chang [:gchang] from comment #67)
> Does this patch need to be reviewed or be landed in m-c?

No. uplift-nsXMLFragmentContentSink-fix.patch is a subset of the "Bug 1356843 - Fix -Wcomma warnings in dom/base/ and dom/xml/." patch which already landed in m-c here:

https://hg.mozilla.org/mozilla-central/rev/fd29e77dcca5

For comparison, here is the regression changeset where the `||` was inadvertently replaced with `,`:

https://hg.mozilla.org/mozilla-central/diff/ec0d84308d7c/dom/xml/nsXMLFragmentContentSink.cpp
Flags: needinfo?(cpeterson)
Comment on attachment 8862299 [details] [diff] [review]
uplift-nsXMLFragmentContentSink-fix.patch

Per comment #65, fix a security bug. Beta54+. Should be in 54 beta 4.
Attachment #8862299 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8862299 [details] [diff] [review]
uplift-nsXMLFragmentContentSink-fix.patch

makes sense, ESR52.2+
Attachment #8862299 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.