Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete unused Jason.Codegen.jump_table_case/4 #108

Merged
merged 1 commit into from
Apr 3, 2020

Conversation

QuinnWilton
Copy link
Contributor

While reading through the code I noticed that this macro was unused.

@michalmuskala michalmuskala merged commit 91a4eaf into michalmuskala:master Apr 3, 2020
@michalmuskala
Copy link
Owner

Thank you! ❤️

@QuinnWilton
Copy link
Contributor Author

QuinnWilton commented Apr 3, 2020

Edit: I actually just went and made this change, and it seems like the tests do in fact fail, so the code is needed. Sorry for the false alarm :) I hadn't yet gone through the encoding logic, and didn't realize Codegen.jump_table/2 was called directly.

Thank you for the quick merge! While tracing through the code last night, I noticed a few other bits of code that don't seem to be used, but I was hesitant to delete them in case you weren't interested in that kind of cleanup PR.

For example, as far as I can tell, this clause is never hit, since ranges are only defined using charlists: https://github.com/michalmuskala/jason/blob/master/lib/codegen.ex#L96

Would it be helpful if I also got rid of code like that too?

@michalmuskala
Copy link
Owner

michalmuskala commented Apr 3, 2020

I believe it's used in here - it generates a list of single-integer elements

slash_escapes = Enum.zip('\b\t\n\f\r\"\\', 'btnfr"\\')

But yeah, if there's any unused code, it should definitely go.

@QuinnWilton
Copy link
Contributor Author

Yup, you're right. I missed the direct calls to Codegen.jump_table/2. Will do, thanks for getting back to me!

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.

None yet

2 participants