Samuel, Thanks for writing this up. I don't agree with some of them, but do like some of the suggestions. I am actually encouraged that the CodingStyle for datapath-windows is evolving.
> - 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 ..'. The guideline is fine. It seems to be a rewording of the existing guideline, but it is ok to do it. > + > + All macros, symbols, and custom types (i.e. typedef to enums, structs, > unions) must > +be prefixed by "OVS_". Functions must not be prefixed by "Ovs". 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. 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. 'should' migth be better. I am using the meanings of 'must' and 'should' as defined in RFC 2119 (https://www.ietf.org/rfc/rfc2119.txt). > + > + Enum constants must follow the name style of the enum type. > + > + Example: > + > +typedef enum _OVS_IPPROTO > +{ > + OVS_IPPROTO_ICMP = 1, > + //constants here. > +}; > + > + This makes easier lookup of enum constants, knowing the enum type. > + > + All types must be declared with typedef. Type typedef must always be used. > + > + Example: > + > +typedef struct _OVS_DATAPATH > +{ > + //fields here > +}OVS_DATAPATH, *POVS_DATAPATH; > + > + Always use OVS_DATAPATH or POVS_DATAPATH. Do not declare variable by > + "struct _OVS_DATAPATH dp;" Sounds good. > + Builtin types: UINT, INT, UINT16, VOID etc. are preferred over unsigned > int, int, > +unsigned short, void, etc. Do not use typedef-s for builtin types that are > lowercase. > +For example, do not use uint32_t, uint16_t, etc. This is already documented in 'TYPES' section and I quote: <quote> TYPES Use data types defined by Windows for most of the code. This is a common practice in Windows driver code, and it makes integrating with the data structures and functions defined by Windows easier. Example: DWORD and BOOLEAN. </quote. If anything you probably want to add to this section and not to the 'NAMING' section. > + 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. > + Variables declared in a .c file that are used only within that .c file > must be static and > +prefixed by "s_" (stands for "static"). Global variables, i.e. variables > that are declared in > +a .h file and used in multiple .c files must be prefixed by "g_" (stands for > "global"). I like the g and s idea, but do we need an '_'? If we are going with camelCasing, '_' is not generally part of camelCasing. > + > + Pointer data types should be prefixed by "p" or "pp". I don't know how much this is going to be buy us honestly. I am assuming that this convention is being added for the following reasons: 1. Makes writing code easier. Maybe mistakes can be avoided while writing code such as: pFOO.BAR = 1; Instead, it becomes easier to write: pFOO->BAR = 1. 2. While accessing variables, it would be make it obvious when to use '&' and when to use '*' for eg. 3. While reading code, the name of the variable would make it obvious whether it is a pointer/array etc. That said, for #1 and #2, a good editor will most of the time to give you drop down menu or just show some error/warning about syntax errors. If something is missed at that point, compilation will catch it. 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. While I agree what you are suggesting is a nice thing, I feel it makes the code a little clunky, for not much additional value. I am ok with this if others are OK. > COMMENTS > > @@ -103,12 +171,41 @@ the name of the function or based on the workflow it is > called from. > In the interest of keeping comments describing functions similar in > structure, use the following template. > > + 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: <nithin patch> /* *---------------------------------------------------------------------------- * Any description of the function, arguments, return types, assumptions and * side effects. *---------------------------------------------------------------------------- */ + 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. </nithin patch> > + > + All private functions (i.e. functions that are to be used only in one .c > file) should > +be defined as static, and be "_" prefixed. > + > + Example: > + > +static BOOLEAN _FunctionName(VOID* p) > +{ > + //code > +} > + > + This makes it clear when studying the code, and when seeing function calls > weather > +the function is supposed to be private or public. Ok. > + > + Public functions that operate on a specific component should have a > component name prefix. > + > + Example: > + > +OVS_FLOW* FlowCreate(VOID* p) > +{ > + //code > +} 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) { } > + 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 .. } > SOURCE FILES > > @@ -137,3 +234,79 @@ quickly figure out where a given module fits into the > larger system. > > 4. Open vSwitch headers, in alphabetical order. Use "", not <>, > to specify Open vSwitch header names. > + > +BRACES > + > + Use Windows-style braces: > + The open brace must be on a separate line. > + > + Example: > + > +if (condition) > +{ > + //code > +} 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 } BTW, why do you call this 'Windows-style'? I had some some research on this and had not found any authoritative coding standard for Windows. Here's some internal notes for your reference as to why we wrote up a CodingStyle for datapath-windows: <notes> There does not seem to be a specific coding standard defined by Microsoft for C++ code. 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. </notes> > +typedef struct _OVS_DATAPATH > +{ > + //fields here > +}OVS_DATAPATH, *pOVS_DATAPATH; I have a prefence again for efficiency reasons, but I don't feel as strongly as the 'if'. typedef struct _OVS_DATAPATH { // fields here } > + All code executed for an if, else, while, do while, or for, must be > + enclosed in braces, even if it's only one instruction to be executed. right on! > + The else part of an if must be on the very next line after the if's closed > + brace (i.e. there should be no blank line in between) > + > + Example: > + > +if (condition) > +{ > + //code > +} > +else > +{ > + //code > +} For efficiency reasons, I believe we should stick with. Most modern editors support syntax highlighting. So, it is easy to spot where the 'else' keyword is. Reading should not be an issue. if (condition) { // code } else { // code } 5 lines v/s 8 lines :) > + > +For all other cases than else, after "}", a blank line must follow Hmm, my opinion is that, statements that can be logically grouped together should be grouped together without lots of new newlines. Again, for using the screen efficiently. Between one logical block to another local block, there should be a newline. Some examples: UINT32 foo; NTSTATUS status; status = RandomFunction(foo); if (status != STATUS_SUCCESS) { goto end; } RandomFunction2OnSuccess(foo); // rest of the code to do something else. > +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. > + Number of lines per file: try to limit to 1000 lines. Maximum allowed > should be 1500. > +If a file is grown to large in number of lines, consider splitting it into > files, based on > +the components the functions operate upon. This, I am sure this is a question that is as passionate as VI v/s EMACS :) 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. > +SPACES WITHIN LINES > + > + Use spaces between operands and operators. > + > + Example: > + > +x + y * z > + > +instead of: > + > +x+y*z > + > + Do not use space after "(" or before ")" > + > + Example: > + > +(x * y * z) + a > + > + instead of: > + > +( x * y * z ) + a Absolutely. Since we are making this document very detailed some other areas that can be covered are: - Add a whitespace after 'if', 'switch' and 'case', before the '('. - Add a whitespace after the ')' in an if condition before the '{'. I don't know how to phrase this better :) thanks, Nithin _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
