Working copy for coding style updates #43

richsalz · 2018-02-16T15:21:03Z

This is to have a better place to review coding style changes and comment before the OMC vote.

mattcaswell · 2018-02-16T15:31:45Z

policies/codingstyle-updates.txt

+
+	if (this_is_true()
+		&& !that_is_false()) {
+	    code();


Do we want to say this isn't necessary for an "else if"?

yes! fixed.

mattcaswell · 2018-02-16T15:33:36Z

policies/codingstyle-updates.txt

+	    ...
+
+Try not to break long lines within a function call, but if you have to, indent
+the rest of the parameter list to be after the function's opening paren.


What about cases where the opening paren is very far over to the right? Sometimes to the point that you still couldn't fit params on any line.

Good point, added some text and pushed new commit.

dot-asm · 2018-02-16T15:37:29Z

policies/codingstyle-updates.txt

+	if (test()) {
+	    /* already alerted */
+	    close();
+	}


I wonder what would you think of

if (test()) /* alredy alerted? */ close();

My personal view is that the comment should go on the line above the if, or below it.

/* If already alerted, go away */ if (test()) close();

Just in case for reference, "you" in my remark was plural :-) Otherwise as for placing it [comment] above, it works for single if, but imagine you have say three-term elsif chain...

it's ugly but i added some words.

dot-asm · 2018-02-16T15:41:29Z

policies/codingstyle-updates.txt

+	#define join(a, b) a ## quote(a)
+
+Free routines should properly handle null; don't check for null before calling
+a free routine.


I don't know what's "free routine" and I can't relate this to anything that was discussed earlier. My impression of prior discussion is rather if a pointer value being NULL is not an actually permitted/recognized value, then don't check for it. Is it incorrect impression?

X509_free, OPENSSL_free, etc. They take NULL just like C's free does.

Ah! Then "object freeing subroutines" perhaps? To eliminate ambiguity?

sure, done.

dot-asm · 2018-02-16T16:07:36Z

policies/codingstyle-updates.txt

+When possible, use size_t for sizes of things (including array indicies)
+
+Prefer ossl_assert over assert. Do not use OPENSSL_assert() in libcrypto or
+libssl.


As you might know I'm in favour of using as little lingo as possible. With this in mind I'd argue that ossl_assert and assert are equally appropriate, just serve different purposes. assert has rather documentation purpose, i.e. is to be used to document assumptions/boundary conditions, not unlikely static. ossl_assert on the other hand is meant to be used rather with if(!ossl_assert()) handler(), i.e. when there is something that can be done at run-time [in non-debug build] when assertion doesn't hold. Or in other words, if there is no actual need for ossl_assert I wouldn't be opposed to use of assert. Because it's standard and recognized construct. In other words I'd argue against unconditional ban of assert.

The difference is that ossl_assert returns a value in non-debug environments while assert is void. That makes

