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

Make tox.c unambiguously parseable. #968

Merged
merged 1 commit into from
Jul 5, 2018
Merged

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jul 5, 2018

Rules:

  1. Constants are uppercase names: THE_CONSTANT.
  2. SUE[1] types start with an uppercase letter and have at least one
    lowercase letter in it: The_Type, THE_Type.
  3. Function types end in "_cb": tox_friend_connection_cb.
  4. Variable and function names are all lowercase: the_function.

This makes it easier for humans reading the code to determine what an
identifier means. I'm not convinced by the enum type name change, but I
don't know a better rule. Currently, a lot of enum types are spelled like
constants, which is confusing.

[1] struct/union/enum


This change is Reviewable

@iphydf iphydf added this to the v0.2.x milestone Jul 5, 2018
@iphydf iphydf force-pushed the parseable branch 2 times, most recently from c0a9ca6 to 972cc97 Compare July 5, 2018 14:31
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.

For the enum types, what do you want to express with making them different than constants? Because as far as I know when compiling they are constant then.

Reviewed 19 of 21 files at r1.
Reviewable status: 0 of 1 LGTMs obtained


toxcore/Messenger.h, line 139 at r1 (raw file):

    USERSTATUS_BUSY,
    USERSTATUS_INVALID
} Userstatus;

somehow my eyes expect UserStatus here, change?

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.

The enum values should be constants, but the enum types aren't constants. They are types, and should be spelled like types.

Reviewable status: 0 of 1 LGTMs obtained


toxcore/Messenger.h, line 139 at r1 (raw file):

Previously, sudden6 wrote…

somehow my eyes expect UserStatus here, change?

User_Status, since that's the naming standard in toxcore (I think it's ew, but oh well).

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.

ah, right, then it's fine for me :lgtm_strong:

Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained


toxcore/Messenger.h, line 139 at r1 (raw file):

Previously, iphydf wrote…

User_Status, since that's the naming standard in toxcore (I think it's ew, but oh well).

ok, User_Status looks slightly better...

@iphydf iphydf force-pushed the parseable branch 2 times, most recently from e9ec113 to ba77a3e Compare July 5, 2018 22:42
Rules:
1. Constants are uppercase names: THE_CONSTANT.
2. SUE[1] types start with an uppercase letter and have at least one
   lowercase letter in it: The_Type, THE_Type.
3. Function types end in "_cb": tox_friend_connection_cb.
4. Variable and function names are all lowercase: the_function.

This makes it easier for humans reading the code to determine what an
identifier means. I'm not convinced by the enum type name change, but I
don't know a better rule. Currently, a lot of enum types are spelled like
constants, which is confusing.

[1] struct/union/enum
@iphydf iphydf merged commit 8739f7f into TokTok:master Jul 5, 2018
@iphydf iphydf deleted the parseable branch July 5, 2018 23:25
@iphydf iphydf modified the milestones: v0.2.x, v0.2.4 Jul 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants