mangler ignores catch() var #7

Closed
wkornewald opened this Issue Oct 1, 2011 · 7 comments

Projects

None yet

2 participants

@wkornewald

When I try to mangle the following code:

function a() {
  var $exc = null;
  try {
    lala();
  } catch($exc) {
    if ($exc.__name__ == 'hi') {
      return 'bam';
    }
  }
  return 'bum';
}

I get the following result:

function a() {
  var a = null;
  try {
    lala();
  } catch ($exc) {
    if (a.__name__ == 'hi') {
      return 'bam';
    }
  }
  return 'bum';
}

As you can see, the $exc variable in the catch() clause was just ignored.

@rspivak rspivak was assigned Oct 1, 2011
@wkornewald

I've implemented a patch here:
wkornewald/slimit@95afeb8
However, I'm not sure if this is the full solution as I'm already hitting the next bug.

@rspivak
Owner
rspivak commented Oct 1, 2011

Hi Waldemar,

I've done some changes very similar to your patch. Could you try the latest 'develop' branch and let me know if there are any other issues.
This is how to bootstrap the project:

$ git clone git://github.com/rspivak/slimit.git
$ cd slimit
$ python bootstrap.py -d
$ bin/buildout
$ bin/slimit -m < pyjslib.js > pyjslib.min.js
@wkornewald

I think you've pushed those changes to the master branch. Anyway, there is still some bug and I'm trying to hunt it down right now. For some reason there is a conflict between mangled names and currently I can only reproduce it with our whole project's source, but not with a simple snippet.

@rspivak
Owner
rspivak commented Oct 3, 2011

You're right. I put it mistakenly on a master branch. Good luck with the bug hunting and let me know if I can help out in any way.

P.S. I put changes on develop branch and reset master to its previous state.

@wkornewald

I think I've found all bugs. At least, now the whole pyjs-generated code works fine. I've also added a unit test for the other bug and fixed the implementation and the unit test for this issue (see my updated pull request).

@rspivak
Owner
rspivak commented Oct 4, 2011

Thanks!

I'll review the changes tomorrow and if everything's alright I'll merge them

@rspivak
Owner
rspivak commented Oct 6, 2011

Merged. Thanks a lot for the patch.

@rspivak rspivak closed this Oct 6, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment