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

RESOLVED FIXED in Firefox 57

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sylvestre, Assigned: sylvestre)

Tracking

(Blocks 1 bug)

Trunk
mozilla57
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(10 attachments)

59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
kvark
: review+
Details
59 bytes, text/x-review-board-request
cpearce
: review+
Details
59 bytes, text/x-review-board-request
nical
: review+
Details
59 bytes, text/x-review-board-request
padenot
: review+
Details
59 bytes, text/x-review-board-request
kats
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
ted
: review+
Details
59 bytes, text/x-review-board-request
njn
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
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 on attachment 8893314 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in gfx/

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

Nice.
Attachment #8893314 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8893319 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in profiler/

https://reviewboard.mozilla.org/r/164396/#review169726
Attachment #8893319 - Flags: review?(n.nethercote) → review+
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+
Attachment #8893316 - Flags: review?(bugmail) → review+
Comment on attachment 8893317 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in mfbt

https://reviewboard.mozilla.org/r/164392/#review169782
Attachment #8893317 - Flags: review?(nfroyd) → review+
Comment on attachment 8893320 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in xpcom/

https://reviewboard.mozilla.org/r/164402/#review169784
Attachment #8893320 - Flags: review?(nfroyd) → review+
Comment on attachment 8893312 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in domv/canvas/WebGL*

https://reviewboard.mozilla.org/r/164382/#review169794
Attachment #8893312 - Flags: review?(kvark) → review+
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.
Attachment #8893313 - Flags: review?(cpearce) → review+
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
Attachment #8893311 - Flags: review?(rjesup) → review+
(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 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 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
Attachment #8893315 - Flags: review?(peterv) → review?(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
Attachment #8893315 - Flags: review?(padenot) → review+
Comment on attachment 8893318 [details]
Bug 1387002 - Replace .size() by .empty() when applicable in crashreporter/

https://reviewboard.mozilla.org/r/164394/#review172488
Attachment #8893318 - Flags: review?(ted) → review+
Pushed by sledru@mozilla.com:
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
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.