Issue 29800603: Noissue - Remove redundant assignment to zero (Closed)
|
Created:
June 6, 2018, 5:35 p.m. by Manish Jethani Modified:
June 7, 2018, 2:39 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionNoissue - Remove redundant assignment to zero
Patch Set 1 #
Total comments: 4
MessagesTotal messages: 6
Patch Set 1 null and 0 are the same as far as bitwise operations are concerned, so these two lines are redundant.
https://codereview.adblockplus.org/29800603/diff/29800604/lib/filterClasses.js File lib/filterClasses.js (left): https://codereview.adblockplus.org/29800603/diff/29800604/lib/filterClasses.j... lib/filterClasses.js:735: let contentType = null; It's kinda confusing to reason about, but I wonder if we could just assign 0 here and remove the other == null check as well? In other words could contentType ever be 0 at the point we're checking it's not null?
https://codereview.adblockplus.org/29800603/diff/29800604/lib/filterClasses.js File lib/filterClasses.js (left): https://codereview.adblockplus.org/29800603/diff/29800604/lib/filterClasses.j... lib/filterClasses.js:735: let contentType = null; On 2018/06/07 13:34:55, kzar wrote: > It's kinda confusing to reason about, but I wonder if we could just assign 0 > here and remove the other == null check as well? > > In other words could contentType ever be 0 at the point we're checking it's not > null? In theory one could write a filter that specifically disables the filter on every single content type, thus yielding the value 0 in the end. It helps to have null as a special value to indicate that no content type was specified, at which point the filter object can default to the value in the prototype object. So yeah I think we need the null.
LGTM https://codereview.adblockplus.org/29800603/diff/29800604/lib/filterClasses.js File lib/filterClasses.js (left): https://codereview.adblockplus.org/29800603/diff/29800604/lib/filterClasses.j... lib/filterClasses.js:735: let contentType = null; On 2018/06/07 14:23:31, Manish Jethani wrote: > In theory one could write a filter that specifically disables the filter on every single content type, thus yielding the value 0 in the end. Yea, I guess you're right. It seems kind of unlikely that a filter explicitly disables every single content type once, then one again... but if it did I guess the result would be `RegExpFilter.prototype.contentType & ~contentType` instead of `0 & ~contentType` without the null.
On 2018/06/07 14:32:32, kzar wrote: > LGTM Thanks! https://codereview.adblockplus.org/29800603/diff/29800604/lib/filterClasses.js File lib/filterClasses.js (left): https://codereview.adblockplus.org/29800603/diff/29800604/lib/filterClasses.j... lib/filterClasses.js:735: let contentType = null; On 2018/06/07 14:32:32, kzar wrote: > On 2018/06/07 14:23:31, Manish Jethani wrote: > > In theory one could write a filter that specifically disables the filter on > every single content type, thus yielding the value 0 in the end. > > Yea, I guess you're right. It seems kind of unlikely that a filter explicitly > disables every single content type once, then one again... but if it did I guess > the result would be `RegExpFilter.prototype.contentType & ~contentType` instead > of `0 & ~contentType` without the null. If every single type is excluded the final value of contentType would be 0, also the initial value. Then the RegExpFilter constructor would have the following check: if (contentType != 0) this.contentType = contentType; So for 0 it would result in applying the filter to all content types - the opposite of what the filter author intended. With null we avoid this. 
|
||||||||||||||||||||||||||||||||||||||||||
