Also, I suggest:
o) multiline comments - to have one style for them only.
I personally prefer:
/*
* comment_line1
* comment_line2
* comment_line3
*/
Others may prefer:
/* comment_line1
* comment_line2
* comment_line3 */
We would need to decide, and keep it one way only.
o) function declarations and definitions.
I believe we should keep this style for all functions that have multiple params:
[CODE]
VOID
FuncName(Type1 param1,
Type2 param2,
Type3 param3)
{
...
[/CODE]
All Type-s should be aligned / on the same colomn.
o) function calls:
I have seen in code both:
Function(param1,
param2,
...,
paramN);
(all params aligned to the same column)
and
Function(param1,
param2, ...,
paramN);
(params on the following lines aligned to 4 more spaces than the first line)
We may need to decide on a single style.
Thanks,
Sam
________________________________________
From: Samuel Ghinet
Sent: Thursday, August 21, 2014 10:45 PM
To: Nithin Raju
Cc: [email protected]
Subject: RE: [ovs-dev] [PATCH 01/15] datapath-windows: Update CodingStyle
Also, I think it would be cleaner & would improve code readability if we would
use a max 1 goto Label, e.g. a "Cleanup". Rather than using multiple labels per
func.
A "Cleanup" would be something like cleanup in case something went wrong - the
all-ok part means reaching the end of the function normally.
I think it would also be easier to maintain such function than if we needed to
put additional goto labels for new cases, or re-consider the paths of each
workflow when you modify code in between labels, or even before the labels.
Sam
________________________________________
From: Samuel Ghinet
Sent: Thursday, August 21, 2014 9:45 PM
To: Nithin Raju
Cc: [email protected]
Subject: RE: [ovs-dev] [PATCH 01/15] datapath-windows: Update CodingStyle
Thanks Nithin for your opinions on coding style!
[QUOTE]
The guideline is fine. It seems to be a rewording of the existing guideline,
but it is ok to do it.
[/QUOTE]
Except, the current coding style says:
"For types, use all upper case for all letters with words separated by '_'. If
camel casing is preferred, use upper case for the first letter."
I personally do not like combining all caps with "_" in between, with this
style:
Eth_RxMode (enum)
Eth_LLC8 (struct)
I would personally prefer
OVS_ETH_RX_MODE (enum)
and
OVS_ETH_LLC8 (struct)
[QUOTE]
> - For types, use all upper case for all letters with words separated by '_'.
> If
> - camel casing is preferred, use upper case for the first letter.
> + All types, all enum constants and all symbol definitions must be all upper
> case.
> +If the name is made of multiple words, separate words by "_".
One general comment is to keep the indentation of new lines to be the same as
the previous line in the same paragraph. So, you'll have to add a couple of
whitspace in front of the 'If the name ..'.
[/QUOTE]
Sorry, my mistake there :)
[QUOTE]
One convention we've followed (and not documented explicitly) is that all entry
points to the driver from NDIS are prefixed with 'OvsExt'. I believe this is
the right thing to do since the NDIS callbacks are spread all over the code,
and it makes sense to call them out if someone wants to start tracing code for
a workflow from the top.
[/QUOTE]
I personally believe that keeping the NDIS filter standard names would be
better.
e.g. instead of calling it OvsExtSendNBL, to call it FilterSendNetBufferLists.
Otherwise, functions that are not NDIS callbacks, do you think it's ok to drop
the Ovs prefix?
And, do you think it ok for all enums, structs, symbols, to have OVS_ prefix?
(i.e. to also have all caps with "_")
[QUOTE]
I am not sure if it makes sense to use 'must' when you say 'Functions must not
be prefixed by "Ovs".' 'must' is a very strong word.
[/QUOTE]
It is a bit unclear to me what "should" should mean in the coding style.
I mean, say about the max 79 chars / line suggestion. When you send a patch,
how will you consider if, say, 84 for a case is ok? Technically, any piece of
code could be split (there is no identifier as big as 79 chars).
How it's more clear to write it, is perhaps, very subjective.
[QUOTE]
I like the g and s idea, but do we need an '_'? If we are going with
camelCasing, '_' is not generally part of camelCasing.
[/QUOTE]
What I don't like about "s" without "_" is that, it may be confounded with
"string", as in the hungarian notation.
[QUOTE]
> + Code annotations are preferred.
I am reading this as 'add comments where the code is rather than documenting
everything in the function description'. Sure, we can make this explicit. But,
pls. add it after the L111 in the current code. Something like:
[/QUOTE]
Look here for code annotations:
http://msdn.microsoft.com/en-us/library/ms182032.aspx
Basically, it would be too much to expect all, but things such as "_In_",
"_In_opt_", "_Inout_", "_Inout_opt_", for func parameters, "_Ret_maybenull_"
for returns used in func declarations may give a clearer understanding of how
the func is used.
[QUOTE]
For comments that describe the internal working of a function, it is best to
add the comments close to where the code is rather + than documenting
everything in the comment block that describes the function itself.
[/QUOTE]
This would be nice as well.
[QUOTE]
> + In Visual Studio, ULONG is identical to UINT and with UINT32. The use of
> ULONG is
> +preferred. Use unsigned types whenever the variable should only have
> positive values.
> +Use typedef-s for UINT32: BE32, BE64, whenever it is known that the variable
> is Big Endian.
> +Use UINT32 only when it is important to know that the value is 32 bit long.
> Otherwise, use
> +ULONG.
ULONG definition is not part of the Visual Studio. You might want to correct
that. Personally, I like using explicitly sized types like UINT16 or UINT32
since I feel I have control over the size independent of the platform
underneath. I'll get back to you on this in a couple of days, since making the
change will mean changes (although trivial) all over the code.
[/QUOTE]
Yeah, a mistake. ULONG is Microsoft API typedef.
ULONG is, in size, identical to UINT, but "int" and "long" in Visual C(++) are
taken as different types, even though they're both 32bit.
So it's Visual Studio / Visual C(++) where "unsigned long" equals in size as
"unsigned int" and "long" with "int".
I personally prefer not to specify size (like UINT32), unless it's very
important for that piece of code to be known that it's 32 bits. E.g. when data
structures are passed between kernel and userspace, where the size of the
variable truly matters.
[QUOTE]
3. While reading code, the name of the variable would make it obvious whether
it is a pointer/array etc.
For #3, having read my share of code, I think looking at how a variable is
accessed makes is very easy to guess what it's type is - pointer v/s
non-pointer. I don't think prefixing a 'p' adds much value. Some examples you
can look at are the OVS userspace code itself. Generally, by looking at how a
variable it accessed, you can guess its type. Same goes with the Linux Datatype.
[/QUOTE]
The "p" prefix does help when the ptr variable is passed to a function or
assigned.
I must admit a programmer can live without "p" prefix. Finally, after you get
familiar with the code a bit, it becomes only a matter of preference.
[QUOTE]
Ok. On thing though is we want, and not in the format you are suggesting. This
is already documented in the 'FUNCTIONS' block. An example won't hurt. Well,
there is an example at the top in the 'NAMING' section, maybe we can refer to
it.
POVS_FLOW
FlowCreate(PVOID p)
{
}
[/QUOTE]
It's ok with me.
[QUOTE]
> + Functions that operate on global data (such as, functions that add /
> remove flows), and expect the
> +caller to hold a lock beforehand, should be suffixed with "Unsafe". If the
> component function (e.g. Flow)
> +does not lock its specific lock (e.g. the flow's lock field), but locks any
> other component's lock or
> +global lock, it should still be suffixed by "Unsafe".
Ok. If we can add some ASSERTS in the function that would be great as well. I
need to look up the NDIS function that can tell us if a particular lock has
been held or not. If not, maybe we can invent a meta data structure that tells
us if a lock is held.
OVS_LOCK {
BOOLEAN locked;
NDIS_LOCK ..
}
[/QUOTE]
I don't think that would be useful.
I mean, if that is a "global" rw lock (global, in the meaning that, e.g. is a
lock of a flow, which belongs to a flow table / datapath, which is global),
some other thread / CPU might have locked that lock,
so the ASSERT would be ok (locked), but as soon as you enter the function body,
that other thread releases the lock, so you're modifying data without holding
the lock. What problem would this solve?
What I meant was to say "hey, you must make sure that YOU locked lock_x, before
calling this function"
AFAIK, windows does not provide a mechanism, such as I have seen in linux
kernel, to say if a lock is held.
[QUOTE]
No, I don't agree with this. This just creates lots of new lines and does uses
screen space very inefficiently. Let's stick with
if (condition) {
// code
}
[/QUOTE]
The BSD style may be ok for small blocks of code, but I find it quite
unreadable when used over 20+ lines of a code block.
[QUOTE]BTW, why do you call this 'Windows-style'?[/QUOTE]
That's what I have seen in all Microsoft code samples.
[QUOTE]
Some examples I’ve
found are:
1) Coding Techniques and Programming Practices:
http://msdn.microsoft.com/en-us/library/aa260844%28VS.60%29.aspx
2) Coding Style Conventions:
http://msdn.microsoft.com/en-us/library/aa378932%28VS.85%29.aspx
3) All-In-One Code Framework Coding Guideline published in
CodePlex (most detailed but not as detailed as the OVS CodingStyle):
http://1code.codeplex.com/wikipage?title=All-In-One%20Code%20Framework%20Coding%20Standards&referringTitle=Docume
We thought it is better to write up a CodingStyle, but keep
it flexible enough to be acceptable to seasoned Windows developers.
[/QUOTE]
"Coding Techniques and Programming Practice" is quite old. Visual Studio 6.
"All-In-One Code Framework Coding Guideline" says "do use Allman bracing style,
which is quite this non-BSD style.
[QUOTE]
Between one logical block to another local block, there should be a newline.
[/QUOTE]
I agree. Also, between variable declarations and code, there should be a blank
line. As in your example.
[QUOTE]
Hmm, my opinion is that, statements that can be logically grouped together
should be grouped together without lots of new newlines
[/QUOTE]
I also believe that we should try to make functions smaller by creating new
functions when needed, and trying to make, as best as possible, each function
do one thing only.
This would reduce the number of parameters functions require, and would make a
function clearer, if it didn't need dozen of comments explaining how it behaves
on each case.
So, what I'm saying is that, perhaps we should try making the functions shorter
by making them more atomic (do one thing) & simple, rather than condense code
by stripping blank lines where we can.
[QUOTE]
> +SWITCH CASES
> +
> + Each "case", if it is preceded by a "break" of a previous case, there must
> be
> + a blank line in between:
> +switch (c)
> +{
> + case v1:
> + //code
> + break;
> +
> + case v2:
> + //code
> + break;
> +}
For efficiency reasons, I feel 'case' should start at the same indentation
level as 'switch'. Same standard is followed by OVS userspace. I am not saying
we should blindly follow OVS userspace, but there's a standard that is working
well.
[/QUOTE]
Oops, my bad here. VS 2013 does also indent the "case" the same as "switch".
Do you agree with blank lines following the "break;"?
[QUOTE]
In general, I don't have any problem with files being long. The code flow has
to be simple to follow and should make sense. Cutting down the number of lines
in a file many times directly translates to cutting down the number of
functions which might result in unnecessary grouping, or a given function
itself may be split into multiple sub-funtions. But, if there are no other
callers to these sub-functions, I don't know if it makes sense to make it a
sub-function. Anyway, I don't think it makes sense to just impose a limit, but
we should handle this on a case by case basis.
[/QUOTE]
I personally find files of very many lines of code daunting.
I also believe it may be useful to create a subfunction, even if it's called
only by one function, if this sub-function makes its caller function clearer &
shorter.
Thanks!
Sam
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
