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.

Sign up for GitHub

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

Jump to bottom

Avoiding conditional directives that split up parts of statements. #1480

Open

marciomribeiro wants to merge 9 commits into irungentoo:master from marciomribeiro:master

marciomribeiro commented Dec 4, 2015

A suggestion to compile entire statements and expressions, as suggested by code style guidelines from the Linux Kernel and practitioners.

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle#n892
https://www.cqse.eu/en/blog/living-in-the-ifdef-hell/

It might improve code understanding, maintainability and error-proneness.

marciomribeiro added 3 commits

December 4, 2015 10:20


          Avoiding conditional directives that split up parts of statements.

30fa7c7


          Avoiding conditional directives that split up parts of statements.

dc5881d


          Avoiding conditional directives that split up parts of statements.

68ca982

GrayHatter self-assigned this

GrayHatter added PR: pending review PR: changes requested labels

Collaborator

GrayHatter commented Jan 2, 2016

@marciomribeiro can you indent the #ifdefs as well?

GrayHatter mentioned this pull request

Cleaning up #ifdefs that break statements #1483

Closed

Author

marciomribeiro commented Mar 8, 2016

I've attached 2 files. Do you prefer Option1 or Option2?

Option 1:

Option 2:

Collaborator

GrayHatter commented Mar 8, 2016

I like option 2

marciomribeiro added 3 commits

March 8, 2016 15:11


          Indenting #ifdefs (as requested by @GrayHatter)

4dcfa8b


          Indenting #ifdefs (as requested by @GrayHatter)

4e2c2df


          Indenting #ifdefs (as requested by @GrayHatter)

e4c075f

Author

marciomribeiro commented Mar 8, 2016

Don't know why the CI build has failed. I didn't change anything but the indentation...

Collaborator

GrayHatter commented Mar 8, 2016

that's normal, the tests are broken

On Tue, Mar 8, 2016 at 11:24 AM, Márcio Ribeiro notifications@github.com
wrote:

Don't know why the CI build has failed. I didn't change anything but the
indentation...

—
Reply to this email directly or view it on GitHub
#1480 (comment).

Author

marciomribeiro commented Mar 8, 2016

Is the pull request now ready to be accepted?!

Collaborator

GrayHatter commented Mar 8, 2016

once I or someone else can review it yeah! thanks!

GrayHatter removed their assignment

Collaborator

GrayHatter commented Jul 9, 2016

@marciomribeiro you may want to reopen this on https://github.com/TokTok/toxcore

Contributor

iphydf commented Sep 22, 2016

Is this going to be merged or continued?

iphydf suggested changes

View reviewed changes

toxencryptsave/crypto_pwhash_scryptsalsa208sha256/nosse/pwhash_scryptsalsa208sha256_nosse.c

@@ @@ -239,12 +239,12 @@ escrypt_kdf_nosse(escrypt_local_t * local, @@
               	uint32_t i;
               	/* Sanity-check parameters. */
-              #if SIZE_MAX > UINT32_MAX
+              	#if SIZE_MAX > UINT32_MAX

Contributor

iphydf Sep 29, 2016

@marciomribeiro please undo this formatting change.

toxencryptsave/crypto_pwhash_scryptsalsa208sha256/nosse/pwhash_scryptsalsa208sha256_nosse.c

               	if (buflen > (((uint64_t)(1) << 32) - 1) * 32) {
               		errno = EFBIG;
               		return -1;
               	}
-              #endif
+              	#endif

Contributor

iphydf Sep 29, 2016

And undo this.

toxencryptsave/crypto_pwhash_scryptsalsa208sha256/nosse/pwhash_scryptsalsa208sha256_nosse.c

-              #endif
-              	    (N > SIZE_MAX / 128 / r)) {
+              	int test = (r > SIZE_MAX / 128 / p) || (N > SIZE_MAX / 128 / r);
+              	#if SIZE_MAX / 256 <= UINT32_MAX

Contributor

iphydf Sep 29, 2016

Move to column 0.

toxencryptsave/crypto_pwhash_scryptsalsa208sha256/nosse/pwhash_scryptsalsa208sha256_nosse.c

-              	    (r > SIZE_MAX / 256) ||
-              #endif
-              	    (N > SIZE_MAX / 128 / r)) {
+              	int test = (r > SIZE_MAX / 128 / p) || (N > SIZE_MAX / 128 / r);

Contributor

iphydf Sep 29, 2016

Use _Bool. Also rename to something more useful. test doesn't say anything. What is it testing?

toxencryptsave/crypto_pwhash_scryptsalsa208sha256/nosse/pwhash_scryptsalsa208sha256_nosse.c

+              	int test = (r > SIZE_MAX / 128 / p) || (N > SIZE_MAX / 128 / r);
+              	#if SIZE_MAX / 256 <= UINT32_MAX
+              	test = test || (r > SIZE_MAX / 256);
+              	#endif

Contributor

iphydf Sep 29, 2016

Move to column 0.

toxencryptsave/crypto_pwhash_scryptsalsa208sha256/scrypt_platform.c

-              	    MAP_ANON | MAP_PRIVATE,
-              #endif
-              	    -1, 0)) == MAP_FAILED)
+              	#ifdef MAP_ANON

Contributor

iphydf Sep 29, 2016

Move all of these back to column 0.

toxencryptsave/crypto_pwhash_scryptsalsa208sha256/sse/pwhash_scryptsalsa208sha256_sse.c

               	uint32_t i;
               	/* Sanity-check parameters. */
-              #if SIZE_MAX > UINT32_MAX
+              	#if SIZE_MAX > UINT32_MAX

Contributor

iphydf Sep 29, 2016

Same thing in this file: revert the style change.

marciomribeiro added 3 commits

January 12, 2017 15:51


          Update pwhash_scryptsalsa208sha256_nosse.c

10ad117


          Update scrypt_platform.c

2bc8074


          Update pwhash_scryptsalsa208sha256_sse.c

b094268

Author

marciomribeiro commented Jan 12, 2017

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: changes requested PR: pending review