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

code style #103

Merged
merged 5 commits into from
Jul 12, 2023
Merged

code style #103

merged 5 commits into from
Jul 12, 2023

Conversation

Hojun-Cho
Copy link
Contributor

I found other things that needed fixing, so I recreated the PR.
modified: 'if(' => 'if ('. Is okay?

'if(' => 'if (':
	modified:   containers/qvector.c
	modified:   internal/qinternal.h
	modified:   utilities/qhash.c
@Hojun-Cho Hojun-Cho mentioned this pull request Jul 10, 2023
@@ -41,15 +41,15 @@
size_t _strsize; \
for(_strsize = 1024; ; _strsize *= 2) { \
Copy link
Owner

Choose a reason for hiding this comment

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

do we want to style this the same way?

@@ -41,15 +41,15 @@
size_t _strsize; \
for(_strsize = 1024; ; _strsize *= 2) { \
s = (char*)malloc(_strsize); \
if(s == NULL) { \
if (s == NULL) { \
Copy link
Owner

Choose a reason for hiding this comment

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

we want the tailing \ lineup with other lines

@wolkykim
Copy link
Owner

thanks for the PR. I left a few comments. Have you checked other files? Are those the only files needing style adjustment?

@wolkykim wolkykim closed this Jul 10, 2023
@wolkykim wolkykim reopened this Jul 10, 2023
@wolkykim
Copy link
Owner

sorry clicked the wrong button. reopened.

@Hojun-Cho
Copy link
Contributor Author

thanks for the PR. I left a few comments. Have you checked other files? Are those the only files needing style adjustment?

Maybe we should style the comments too? If not, I think we're done.

@@ -39,17 +39,17 @@

#define DYNAMIC_VSPRINTF(s, f) do { \
size_t _strsize; \
for(_strsize = 1024; ; _strsize *= 2) { \
for (_strsize = 1024; ; _strsize *= 2) { \
Copy link
Owner

Choose a reason for hiding this comment

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

the tailing \ are off lining up with others.

DEBUG("DYNAMIC_VSPRINTF(): can't allocate memory."); \
break; \
} \
va_list _arglist; \
va_start(_arglist, f); \
int _n = vsnprintf(s, _strsize, f, _arglist); \
va_end(_arglist); \
if(_n >= 0 && _n < _strsize) break; \
if (_n >= 0 && _n < _strsize) break; \
Copy link
Owner

Choose a reason for hiding this comment

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

the tailing \ are off lining up with others here too. This is happening for many of the updates. Please check them again. Maybe easier to see it on the diff UI https://github.com/wolkykim/qlibc/pull/103/files

@Hojun-Cho
Copy link
Contributor Author

Fixed. Thanks for letting me know. Are the changes correct?

@Hojun-Cho Hojun-Cho requested a review from wolkykim July 12, 2023 05:47
Copy link
Owner

@wolkykim wolkykim left a comment

Choose a reason for hiding this comment

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

What's your plan for other files? Are you going to work on applying this convention to all the other code and header files? Or partially only to those files included in this PR?

Comment on lines 75 to 90
#define Q_MUTEX_NEW(m,r) do { \
qmutex_t *x = (qmutex_t *)calloc(1, sizeof(qmutex_t)); \
pthread_mutexattr_t _mutexattr; \
pthread_mutexattr_init(&_mutexattr); \
if (r == true) { \
pthread_mutexattr_settype(&_mutexattr, PTHREAD_MUTEX_RECURSIVE); \
} \
int _ret = pthread_mutex_init(&(x->mutex), &_mutexattr); \
pthread_mutexattr_destroy(&_mutexattr); \
if(_ret == 0) { \
m = x; \
} else { \
DEBUG("Q_MUTEX: can't initialize mutex. [%d]", _ret); \
free(x); \
m = NULL; \
} \
} \
int _ret = pthread_mutex_init(&(x->mutex), &_mutexattr); \
pthread_mutexattr_destroy(&_mutexattr); \
if (_ret == 0) { \
m = x; \
} else { \
DEBUG("Q_MUTEX: can't initialize mutex. [%d]", _ret); \
free(x); \
m = NULL; \
} \
Copy link
Owner

Choose a reason for hiding this comment

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

You only need to update L79 and L84.

@Hojun-Cho
Copy link
Contributor Author

Hojun-Cho commented Jul 12, 2023

I created the PR because I enjoy looking at qlibc code. I don't plan to apply this rule to any other files (though I will if you need to). Am I messing with qlibc code?

@Hojun-Cho Hojun-Cho requested a review from wolkykim July 12, 2023 07:05
@wolkykim
Copy link
Owner

That's nice. And no, you're not messing with the code but making it better. I appreciate your work. Yeah, making everything consistent would be nice but it's a large work.

@wolkykim wolkykim merged commit 2f43e41 into wolkykim:master Jul 12, 2023
1 check passed
@wolkykim
Copy link
Owner

This is great. Just merged in. I'm looking forward to seeing more PRs like this. Thank you.
Also don't forget to submit a PR for adding your name to the contributor list https://github.com/wolkykim/qlibc/blob/2e46b2f54124bdcb7d5e5ede4973d83e8a6d9227/README.md

@Hojun-Cho
Copy link
Contributor Author

Thank you for creating such a great library ^%^.

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

2 participants