Issue 29877558: Noissue - Avoid redundant lookup in keyword-by-filter map (Closed)
|
Created:
Sept. 10, 2018, 12:21 p.m. by Manish Jethani Modified:
Sept. 19, 2018, 6:47 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionNoissue - Avoid redundant lookup in keyword-by-filter map
Patch Set 1 #
Total comments: 7
Patch Set 2 : Use string comparison #
Total comments: 1
Patch Set 3 : Revert to Patch Set 1 #MessagesTotal messages: 17
Patch Set 1 There's no need to call Map.has() when we're going to call Map.get() anyway for non-slow filters. Map.has() and Map.get() have the same time complexity (equally slow/fast).
https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js#newc... lib/matcher.js:351: return !keyword; Why the `!keyword`. I'd just return the proper boolean value here.
https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js#newc... lib/matcher.js:351: return !keyword; On 2018/09/13 15:52:40, hub wrote: > Why the `!keyword`. I'd just return the proper boolean value here. Do you mean `keyword != ""`?
https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js#newc... lib/matcher.js:351: return !keyword; On 2018/09/14 05:28:32, Manish Jethani wrote: > On 2018/09/13 15:52:40, hub wrote: > > Why the `!keyword`. I'd just return the proper boolean value here. > > Do you mean `keyword != ""`? no. just `return true`
https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js#newc... lib/matcher.js:351: return !keyword; On 2018/09/14 12:20:49, hub wrote: > On 2018/09/14 05:28:32, Manish Jethani wrote: > > On 2018/09/13 15:52:40, hub wrote: > > > Why the `!keyword`. I'd just return the proper boolean value here. > > > > Do you mean `keyword != ""`? > > no. just `return true` Well it should be false if the keyword is an empty string.
https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js#newc... lib/matcher.js:351: return !keyword; On 2018/09/14 13:36:25, Manish Jethani wrote: > On 2018/09/14 12:20:49, hub wrote: > > On 2018/09/14 05:28:32, Manish Jethani wrote: > > > On 2018/09/13 15:52:40, hub wrote: > > > > Why the `!keyword`. I'd just return the proper boolean value here. > > > > > > Do you mean `keyword != ""`? > > > > no. just `return true` > > Well it should be false if the keyword is an empty string. So, returning !keyword here would return true in the case where keyword is an empty string, and false if it is not empty. Is that correct? perhaps based solely on the discussion here it makes sense to return a bool for readability? I do see that that would add an additional step. Personally, I find that this approach makes sense and fits the previous convention (although that is not an argument one way or another just a fact)
https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js#newc... lib/matcher.js:351: return !keyword; On 2018/09/17 19:13:18, Jon Sonesen wrote: > On 2018/09/14 13:36:25, Manish Jethani wrote: > > On 2018/09/14 12:20:49, hub wrote: > > > On 2018/09/14 05:28:32, Manish Jethani wrote: > > > > On 2018/09/13 15:52:40, hub wrote: > > > > > Why the `!keyword`. I'd just return the proper boolean value here. > > > > > > > > Do you mean `keyword != ""`? > > > > > > no. just `return true` > > > > Well it should be false if the keyword is an empty string. > > So, returning !keyword here would return true in the case where keyword is an > empty string, and false if it is not empty. Is that correct? Sorry, I got it backwards. It should return true if it's empty. So it should be `return !keyword` or `return keyword == ""`. > perhaps based > solely on the discussion here it makes sense to return a bool for readability? I > do see that that would add an additional step. Personally, I find that this > approach makes sense and fits the previous convention (although that is not an > argument one way or another just a fact) We can't return either `true` or `false` ... unless you're suggesting `return keyword == "" ? true : false` (which doesn't seem more readable to me, YMMV).
On 2018/09/17 23:36:35, Manish Jethani wrote: > https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js > File lib/matcher.js (right): > > https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js#newc... > lib/matcher.js:351: return !keyword; > On 2018/09/17 19:13:18, Jon Sonesen wrote: > > On 2018/09/14 13:36:25, Manish Jethani wrote: > > > On 2018/09/14 12:20:49, hub wrote: > > > > On 2018/09/14 05:28:32, Manish Jethani wrote: > > > > > On 2018/09/13 15:52:40, hub wrote: > > > > > > Why the `!keyword`. I'd just return the proper boolean value here. > > > > > > > > > > Do you mean `keyword != ""`? > > > > > > > > no. just `return true` > > > > > > Well it should be false if the keyword is an empty string. > > > > So, returning !keyword here would return true in the case where keyword is an > > empty string, and false if it is not empty. Is that correct? > > Sorry, I got it backwards. It should return true if it's empty. So it should be > `return !keyword` or `return keyword == ""`. > > > perhaps based > > solely on the discussion here it makes sense to return a bool for readability? > I > > do see that that would add an additional step. Personally, I find that this > > approach makes sense and fits the previous convention (although that is not an > > argument one way or another just a fact) > > We can't return either `true` or `false` ... unless you're suggesting `return > keyword == "" ? true : false` (which doesn't seem more readable to me, YMMV). Sorry I'm not being clear I was saying returning !keyword is preferable ;p
https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29877558/diff/29877559/lib/matcher.js#newc... lib/matcher.js:351: return !keyword; On 2018/09/17 23:36:35, Manish Jethani wrote: > On 2018/09/17 19:13:18, Jon Sonesen wrote: > > On 2018/09/14 13:36:25, Manish Jethani wrote: > > > On 2018/09/14 12:20:49, hub wrote: > > > > On 2018/09/14 05:28:32, Manish Jethani wrote: > > > > > On 2018/09/13 15:52:40, hub wrote: > > > > > > Why the `!keyword`. I'd just return the proper boolean value here. > > > > > > > > > > Do you mean `keyword != ""`? > > > > > > > > no. just `return true` > > > > > > Well it should be false if the keyword is an empty string. > > > > So, returning !keyword here would return true in the case where keyword is an > > empty string, and false if it is not empty. Is that correct? > > Sorry, I got it backwards. It should return true if it's empty. So it should be > `return !keyword` or `return keyword == ""`. > > > perhaps based > > solely on the discussion here it makes sense to return a bool for readability? > I > > do see that that would add an additional step. Personally, I find that this > > approach makes sense and fits the previous convention (although that is not an > > argument one way or another just a fact) > > We can't return either `true` or `false` ... unless you're suggesting `return > keyword == "" ? true : false` (which doesn't seem more readable to me, YMMV). I see. Works for me as it is.
Patch Set 2: Use string comparison OK, using string comparison now. It shouldn't make a difference to performance since it's an empty string, but makes the log clearer.
https://codereview.adblockplus.org/29877558/diff/29884560/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29877558/diff/29884560/lib/matcher.js#newc... lib/matcher.js:351: return keyword == ""; Well hold on, if we change this here to `keyword == ""` then we should also change the condition in the following line of code, because it too never returns null, just an empty string. I'm iffy about this. I think `!keyword` is fine. Do we agree on this?
On 2018/09/18 14:10:26, Manish Jethani wrote: > https://codereview.adblockplus.org/29877558/diff/29884560/lib/matcher.js > File lib/matcher.js (right): > > https://codereview.adblockplus.org/29877558/diff/29884560/lib/matcher.js#newc... > lib/matcher.js:351: return keyword == ""; > Well hold on, if we change this here to `keyword == ""` then we should also > change the condition in the following line of code, because it too never returns > null, just an empty string. > > I'm iffy about this. I think `!keyword` is fine. Do we agree on this? Yeah, I would say LGTM
On 2018/09/18 14:10:26, Manish Jethani wrote: > https://codereview.adblockplus.org/29877558/diff/29884560/lib/matcher.js > File lib/matcher.js (right): > > https://codereview.adblockplus.org/29877558/diff/29884560/lib/matcher.js#newc... > lib/matcher.js:351: return keyword == ""; > Well hold on, if we change this here to `keyword == ""` then we should also > change the condition in the following line of code, because it too never returns > null, just an empty string. > > I'm iffy about this. I think `!keyword` is fine. Do we agree on this? Yeah, I would say LGTM
On 2018/09/19 16:12:21, Jon Sonesen wrote: > On 2018/09/18 14:10:26, Manish Jethani wrote: > > https://codereview.adblockplus.org/29877558/diff/29884560/lib/matcher.js > > File lib/matcher.js (right): > > > > > https://codereview.adblockplus.org/29877558/diff/29884560/lib/matcher.js#newc... > > lib/matcher.js:351: return keyword == ""; > > Well hold on, if we change this here to `keyword == ""` then we should also > > change the condition in the following line of code, because it too never > returns > > null, just an empty string. > > > > I'm iffy about this. I think `!keyword` is fine. Do we agree on this? > > Yeah, I would say LGTM Sorry, !keyword LGTM so PS1
Jon, can you give a quick LGTM for formality sake? I've reverted to PS1.
On 2018/09/19 18:02:23, Manish Jethani wrote: > Jon, can you give a quick LGTM for formality sake? I've reverted to PS1. Sure! LGTM 
|
