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

Cleanup some nits (gcc -Wextra) #571

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

Dragorn421
Copy link
Contributor

No description provided.

@@ -96,7 +96,6 @@ void display_init( resolution_t res, bitdepth_t bit, uint32_t num_buffers, gamma
/* Can't have the video interrupt happening here */
disable_interrupts();

/* Minimum is two buffers. */
Copy link
Contributor

Choose a reason for hiding this comment

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

I think updating the comment is preferable to removing it. While code obviously takes precedence over comments in terms of information value, a comment as a snapshot in time serves as a possible reminder of any assumptions, motivations, or limitations that were held at the time the comment was written. Someone looking at this code 5 years from now might struggle to imagine why the code is doing what it's doing if there is no comment attached.

Copy link
Collaborator

@rasky rasky left a comment

Choose a reason for hiding this comment

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

Thanks! If you want, I can take care of the requested changes, just let me know

{
assert(idx >= 0 && idx < symt->addrtab_size);
assert(idx < symt->addrtab_size);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a specific rationale for this int/unsigned changes? In general I prefer to use signed numbers because they have less suprising behaviors in case of bugs, I've had quite a few issues with unsigned having surprising behaviors.

min -= 1;
entry = symt_addrtab_entry(symt, min);
}
if (idx != NULL) *idx = min;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also what's the rationale of this? Is there some GCC -Wextra warning on this?

@@ -509,7 +514,7 @@ static void backtrace_foreach(void (*cb)(void *arg, void *ptr), void *arg)
return;
}
}
// FALLTHROUGH!
__attribute__((fallthrough));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought this was matched by GCC, but I guess if you changed it, it wasn't?

I think it should match this regex:

[ \t.!]*(ELSE,? |INTENTIONAL(LY)? )?
FALL(S | |-)?THR(OUGH|U)[ \t.!]*(-[^\n\r]*)?```

which is one of the various used by `-Wimplicit-fallthrough`

@@ -470,7 +470,7 @@ void graphics_draw_character( surface_t* disp, int x, int y, char ch )

for( int yp = sy; yp < ey; yp++ )
{
const register int run = yp * sprite_font.sprite->width;
register const int run = yp * sprite_font.sprite->width;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you're touching this, just drop register

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

3 participants