1356843 - (Wcomma) Fix and enable clang's -Wcomma warnings

Status: RESOLVED FIXED

(Core :: General, defect, P3)

Description

[Chris Peterson [:cpeterson]]

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 1

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warnings in dom/base/ and dom/xml/.

clang's -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.

Note that this warning indicates that nsXMLFragmentContentSink::CloseElement() has been calling aContent->IsHTMLElement(nsGkAtoms::script) but not using its return value to determine when to call PreventExecution() since bug 1134280 changed this code in 2015!

dom/base/nsGlobalWindow.cpp:9344: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

Review commit: https://reviewboard.mozilla.org/r/130558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130558/

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warnings in gfx/layers/Layers.cpp.

clang's -Wcomma warning warns about suspicious use of the comma operator such as between two statements.

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

Review commit: https://reviewboard.mozilla.org/r/130560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130560/

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warning in intl/uconv/util/nsUCSupport.cpp.

clang's -Wcomma warning warns about suspicious use of the comma operator such as between two statements.

intl/uconv/util/nsUCSupport.cpp:552:34 [-Wcomma] possible misuse of comma operator here

Review commit: https://reviewboard.mozilla.org/r/130562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130562/

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warnings in js/.

clang's -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.

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:1266: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

Review commit: https://reviewboard.mozilla.org/r/130564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130564/

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warning in modules/libjar/nsZipArchive.cpp.

clang's -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.

modules/libjar/nsZipArchive.cpp:651:25 [-Wcomma] possible misuse of comma operator here

Review commit: https://reviewboard.mozilla.org/r/130566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130566/

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warning in netwerk/protocol/http/Http2Stream.cpp.

clang's -Wcomma warning warns about suspicious use of the comma operator such as between two statements.

netwerk/protocol/http/Http2Stream.cpp:436:59 [-Wcomma] possible misuse of comma operator here

Review commit: https://reviewboard.mozilla.org/r/130568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130568/

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warnings in toolkit/.

clang's -Wcomma warning warns about suspicious use of the comma operator such as between two statements or calling a function for side effects within an expression. Check NS_SUCCEEDED() to use HasMoreElement() in an expression and confirm that it actually returned a legitimate out parameter.

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

Review commit: https://reviewboard.mozilla.org/r/130570/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130570/

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warning in extensions/spellcheck/src/mozSpellChecker.cpp.

clang's -Wcomma warning warns about suspicious use of the comma operator such as calling a function for side effects within an expression. Check NS_SUCCEEDED() to use HasMoreElement() in an expression and confirm that it actually returned a legitimate out parameter.

extensions/spellcheck/src/mozSpellChecker.cpp:532:54 [-Wcomma] possible misuse of comma operator here

Review commit: https://reviewboard.mozilla.org/r/130572/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130572/

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warnings in xpcom/io/nsLocalFileUnix.cpp.

clang's -Wcomma warning warns about suspicious use of the comma operator such as calling a function for side effects within an expression. Check NS_SUCCEEDED() to use HasMoreElement() in an expression and confirm that it actually returned a legitimate out parameter.

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

Review commit: https://reviewboard.mozilla.org/r/130574/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130574/

Comment 10

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warnings in widget/cocoa/nsDeviceContextSpecX.mm.

clang's -Wcomma warning warns about suspicious use of the comma operator such as between two statements.

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

Review commit: https://reviewboard.mozilla.org/r/130576/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130576/

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warning in xpfe/appshell/nsXULWindow.cpp.

clang's -Wcomma warning warns about suspicious use of the comma operator such as calling a function for side effects within an expression. Check NS_SUCCEEDED() to use HasMoreElement() in an expression and confirm that it actually returned a legitimate out parameter.

xpfe/appshell/nsXULWindow.cpp:2154:50 [-Wcomma] possible misuse of comma operator here

Review commit: https://reviewboard.mozilla.org/r/130578/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130578/

Comment 12

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warnings in layout/.

clang's -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. In addition to fixing the warnings, replace some char16_t(' ') casts of char literals with shorter UTF-16 character literals u' '.

layout/painting/FrameLayerBuilder.cpp:3647:31 [-Wcomma] possible misuse of comma operator here
layout/painting/FrameLayerBuilder.cpp:3657: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:1118:60 [-Wcomma] possible misuse of comma operator here

Review commit: https://reviewboard.mozilla.org/r/130580/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130580/

Comment 13

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warning in tools/profiler/lul/LulDwarf.cpp.

clang's -Wcomma warning warns about suspicious use of the comma operator such as between two statements.

tools/profiler/lul/LulDwarf.cpp:604:15: warning: possible misuse of comma operator here [-Wcomma]

Review commit: https://reviewboard.mozilla.org/r/130582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130582/

Comment 14

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Enable -Wcomma clang warnings.

Warn about possible misuse of the comma operator such as between two statements or to call a function for side effects within an expression. Only enable these -Wcomma warnings for C++ code because there are almost two hundred -Wcomma warnings in third-party C libraries. I reviewed the C warnings and confirmed none of them were latent bugs. We won't fix these libraries' warnings so they are just noise.

