Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29790621: Noissue - Delete properties instead of setting to null (Closed)

Created:
May 25, 2018, 1:48 p.m. by Manish Jethani
Modified:
May 25, 2018, 10:22 p.m.
Reviewers:
kzar
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

Noissue - Delete properties instead of setting to null

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -5 lines) Patch
M lib/filterClasses.js View 2 chunks +2 lines, -5 lines 0 comments Download

Messages

Total messages: 4
Manish Jethani
May 25, 2018, 1:48 p.m. (2018-05-25 13:48:31 UTC) #1
Manish Jethani
Patch Set 1 What we did in https://hg.adblockplus.org/adblockpluscore/rev/9194ac6bd473
May 25, 2018, 1:49 p.m. (2018-05-25 13:49:11 UTC) #2
kzar
NOT LGTM, this change seems pointless and if you're trying to make the code more ...
May 25, 2018, 1:58 p.m. (2018-05-25 13:58:50 UTC) #3
Manish Jethani
May 25, 2018, 10:21 p.m. (2018-05-25 22:21:49 UTC) #4
On 2018/05/25 13:58:50, kzar wrote:
> NOT LGTM, this change seems pointless and if you're trying to make the code
more
> consistent you'd be better off adjusting your previous change to just assign
> null instead.

Well it wasn't supposed to be pointless, I thought deleting the property would
improve performance and memory usage. It turns out I couldn't be more wrong. I
did some tests and it seems the opposite is true.

Here's the alternative change then:

https://codereview.adblockplus.org/29790626

Powered by Google App Engine
This is Rietveld