1387002 - Replace .size() by .empty() when applicable

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

Fixed by http://clang.llvm.org/extra/clang-tidy/checks/readability-container-size-empty.html
"It is not guaranteed that size() is a constant-time function, and it is generally more efficient and also shows clearer intent to use empty(). "

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1387002 - Replace .size() by .empty() when applicable in webrtc & mtransport

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

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1387002 - Replace .size() by .empty() when applicable in domv/canvas/WebGL*

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

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1387002 - Replace .size() by .empty() when applicable in gmp

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

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1387002 - Replace .size() by .empty() when applicable in gfx/

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

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1387002 - Replace .size() by .empty() when applicable in dom/media/webspeech/

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

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1387002 - Replace .size() by .empty() when applicable in layout/tables/

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

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1387002 - Replace .size() by .empty() when applicable in mfbt

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

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1387002 - Replace .size() by .empty() when applicable in crashreporter/

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

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1387002 - Replace .size() by .empty() when applicable in profiler/

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

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1387002 - Replace .size() by .empty() when applicable in xpcom/

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

Comment 11

[Nicolas Silva [:nical]]

Comment on attachment 8893314 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in gfx/

https://reviewboard.mozilla.org/r/164386/#review169700

Nice.

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 8893319 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in profiler/

https://reviewboard.mozilla.org/r/164396/#review169726

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8893316 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in layout/tables/

https://reviewboard.mozilla.org/r/164390/#review169756

I've never touched this file before so I don't know why you picked me to review. But it seems trivial enough so r+

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8893317 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in mfbt

https://reviewboard.mozilla.org/r/164392/#review169782

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8893320 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in xpcom/

https://reviewboard.mozilla.org/r/164402/#review169784

Comment 16

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8893312 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in domv/canvas/WebGL*

https://reviewboard.mozilla.org/r/164382/#review169794

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8893313 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in gmp

https://reviewboard.mozilla.org/r/164384/#review170036

You've changed a bunch of existing uses of empty() in ternary expressions to change the order of the true case. I don't think this is necessary, and I think it's best to leave the order of those expressions as they were to make it more familiar on the maintainers of the code. r+ with those changes removed.

::: dom/media/webrtc/MediaTrackConstraints.h:207
(Diff revision 1)
>      ~StringRange() {}
>  
>      void SetFrom(const dom::ConstrainDOMStringParameters& aOther);
>      ValueType Clamp(const ValueType& n) const;
>      ValueType Get(const ValueType& defaultValue) const {
> -      return Clamp(!mIdeal.empty() ? mIdeal : defaultValue);
> +      return Clamp(mIdeal.empty() ? defaultValue : mIdeal);

I don't see why this one needs to change.

::: media/mtransport/nricectx.cpp:1019
(Diff revision 1)
>    for (auto& attr : attrs) {
>      attrs_in.push_back(const_cast<char *>(attr.c_str()));
>    }
>  
>    int r = nr_ice_peer_ctx_parse_global_attributes(peer_,
> -                                                  attrs_in.empty() ?
> +                                                  !attrs_in.empty() ?

I don't see why this one needs to change.

::: media/mtransport/nricemediastream.cpp:238
(Diff revision 1)
>    }
>  
>    // Still need to call nr_ice_ctx_parse_stream_attributes.
>    int r = nr_ice_peer_ctx_parse_stream_attributes(ctx_peer_,
>                                                    stream_,
> -                                                  attributes_in.empty() ?
> +                                                  !attributes_in.empty() ?

I don't see why this one needs to change.

::: media/mtransport/test/test_nr_socket_ice_unittest.cpp:197
(Diff revision 1)
>      for (auto& attr : attrs) {
>        attrs_in.push_back(const_cast<char *>(attr.c_str()));
>      }
>  
>      int r = nr_ice_peer_ctx_parse_global_attributes(peer_ctx_,
> -                                                    attrs_in.empty() ?
> +                                                    !attrs_in.empty() ?

I don't see why this one needs to change.

::: media/webrtc/signaling/gtest/mediapipeline_unittest.cpp:313
(Diff revision 1)
>    }
>  
>    uint32_t GetLocalSSRC() {
>      std::vector<uint32_t> res;
>      res = audio_conduit_->GetLocalSSRCs();
> -    return res.empty() ? 0 : res[0];
> +    return !res.empty() ? res[0] : 0;

I don't see why this one needs to change.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8893311 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in webrtc & mtransport

https://reviewboard.mozilla.org/r/164380/#review170058

::: dom/media/webrtc/MediaTrackConstraints.h:207
(Diff revision 1)
>      ~StringRange() {}
>  
>      void SetFrom(const dom::ConstrainDOMStringParameters& aOther);
>      ValueType Clamp(const ValueType& n) const;
>      ValueType Get(const ValueType& defaultValue) const {
> -      return Clamp(mIdeal.size() ? mIdeal : defaultValue);
> +      return Clamp(!mIdeal.empty() ? mIdeal : defaultValue);

instead of "!empty() ? full : empty", perhaps 
"empty() ? empty : full"

!'s tend to be missed, and you did this transformation elsewhere

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Randell Jesup [:jesup] from comment #18)
> Comment on attachment 8893311 [details]
> Bug 1387002 - Replace .size() by .empty() when applicable in webrtc &
> mtransport
> 
> https://reviewboard.mozilla.org/r/164380/#review170058
> 
> ::: dom/media/webrtc/MediaTrackConstraints.h:207
> (Diff revision 1)
> >      ~StringRange() {}
> >  
> >      void SetFrom(const dom::ConstrainDOMStringParameters& aOther);
> >      ValueType Clamp(const ValueType& n) const;
> >      ValueType Get(const ValueType& defaultValue) const {
> > -      return Clamp(mIdeal.size() ? mIdeal : defaultValue);
> > +      return Clamp(!mIdeal.empty() ? mIdeal : defaultValue);
> 
> instead of "!empty() ? full : empty", perhaps 
> "empty() ? empty : full"
> 
> !'s tend to be missed, and you did this transformation elsewhere

This change was only made in WebRTC code, so if you're happy with it then I retract my review comments.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8893311 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in webrtc & mtransport

https://reviewboard.mozilla.org/r/164380/#review170188

::: dom/media/webrtc/MediaTrackConstraints.h:207
(Diff revision 1)
>      ~StringRange() {}
>  
>      void SetFrom(const dom::ConstrainDOMStringParameters& aOther);
>      ValueType Clamp(const ValueType& n) const;
>      ValueType Get(const ValueType& defaultValue) const {
> -      return Clamp(mIdeal.size() ? mIdeal : defaultValue);
> +      return Clamp(!mIdeal.empty() ? mIdeal : defaultValue);

Oh, crap, this is what I have done here:
https://reviewboard.mozilla.org/r/164384/diff/1#index_header
wrong place :(

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8893313 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in gmp

https://reviewboard.mozilla.org/r/164384/#review170190

::: dom/media/webrtc/MediaTrackConstraints.h:207
(Diff revision 1)
>      ~StringRange() {}
>  
>      void SetFrom(const dom::ConstrainDOMStringParameters& aOther);
>      ValueType Clamp(const ValueType& n) const;
>      ValueType Get(const ValueType& defaultValue) const {
> -      return Clamp(!mIdeal.empty() ? mIdeal : defaultValue);
> +      return Clamp(mIdeal.empty() ? defaultValue : mIdeal);

Oups, it was supposed to be in
https://reviewboard.mozilla.org/r/164380/diff/1#index_header

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8893311 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in webrtc & mtransport

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

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8893312 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in domv/canvas/WebGL*

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

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8893313 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in gmp

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

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8893314 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in gfx/

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

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8893315 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in dom/media/webspeech/

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

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8893316 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in layout/tables/

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

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8893317 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in mfbt

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

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8893318 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in crashreporter/

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

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8893319 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in profiler/

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

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8893320 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in xpcom/

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

Comment 32

[Paul Adenot (:padenot)]

Comment on attachment 8893315 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in dom/media/webspeech/

https://reviewboard.mozilla.org/r/164388/#review171680

Comment 33

[(not currently active) Ted Mielczarek]

Comment on attachment 8893318 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in crashreporter/

https://reviewboard.mozilla.org/r/164394/#review172488

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33e2405b3ddd
Replace .size() by .empty() when applicable in webrtc & mtransport r=jesup
https://hg.mozilla.org/integration/autoland/rev/8dab5fd85c70
Replace .size() by .empty() when applicable in domv/canvas/WebGL* r=kvark
https://hg.mozilla.org/integration/autoland/rev/80e4a5620049
Replace .size() by .empty() when applicable in gmp r=cpearce
https://hg.mozilla.org/integration/autoland/rev/6a174665b5bc
Replace .size() by .empty() when applicable in gfx/ r=nical
https://hg.mozilla.org/integration/autoland/rev/bc8bca54da9e
Replace .size() by .empty() when applicable in dom/media/webspeech/ r=padenot
https://hg.mozilla.org/integration/autoland/rev/acafa453e8a3
Replace .size() by .empty() when applicable in layout/tables/ r=kats
https://hg.mozilla.org/integration/autoland/rev/8d9d22d532a0
Replace .size() by .empty() when applicable in mfbt r=froydnj
https://hg.mozilla.org/integration/autoland/rev/35f231196371
Replace .size() by .empty() when applicable in crashreporter/ r=ted
https://hg.mozilla.org/integration/autoland/rev/7f976dd75f7a
Replace .size() by .empty() when applicable in profiler/ r=njn
https://hg.mozilla.org/integration/autoland/rev/054d2e90007e
Replace .size() by .empty() when applicable in xpcom/ r=froydnj

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/33e2405b3ddd
https://hg.mozilla.org/mozilla-central/rev/8dab5fd85c70
https://hg.mozilla.org/mozilla-central/rev/80e4a5620049
https://hg.mozilla.org/mozilla-central/rev/6a174665b5bc
https://hg.mozilla.org/mozilla-central/rev/bc8bca54da9e
https://hg.mozilla.org/mozilla-central/rev/acafa453e8a3
https://hg.mozilla.org/mozilla-central/rev/8d9d22d532a0
https://hg.mozilla.org/mozilla-central/rev/35f231196371
https://hg.mozilla.org/mozilla-central/rev/7f976dd75f7a
https://hg.mozilla.org/mozilla-central/rev/054d2e90007e