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

Fix a bunch of compiler warnings and remove suppressions. #808

Merged
merged 1 commit into from
Feb 24, 2018

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Feb 23, 2018

This change is Reviewable

@iphydf iphydf added this to the v0.2.x milestone Feb 23, 2018
@iphydf iphydf force-pushed the fix-warnings branch 6 times, most recently from 593b04d to ec97ddd Compare February 23, 2018 02:50
@sudden6
Copy link

sudden6 commented Feb 23, 2018

Reviewed 47 of 50 files at r1.
Review status: 47 of 50 files reviewed at latest revision, 2 unresolved discussions.


toxcore/crypto_core.c, line 222 at r1 (raw file):

static uint32_t host_to_network(uint32_t x)
{
#if !defined(BYTE_ORDER) || BYTE_ORDER == LITTLE_ENDIAN

I don't think it's a good idea to silently assume a byte order., #error when BYTE_ORDER is undefined would be better.


toxcore/DHT.c, line 1087 at r1 (raw file):

    }

    {

why the extra block? I thought C99 supports variable declarations everywhere?


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Feb 23, 2018

Review status: 47 of 50 files reviewed at latest revision, 2 unresolved discussions.


toxcore/crypto_core.c, line 222 at r1 (raw file):

Previously, sudden6 wrote…

I don't think it's a good idea to silently assume a byte order., #error when BYTE_ORDER is undefined would be better.

I agree, but that's what we do today. BYTE_ORDER is in fact not defined. This code will go away soon anyway, so I'd rather not spend much time on it.


toxcore/DHT.c, line 1087 at r1 (raw file):

Previously, sudden6 wrote…

why the extra block? I thought C99 supports variable declarations everywhere?

  1. index is redefined (-Wshadow) below.
  2. The block reduces the scope of these variables, helping readability. The block could probably be its own function, but I'm avoiding moving refactorings while we have a large change pending merge (ingvar's ngc changes).

Comments from Reviewable

@sudden6
Copy link

sudden6 commented Feb 23, 2018

:lgtm_strong:


Reviewed 3 of 50 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


toxcore/crypto_core.c, line 222 at r1 (raw file):

Previously, iphydf wrote…

I agree, but that's what we do today. BYTE_ORDER is in fact not defined. This code will go away soon anyway, so I'd rather not spend much time on it.

ok, hope it really goes away soon.


toxcore/DHT.c, line 1087 at r1 (raw file):

Previously, iphydf wrote…
  1. index is redefined (-Wshadow) below.
  2. The block reduces the scope of these variables, helping readability. The block could probably be its own function, but I'm avoiding moving refactorings while we have a large change pending merge (ingvar's ngc changes).

ok, didn't see the shadowing


Comments from Reviewable

@iphydf iphydf force-pushed the fix-warnings branch 3 times, most recently from b97f472 to fae2938 Compare February 24, 2018 22:15
@iphydf iphydf merged commit d3b286c into TokTok:master Feb 24, 2018
@iphydf iphydf deleted the fix-warnings branch February 24, 2018 22:24
@iphydf iphydf modified the milestones: v0.2.x, v0.2.0 Feb 24, 2018
This pull request was closed.
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.

2 participants