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 style in toxav.c. #1084

Merged
merged 1 commit into from
Aug 15, 2018
Merged

Fix style in toxav.c. #1084

merged 1 commit into from
Aug 15, 2018

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Aug 12, 2018

  • Use Camel_Snake_Case for type names.
  • Use at least 4 characters for constant names. I.e. END is a type
    name, but RETURN is a constant name. This is because DHT is a type
    name (yay consistency).
  • Using min_* functions instead of MIN, we can avoid a cast.
  • Use for-loops for for-each-frame semantics instead of while.
  • Don't use assignments as expressions.
  • ++i instead of i++.
  • Function pointers are dereferenced automatically, so no need to
    manually do so.
  • Avoid void pointers that lie about not being spaghetti code. Toxcore
    and toxav are both spaghetti and shouldn't pretend anything else.
  • Don't use empty statements (e.g. no ;; anywhere in the code).

This change is Reviewable

@iphydf iphydf added this to the v0.2.x milestone Aug 12, 2018
@iphydf iphydf force-pushed the toxav-style branch 3 times, most recently from 59ba77e to ea55a79 Compare August 12, 2018 23:34
Copy link

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @iphydf)


toxav/toxav.h, line 748 at r1 (raw file):

 */
int toxav_add_av_groupchat(Tox *tox,
                           void (*audio_callback)(void *, uint32_t, uint32_t, const int16_t *, unsigned int, uint8_t, uint32_t, void *),

make audio_callback a type?


toxav/toxav.c, line 188 at r1 (raw file):

    msi_register_callback(av->msi, callback_capabilites, MSI_ON_CAPABILITIES);

RETURN:

maybe call this one FAIL or EXIT? `return has a very specific meaning in C, so I think we should avoid that.

Copy link
Member Author

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf)


toxav/toxav.h, line 748 at r1 (raw file):

Previously, sudden6 wrote…

make audio_callback a type?

It is a type in groupav.h, but I don't want to expose it in the public api, because it's a non-sensical type. It says tox is a void pointer, when clearly it should be a Tox pointer. We can't fix this right now, so I'll fix it in 0.3.0. I've added a todo above.


toxav/toxav.c, line 188 at r1 (raw file):

Previously, sudden6 wrote…

maybe call this one FAIL or EXIT? `return has a very specific meaning in C, so I think we should avoid that.

I considered FAIL, but this is all the returns, not just the error returns. "exit" also has a specific meaning, namely to terminate the program. This is in fact jumping to the return statement, and since we're C, not C++, we have to actually spell out all the destructors that would normally be inserted at a real "return" statement.

Thoughts?

@iphydf iphydf force-pushed the toxav-style branch 2 times, most recently from b270f2a to e6beaf9 Compare August 13, 2018 22:43
* Use Camel_Snake_Case for type names.
* Use at least 4 characters for constant names. I.e. `END` is a type
  name, but `RETURN` is a constant name. This is because `DHT` is a type
  name (yay consistency).
* Using `min_*` functions instead of MIN, we can avoid a cast.
* Use `for`-loops for for-each-frame semantics instead of `while`.
* Don't use assignments as expressions.
* `++i` instead of `i++`.
* Function pointers are dereferenced automatically, so no need to
  manually do so.
* Avoid void pointers that lie about not being spaghetti code. Toxcore
  and toxav are both spaghetti and shouldn't pretend anything else.
* Don't use empty statements (e.g. no `;;` anywhere in the code).
Copy link

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r2.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @sudden6)


toxav/toxav.c, line 188 at r1 (raw file):

Previously, iphydf wrote…

I considered FAIL, but this is all the returns, not just the error returns. "exit" also has a specific meaning, namely to terminate the program. This is in fact jumping to the return statement, and since we're C, not C++, we have to actually spell out all the destructors that would normally be inserted at a real "return" statement.

Thoughts?

I have also seen CLEANUP being used for this, it would not clash with any C specific key words AFAIK, that we have to call all destructors ourself is unfortunate, I know...

Feel free to choose your preferred label, there probably isn't a single best solution.

Copy link

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewed 1 of 3 files at r2.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf)

@iphydf iphydf merged commit 475f01b into TokTok:master Aug 15, 2018
@iphydf iphydf deleted the toxav-style branch August 15, 2018 22:49
@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.6 Aug 16, 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.

3 participants