Review commit: https://reviewboard.mozilla.org/r/130584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130584/

Comment 15

[Stephen A Pohl [:spohl]]

Comment on attachment 8858585 [details]
Bug 1356843 - Fix -Wcomma warnings in widget/cocoa/nsDeviceContextSpecX.mm.

https://reviewboard.mozilla.org/r/130576/#review133218

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8858586 [details]
Bug 1356843 - Fix -Wcomma warning in xpfe/appshell/nsXULWindow.cpp.

https://reviewboard.mozilla.org/r/130578/#review133220

Comment 17

[Chris Peterson [:cpeterson]]

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.

Comment 18

[Jonathan Hao (inactive) [:jhao]]

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.

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 8858582 [details]
Bug 1356843 - Fix -Wcomma warnings in toolkit/.

https://reviewboard.mozilla.org/r/130570/#review133240

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8858583 [details]
Bug 1356843 - Fix -Wcomma warning in extensions/spellcheck/src/mozSpellChecker.cpp.

https://reviewboard.mozilla.org/r/130572/#review133268

Comment 21

[Mike Hommey [:glandium]]

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.

Comment 22

[Daniel Holbert [:dholbert]]

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 +.

Comment 23

[Daniel Holbert [:dholbert]]

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.

Comment 24

[Nathan Froyd [:froydnj]]

Comment on attachment 8858584 [details]
Bug 1356843 - Fix -Wcomma warnings in xpcom/io/nsLocalFileUnix.cpp.

https://reviewboard.mozilla.org/r/130574/#review133346

Comment 25

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8858577 [details]
Bug 1356843 - Fix -Wcomma warnings in gfx/layers/Layers.cpp.

https://reviewboard.mozilla.org/r/130560/#review133392

Comment 26

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8858580 [details]
Bug 1356843 - Fix -Wcomma warning in modules/libjar/nsZipArchive.cpp.

https://reviewboard.mozilla.org/r/130566/#review133434

Comment 27

[Chris Peterson [:cpeterson]]

