-
Notifications
You must be signed in to change notification settings - Fork 463
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
Even less warnings #13
Conversation
- more signed/unsigned - unused variables - suggest braces around assignment
One warning remains and should be taken care of properly. In any case: this patch should be reviewed carefully.
Wow, thanks for the cleanup! I'll review that last patch this evening. |
malloc does not initialize the memory to zero, so sass_context *ctx = sass_new_context() will create a context with a random value in ctx->output_string. If sass_free_context(ctx) was called immediately thereafter, ctx->output_string would have a random value and the result of the free(ctx->output_string) would be undefined. In the worst case, this corrupts the heap and the process dies much much later. Also, free isn't delete and mustn't be called with a NULL pointer.
You're welcome. Do have a look at 8b24df9. That one is really a potential problem. |
Also, may I suggest that the C-API is changed? I'd do something like:
I would omit Also, for the record: Yes, this isn't pretty, even uglier than the current version. But I think that If you are interested, I can work on a patch. |
lars- yeah, I'm up for that change. I like it. Just have to make sure to coordinate this with the SassJS guys' work. Or, were you helping with that too? |
No, I just helped out with the Python wrappers. I'll be offline for a week now, but when I'm back, I'll try to come up with a patch and look at what the SassJS guys are up to. |
Lars, I noticed you said there were no tests... there are! They are just in SassC... which is the I just added a note to the readme about this... 6b8405d |
Two more patches, and then only one warning remains (and that should actually be taken care of properly)
The last patch, titled default statements and "control reaches end of non-void function" should be reviewed carefully. I've tried to do sensible things, but I was a bit on autopilot.