Skip to content

I think this is a faster commentRegExp#1538

Closed
lsa2127291 wants to merge 1 commit into
requirejs:masterfrom
lsa2127291:lsa2127291
Closed

I think this is a faster commentRegExp#1538
lsa2127291 wants to merge 1 commit into
requirejs:masterfrom
lsa2127291:lsa2127291

Conversation

@lsa2127291

@lsa2127291 lsa2127291 commented May 9, 2016

Copy link
Copy Markdown

Through the test,I think this is a faster way to clear comment.

var Benchmark=require('benchmark');
var suite=new Benchmark.Suite;
var commentRegExp = /(\/\*([\s\S]*?)\*\/|([^:]|^)\/\/(.*)$)/mg;
var optCommentRegExp=/\/\*[\s\S]*?\*\/|([^:]|^)\/\/.*$/mg;
var testFunction=function(require){
    /**test requirejs**/
    var a=require("a.js");
    a.doSometing();//todo someTings
    //...
};
//console.log('opt', testFunction.toString().replace(optCommentRegExp,optCommentReplace));
//console.log('normal', testFunction.toString().replace(commentRegExp,commentReplace));
function optCommentReplace(match,singlePrefix) {
    return singlePrefix || '';
}
function commentReplace(match, multi, multiText, singlePrefix) {
    return singlePrefix || '';
}
suite.add('CommentRegExp#test',function(){
    testFunction.toString().replace(commentRegExp,commentReplace);
}).add('OptCommentRegExp#test',function(){
    testFunction.toString().replace(optCommentRegExp,optCommentReplace);

}).on('cycle', function(event) {
        console.log(String(event.target));
    })
    .on('complete', function() {
        console.log('Fastest is ' + this.filter('fastest').map('name'));
    }).run({'async':true});

@jrburke jrburke added this to the 2.2.1 milestone May 10, 2016
@jrburke

jrburke commented May 10, 2016

Copy link
Copy Markdown
Member

Summary of the change, for my future reference, just remove the parens from the regexp.

Looks like the style checking failed, you can run it locally after doing an npm install by running jscs . && jshint require.js. It should be enough to remove the the commented out old regexp (the diff shows the previous form), and to put spaces around the = sign. The comment at the end of the line is not needed either.

I will put this in consideration for 2.2.1, but want to evaluate it more. It may take a couple weeks for me to conclude it. What would help is any info on if the parens removal affects the kinds of strings that match.

@jrburke

jrburke commented Sep 4, 2016

Copy link
Copy Markdown
Member

This is addressed in #1582.

@jrburke jrburke closed this Sep 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants