Testing all leading comments against being PURE comments #2305
Conversation
| && (pure_comment = find_if(function (comment) { | ||
| return /[@#]__PURE__/.test(comment.value); | ||
| }, comments))) { | ||
| pure = pure_comment; |
There was a problem hiding this comment.
@alexlamsl Are you okay with the efficiency of find_if in this scenario? If so, LGTM.
There was a problem hiding this comment.
pure & pure_comment now feels redundant as find_if() will return undefined, e.g. falsy, if no comment has matched.
There was a problem hiding this comment.
pure & pure_comment now feels redundant as find_if() will return undefined, e.g. falsy, if no comment has matched.
It's not redundant. It would screw up the caching of return this.pure = pure;. undefined is not falsey in this scenario - it means "not set". false means no pure annotation found previously and no need to scan again.
There was a problem hiding this comment.
Ah okay - thanks for the clarification 👍
There was a problem hiding this comment.
@alexlamsl I see your point. Could get rid of var pure and just:
There was a problem hiding this comment.
I think this should work:
- return this.pure = pure;
+ return this.pure = pure_comment || false;There was a problem hiding this comment.
its already initialized with false few line above
There was a problem hiding this comment.
Assuming we want to eliminate var pure ...
pure_comment is not initialized to false:
https://github.com/mishoo/UglifyJS2/pull/2305/files#diff-230c53e13e3ee367aec7d49470eb42caR1983
and find_if returns undefined if nothing found.
There was a problem hiding this comment.
Assuming we want to eliminate
var pure...
Or just leave PR as is. It works.
There was a problem hiding this comment.
Yeah, don't sweat too much over my OCD... 😅
test/compress/issue-1261.js
Outdated
| "WARN: Dropping __PURE__ call [test/compress/issue-1261.js:84,33]", | ||
| "WARN: Dropping unused variable iife1 [test/compress/issue-1261.js:84,12]", | ||
| "WARN: Dropping __PURE__ call [test/compress/issue-1261.js:100,45]", | ||
| "WARN: Dropping unused variable MyClass [test/compress/issue-1261.js:100,12]" |
There was a problem hiding this comment.
@kzc wouldn't you prefer the trailing comma here & https://github.com/mishoo/UglifyJS2/pull/2305/files#diff-0de0e29c110ed6d6b57fec7a54c864b4R183 ?
There was a problem hiding this comment.
Yes, I'd prefer trailing commas.
|
@Andarist thanks for the work - LGTM with minor nits. |
test/compress/issue-1261.js
Outdated
| "WARN: Dropping __PURE__ call [test/compress/issue-1261.js:145,24]", | ||
| "WARN: Condition always true [test/compress/issue-1261.js:145,8]", | ||
| "WARN: Dropping __PURE__ call [test/compress/issue-1261.js:146,31]", | ||
| "WARN: Condition always false [test/compress/issue-1261.js:146,8]" |
Andarist
force-pushed
the
pure-comments-any-index
branch
from
fb8b222
to
4236e9d
Compare
|
ive added missing trailing commas |
|
@Andarist thanks again for the patch. I'm going to restart So there should probably be a new release before Monday. |
As discussed under https://github.com/mishoo/UglifyJS2/pull/1448/files#r137827678
Change is pretty straight-forward. It allows i.e. babel plugins to insert PURE comments easier, allowing easier interoperability with other automatically inserted annotations.