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: dev@openvswitch.org 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 dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
