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

Improve static and const correctness. #97

Merged
merged 1 commit into from
Sep 7, 2016
Merged

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Sep 5, 2016

  • Any non-externally-visible declarations should be static.
  • Casting away the const qualifier from pointers-to-const is
    dangerous. All but one instance of this are now correct. The one
    instance where we can't keep const is one where toxav code actually
    writes to a chunk of memory marked as const. This code also assumes
    4 byte alignment of data packets. I don't know whether that is a valid
    assumption, but it's likely unportable, and not obviously correct.
  • Replaced empty parameter lists with (void) to avoid passing
    parameters to it. Empty parameter lists are old style declarations for
    unknown number and type of arguments.
  • Commented out (as #if DHT_HARDENING block) the hardening code that
    was never executed.
  • Minor style fix: don't use default in enum-switches unless the number
    of enumerators in the default case is very large. In this case, it was
    2, so we want to list them both explicitly to be warned about missing
    one if we add one in the future.
  • Removed the only two function declarations from nTox.h and put them
    into nTox.c. They are not used outside and nTox is not a library.

This change is Reviewable

@nurupo
Copy link
Member

nurupo commented Sep 6, 2016

Reviewed 38 of 38 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


toxav/msi.c, line 685 [r1] (raw file):

        break;

        case msi_CallRequested:

// intentional fall-through


Comments from Reviewable

@GrayHatter
Copy link

Reviewed 36 of 38 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


testing/DHT_test.c, line 157 [r1] (raw file):

#if 0
static void printpacket(uint8_t *data, uint32_t length, IP_Port ip_port)

wouldn't this be better in an #if NDEBUG block?


toxav/msi.c, line 685 [r1] (raw file):

Previously, nurupo wrote…

// intentional fall-through

Usually the lack of curly braces means it's an intentional fall through.

Either way is fine with me.


toxcore/Messenger.c, line 59 [r1] (raw file):

 *  return -1 if realloc fails.
 */
static int realloc_friendlist(Messenger *m, uint32_t num)

FYI this change will be reverted in multidevice


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Sep 6, 2016

Review status: 19 of 38 files reviewed at latest revision, 3 unresolved discussions.


testing/DHT_test.c, line 157 [r1] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

wouldn't this be better in an #if NDEBUG block?

It's used in the "slvrTODO" block at the bottom. NDEBUG is inappropriate. I've added an slvrTODO here as well.

toxav/msi.c, line 685 [r1] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Usually the lack of curly braces means it's an intentional fall through.

Either way is fine with me.

In other parts of the code, this is not standard practice. You're welcome to go through all `switch` statements and add comments like that in a separate PR, but in this PR, I'd like to be consistent with the surrounding code.

toxcore/Messenger.c, line 59 [r1] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

FYI this change will be reverted in multidevice

Acknowledged.

Comments from Reviewable

- Any non-externally-visible declarations should be `static`.
- Casting away the `const` qualifier from pointers-to-const is
  dangerous. All but one instance of this are now correct. The one
  instance where we can't keep `const` is one where toxav code actually
  writes to a chunk of memory marked as `const`. This code also assumes
  4 byte alignment of data packets. I don't know whether that is a valid
  assumption, but it's likely unportable, and *not* obviously correct.
- Replaced empty parameter lists with `(void)` to avoid passing
  parameters to it. Empty parameter lists are old style declarations for
  unknown number and type of arguments.
- Commented out (as `#if DHT_HARDENING` block) the hardening code that
  was never executed.
- Minor style fix: don't use `default` in enum-switches unless the number
  of enumerators in the default case is very large. In this case, it was
  2, so we want to list them both explicitly to be warned about missing
  one if we add one in the future.
- Removed the only two function declarations from nTox.h and put them
  into nTox.c. They are not used outside and nTox is not a library.
@GrayHatter
Copy link

:lgtm_strong:


Reviewed 19 of 19 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@JFreegman
Copy link
Member

:lgtm_strong:


Reviewed 19 of 38 files at r1, 19 of 19 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Sep 7, 2016

:lgtm_strong:


Reviewed 19 of 19 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


toxav/msi.c, line 685 [r1] (raw file):
It is a standard practice in tox-boostrapd!

Oh well, too bad it's all or nothing.

Usually the lack of curly braces means it's an intentional fall through.

Use of curly braces is entirely optional though, they are not required for multi-line case. Unless irungentoo always uses them like that, the "no curely braces" = "fall through" is a rather questionable oracle.


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Sep 7, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion.


toxav/msi.c, line 685 [r1] (raw file):

Previously, nurupo wrote…

It is a standard practice in tox-boostrapd!

Oh well, too bad it's all or nothing.

Usually the lack of curly braces means it's an intentional fall through.

Use of curly braces is entirely optional though, they are not required for multi-line case. Unless irungentoo always uses them like that, the "no curely braces" = "fall through" is a rather questionable oracle.

I see. I think an uninterrupted sequence of case statements doesn't need fall-through comments. It's very obvious that it's a fall-through. If there is any statement between two case statements, it definitely requires such a comment.

Comments from Reviewable

@iphydf iphydf unassigned tux3 Sep 7, 2016
@iphydf iphydf closed this Sep 7, 2016
@iphydf iphydf deleted the static-const branch September 7, 2016 08:50
@iphydf iphydf merged commit ad26560 into TokTok:master Sep 7, 2016
@iphydf iphydf modified the milestone: v0.0.1 Nov 6, 2016
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.

5 participants