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

Static analyzer-found cleanup opportunities #155

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

ryandrake08
Copy link

A couple of issues found using clang's static analyzer. Most are pretty trivial (removal of unnecessary / unused code). Two moderately concerning ones:

  • The buffer shader_read() allocates is too large (sizeof char* vs sizeof char)
  • Potential memory leak in texture_font_load_glyph

Feel free to merge if you think it's helpful.

@rougier
Copy link
Owner

rougier commented Feb 11, 2017

Thanks! I've never used the clang static analyzer. I'll look into proposed modification and merge them.

@@ -125,12 +125,10 @@ ansi_to_markup( char *sequence, size_t length, markup_t *markup )
else if( (set_fg == 0) && (code == 5) )
{
set_fg = 1;
code = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line prevents from ever entering the test just after. Do you know how the analyzer decides this can be safely removed ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Observe line 177, code gets assigned 0 outside the if-else statement anyway, directly after line 128 is executed. So line 128 is redundant it seems.

float glyph_height = glyph->height * width/(float)glyph->width;
float glyph_width = glyph->width * height/(float)glyph->height;
int x = -glyph_width/2 + width/2.;
int y = -glyph_height/2 + height/2.;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if not used, this is relevant information. Maybe commenting those line would be better, no ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll push a change with them added back, but commented out!

@ryandrake08
Copy link
Author

Thanks for the feedback. Adjustments made. This library is very useful by the way!

@rougier
Copy link
Owner

rougier commented Jun 9, 2017

@ryandrake08 Thanks for the fix. Just tell me you're ready for a merge.

@ryandrake08
Copy link
Author

Feel free to merge whenever. This PR is turning into a grab bag of warnings fixes and cleanups. The only change that may actually change the functionality of the library is 3df29ce. Please have a look at what this one is doing. You may prefer I leave this out. I found I needed it in my application to improve the look of my rendered text. Without this padding I was seeing undesired graphic artifacts around the edges of each glyph.

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.

3 participants