if (ossl_assert(...

possible. It's used nearly 100 times in openssl code.

The difference is that ossl_assert returns a value in non-debug environments while assert is void.

This doesn't contradict what I'm saying, rather contrary. They are different and as such have different applications. And in such case you can't really say "prefer one over another"...

I think there could be a tendency to use assert where ossl_assert might be more appropriate. So rather than saying prefer one over the other, perhaps we should explain when you should use each one?

I thought we didn't want crashes, and that if you had an assert, there had to be a runtime test and error return, and that this led to ossl_assert which combines both.

As initially said, I don't see relation between ossl_assert and assert as "instead". In sense that it's not always the case that ossl_assert would be more appropriate. If anything, ossl_assert is rather replacement for normal if (condition) handler, except that it will crash in debug build taking the snapshot of hard-to-reproduce case. But here is question in the context. In the essence we are commenting on to-do list for updates. But what about the actual outcome? Does this to-do item mean that there will be "use of assertions" paragraph? If so, it might be meaningful to discuss that.

I am trying to put down what we think is the right thing to say. If we don't have agreement, we can just remove this item.

There seems at least agreement that we shouldn't use OPENSSL_assert(). There is also agreement that we can use ossl_assert() and assert(). We should say at least that. I do think that we should, in most cases, prefer ossl_assert() over assert() - but it sounds like that will be difficult to agree - so we could just drop that bit of it.

??? I'm not suggesting to remove the item. just don't see place for "ossl_assert instead of assert". Absolutely not if it appears as single sentence in the middle of guide. I rather expect that there will be a paragraph, i.e. multiple sentences. One can even wonder if it's worth clarifying the historical background, i.e. that there was OPENSSL_assert and that we are omitting it in favour of "soft" assertions, i.e. ones that crash only in debug builds.

levitte · 2018-02-16T16:28:57Z

policies/codingstyle-updates.txt

+	if (this_is_true()
+		&& !that_is_false()) {
+	    code();
+	    ...


This i a bit confusing. First, you talk about giving continuation lines in conditionals in general an extra indent, and then you seem to give if a special case...

Side note: if someone has a c-mode (emacs) hack to get that extra indentation, let me know. Doing it by hand is a royal pain...

If you have a better way to word it, please let me know. if is a special case.

I'm with Richard in sense that it is confusing. If extra indentation in multi-line condition is specific to "if", then having example in paragraph prior mentioning "if" is confusing.

Just in case to clarify. It's suggested to use extra indentation [in multi-line condition] specifically with "if" [and not for example with "while" as there is no visual dissonance].

Pushed commit that says "two indents for a continued if" Does that help?

paulidale · 2018-02-18T21:00:06Z

policies/codingstyle-updates.txt

+
+	#define foo(a, b) ((a) * (b))
+	#define quote(a) #a
+	#define join(a, b) a ## quote(a)


quote(b) perhaps? This is relevant to the example but having an unused parameter to the macro is a little odd.

paulidale · 2018-02-18T21:04:10Z

policies/codingstyle-updates.txt

+[ Matt said "initialize in the declaration if appropriate; can someone provide wording? ]
+
+This is controversial, so maybe drop it.  Don't use else after return or goto
+unless it makes the flow more clear.


Can anyone give an example where it makes the flow clearer?

The best I can think of is a sequence like:

if (...) .... else if (...) return else

However, here there is a semantic difference between having the else and not, so it isn't a good example of clarifying code flow.

Stronger would be:

Don't use else after return or goto unless doing so changes the meaning of the code (to what is required).

The best I can think of is a sequence

It's exactly the case, switch-like chains. One can even wonder if it's worth explicitly mentioning just that.

Any (very rough is fine!) suggestions on wording?

I'm just wondering if it would be appropriate to mention specifically "switch-like if chains". Like in "returns are tolerated in switch-like 'if' chains [as otherwise are discouraged/frown upon]."

paulidale · 2018-02-18T21:08:46Z

policies/codingstyle-updates.txt

+Prefer ossl_assert over assert. Do not use OPENSSL_assert() in libcrypto or
+libssl.
+
+[ Matt said "initialize in the declaration if appropriate; can someone provide wording? ]


I'm a little iffy here. I generally agree with the sentiment but there are lots of exceptions.

Declaration long before first use.

Series of complicated initialisers.

It will come down to appropriate.

What about:

Initialize variables as part of their declarations when it makes the intent clearer.

or:

Favor variable initialization as part of declaration unless it makes the intent of the code less clear.

richsalz · 2018-02-19T16:58:44Z

Added a commit to try and address @paulidale 's concerns.

richsalz · 2018-02-19T19:13:16Z

Pushed new command that tries to get the assert wording right, and also mentions the else-return sequence construct.

dot-asm · 2018-03-01T20:40:04Z

policies/codingstyle-updates.txt

+
+	some_long_condition()
+	    && (short1() || short2())
+	    && some_other_flag != 0


I don't quite follow why extra indent here. I mean how is it different from above example? If short1() and short2() are strongly related and deserve to be in (), then how is it different from just short1()?

Oh! On related note I wonder if it makes sense to explicitly recommend following formatting for multi-line ?:

condition ? this() : that();

In other words with : aligned with ?

The nested indent examples were wrong, I fixed them.
I am curious what others think of ?: I like this:

foo() ? that() : this()

for what it's worth.

Please, when coming up with this, consider creating cc-mode mods for us emacs users.
(otherwise, I think I know whose name I'm gonna scream in frustration ;-))

kroeckx · 2018-03-06T21:17:58Z

policies/codingstyle-updates.txt

@@ -0,0 +1,75 @@
+A summary of the discussion thread so far.  Not surprisingly, it's all about
+the whitespace. :)


You mean those 2 spaces after the '.'?

dot-asm · 2018-03-21T11:34:01Z

Quoting openssl/openssl#5626 (comment):

"Even though styling guide refers to it as preferred style, I can't recall that we would actually tolerate opening /* and closing */ [in multi-line comment] not being on lines of their own. Not in new code."

Would it be appropriate to re-word "commenting" section so that one doesn't have to put it as "I can't recall"? Or do we tolerate? Even in such case it might be appropriate to put it "preferred, but not the only acceptable, ..."

dot-asm · 2018-03-21T11:38:29Z

policies/codingstyle-updates.txt

+ossl_assert() or assert(), as appropriate.
+
+Favor variable initialization as part of declaration unless it makes the
+intent of the code less clear.


At couple of occasions I felt compelled to object something similar to

int a, b, c = 3, d, e, f;

I'd argue that assignment should be either last or on its own line.

added a commit to address this.

richsalz · 2018-03-21T12:46:10Z

Added a commit about multi-line comments.

richsalz · 2018-04-10T02:08:47Z

The vote was two yes, two no, three abstain and one not voting. Closing the PR. I hope someone else will take this up.

richsalz self-assigned this Feb 16, 2018

mattcaswell reviewed Feb 16, 2018

View reviewed changes

dot-asm reviewed Feb 16, 2018

View reviewed changes

levitte reviewed Feb 16, 2018

View reviewed changes

paulidale reviewed Feb 18, 2018

View reviewed changes

dot-asm reviewed Mar 1, 2018

View reviewed changes

richsalz mentioned this pull request Mar 6, 2018

Document no-space-after-cast #25

Closed

richsalz added 7 commits March 6, 2018 15:35

Working copy for coding style updates

7fbb84c

Matts first comments

b2c51fd

Andys feedback

f6a287b

Paulis feedback

283785f

Reword assert usage. Explain else/return construct.

7f18bb9

Explain *two* indents for long if lines.

b5d25b4

Fix nested conditionals indent

a2d317b

kroeckx reviewed Mar 6, 2018

View reviewed changes

richsalz added 2 commits March 6, 2018 16:26

Update intro comments

3b27034

Fix line-endngs

dd4bca8

richsalz closed this Mar 20, 2018

richsalz deleted the coding-style-updates branch March 20, 2018 13:44

richsalz restored the coding-style-updates branch March 20, 2018 13:44

richsalz reopened this Mar 20, 2018

dot-asm reviewed Mar 21, 2018

View reviewed changes

richsalz closed this Apr 10, 2018

richsalz mentioned this pull request Apr 10, 2018

Add policy on assertion usage #42

Closed

richsalz deleted the coding-style-updates branch May 14, 2018 20:38

richsalz restored the coding-style-updates branch May 14, 2018 20:38

richsalz deleted the coding-style-updates branch May 29, 2018 19:59

kroeckx mentioned this pull request Sep 2, 2023

Add assert policy openssl/technical-policies#72

Closed

		@@ -0,0 +1,75 @@
		A summary of the discussion thread so far. Not surprisingly, it's all about
		the whitespace. :)

Working copy for coding style updates #43

Working copy for coding style updates #43

Conversation

richsalz commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richsalz commented Feb 19, 2018

richsalz commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm commented Mar 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richsalz commented Mar 21, 2018

richsalz commented Apr 10, 2018

dot-asm Feb 16, 2018 •

edited

Loading

dot-asm Feb 19, 2018 •

edited

Loading