(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 28

[Chris Peterson [:cpeterson]]

Attachment: Bug 1356843 - Fix -Wcomma warnings in dom/svg/SVGGeomeetryElement.h.

clang's -Wcomma warning warns about suspicious use of the comma operator such as between two statements.

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

Review commit: https://reviewboard.mozilla.org/r/131036/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131036/

Comment 29

[Chris Peterson [:cpeterson]]

Comment on attachment 8858583 [details]
Bug 1356843 - Fix -Wcomma warning in extensions/spellcheck/src/mozSpellChecker.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130572/diff/1-2/

Comment 30

[Chris Peterson [:cpeterson]]

Comment on attachment 8858577 [details]
Bug 1356843 - Fix -Wcomma warnings in gfx/layers/Layers.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130560/diff/1-2/

Comment 31

[Chris Peterson [:cpeterson]]

Comment on attachment 8858587 [details]
Bug 1356843 - Fix -Wcomma warnings in layout/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130580/diff/1-2/

Comment 32

[Chris Peterson [:cpeterson]]

Comment on attachment 8858580 [details]
Bug 1356843 - Fix -Wcomma warning in modules/libjar/nsZipArchive.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130566/diff/1-2/

Comment 33

[Chris Peterson [:cpeterson]]

Comment on attachment 8858581 [details]
Bug 1356843 - Fix -Wcomma warning in netwerk/protocol/http/Http2Stream.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130568/diff/1-2/

Comment 34

[Chris Peterson [:cpeterson]]

Comment on attachment 8858582 [details]
Bug 1356843 - Fix -Wcomma warnings in toolkit/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130570/diff/1-2/

Comment 35

[Chris Peterson [:cpeterson]]

Comment on attachment 8858588 [details]
Bug 1356843 - Fix -Wcomma warning in tools/profiler/lul/LulDwarf.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130582/diff/1-2/

Comment 36

[Chris Peterson [:cpeterson]]

Comment on attachment 8858585 [details]
Bug 1356843 - Fix -Wcomma warnings in widget/cocoa/nsDeviceContextSpecX.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130576/diff/1-2/

Comment 37

[Chris Peterson [:cpeterson]]

Comment on attachment 8858584 [details]
Bug 1356843 - Fix -Wcomma warnings in xpcom/io/nsLocalFileUnix.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130574/diff/1-2/

Comment 38

[Chris Peterson [:cpeterson]]

Comment on attachment 8858586 [details]
Bug 1356843 - Fix -Wcomma warning in xpfe/appshell/nsXULWindow.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130578/diff/1-2/

Comment 39

[Chris Peterson [:cpeterson]]

Comment on attachment 8858589 [details]
Bug 1356843 - Enable -Wcomma clang warnings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130584/diff/1-2/

Comment 40

[Chris Peterson [:cpeterson]]

Comment on attachment 8858578 [details]
Bug 1356843 - Fix -Wcomma warning in intl/uconv/util/nsUCSupport.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130562/diff/1-2/

Comment 41

[Chris Peterson [:cpeterson]]

Comment on attachment 8858579 [details]
Bug 1356843 - Fix -Wcomma warnings in js/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130564/diff/1-2/

Comment 42

[Chris Peterson [:cpeterson]]

Comment on attachment 8858576 [details]
Bug 1356843 - Fix -Wcomma warnings in dom/base/ and dom/xml/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130558/diff/1-2/

Comment 43

[Chris Peterson [:cpeterson]]

Comment on attachment 8858585 [details]
Bug 1356843 - Fix -Wcomma warnings in widget/cocoa/nsDeviceContextSpecX.mm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130576/diff/2-3/

Comment 44

[Chris Peterson [:cpeterson]]

Comment on attachment 8858584 [details]
Bug 1356843 - Fix -Wcomma warnings in xpcom/io/nsLocalFileUnix.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130574/diff/2-3/

Comment 45

[Chris Peterson [:cpeterson]]

Comment on attachment 8858586 [details]
Bug 1356843 - Fix -Wcomma warning in xpfe/appshell/nsXULWindow.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130578/diff/2-3/

Comment 46

[Chris Peterson [:cpeterson]]

Comment on attachment 8858589 [details]
Bug 1356843 - Enable -Wcomma clang warnings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130584/diff/2-3/

Comment 47

[Chris Peterson [:cpeterson]]

Comment on attachment 8858576 [details]
Bug 1356843 - Fix -Wcomma warnings in dom/base/ and dom/xml/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130558/diff/2-3/

Comment 48

[Chris Peterson [:cpeterson]]

Comment on attachment 8858578 [details]
Bug 1356843 - Fix -Wcomma warning in intl/uconv/util/nsUCSupport.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130562/diff/2-3/

Comment 49

[Chris Peterson [:cpeterson]]

Comment on attachment 8858579 [details]
Bug 1356843 - Fix -Wcomma warnings in js/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130564/diff/2-3/

Comment 50

[Chris Peterson [:cpeterson]]

Comment on attachment 8858588 [details]
Bug 1356843 - Fix -Wcomma warning in tools/profiler/lul/LulDwarf.cpp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130582/diff/2-3/

Comment 51

[Daniel Holbert [:dholbert]]

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

Comment 52

[Daniel Holbert [:dholbert]]

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.

Comment 53

[Boris Zbarsky [:bzbarsky]]

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!

Comment 54

[Chris Peterson [:cpeterson]]

(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.

Comment 55

[Chris Peterson [:cpeterson]]

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

Comment 56

[Pulsebot]

Pushed by [e-mail]:
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

Comment 57

[Jason Orendorff [:jorendorff]]

Comment on attachment 8858579 [details]
Bug 1356843 - Fix -Wcomma warnings in js/.

https://reviewboard.mozilla.org/r/130564/#review135002

Comment 58

[Jason Orendorff [:jorendorff]]

Comment on attachment 8858579 [details]
Bug 1356843 - Fix -Wcomma warnings in js/.

https://reviewboard.mozilla.org/r/130564/#review135004

Comment 59

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fd29e77dcca5
https://hg.mozilla.org/mozilla-central/rev/89fc84197a69
https://hg.mozilla.org/mozilla-central/rev/e1d8bafd90ac
https://hg.mozilla.org/mozilla-central/rev/fd2486567850
https://hg.mozilla.org/mozilla-central/rev/079ecec4a4f4
https://hg.mozilla.org/mozilla-central/rev/92dcecd49be3
https://hg.mozilla.org/mozilla-central/rev/725c90da85e6
https://hg.mozilla.org/mozilla-central/rev/3e5eb701aed0
https://hg.mozilla.org/mozilla-central/rev/ba51d0bb0f8b
https://hg.mozilla.org/mozilla-central/rev/1842e1612409
https://hg.mozilla.org/mozilla-central/rev/858182626955
https://hg.mozilla.org/mozilla-central/rev/ef65aa6f8946
https://hg.mozilla.org/mozilla-central/rev/c835b72c4ccc

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6deb37b2d3a1
Fix -Wcomma warnings in js/. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdef06c12da6
Enable -Wcomma clang warnings. r=glandium

Comment 61

[Chris Peterson [:cpeterson]]

I fixed the -Wcomma warnings in harfbuzz upstream:

https://github.com/behdad/harfbuzz/commit/aacca37590656e235218557ea509eb5624dfbff9

Comment 62

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6deb37b2d3a1
https://hg.mozilla.org/mozilla-central/rev/bdef06c12da6

Comment 63

[Chris Peterson [:cpeterson]]

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

Comment 64

[Gerry Chang [:gchang]]

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-.

Comment 65

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 66

[Chris Peterson [:cpeterson]]

Attachment: uplift-nsXMLFragmentContentSink-fix.patch

(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

Comment 67

[Gerry Chang [:gchang]]

Hi Chris,
Does this patch need to be reviewed or be landed in m-c?

Comment 68

[Chris Peterson [: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

Comment 69

[Gerry Chang [:gchang]]

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.

Comment 70

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d9d5690e22ee

Comment 71

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8862299 [details] [diff] [review]
uplift-nsXMLFragmentContentSink-fix.patch

makes sense, ESR52.2+

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/3a80d48a9249