Re: [dpdk-dev] tools brainstorming

["Butler, Siobhan A" <siobhan.a.butler@intel.com>]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, April 8, 2015 7:16 PM
> To: Butler, Siobhan A
> Cc: Thomas Monjalon; dev@dpdk.org
> Subject: Re: [dpdk-dev] tools brainstorming
> 
> Thanks for doing this, it is a great start.
> I admit strong bias towards Linux kernel style.
> 
> Could you use one of the standard markup styles so that it could get put in
> documentation?


Sure Stephen - will tidy it all up when we get a consensus :) 
Thanks for the feedback 
Siobhan 
> 
> 
> > License Header
> > --------------
> 
> I prefer the file just say that it is BSD or GPL and refer to license files in the
> package. That way if something has to change it doesn't need a massive
> license sweep
> 
> 
> >
> > Macros
> >
> > Do not ``#define`` or declare names in the implementation namespace
> except for implementing application interfaces.
> >
> > The names of ``unsafe`` macros (ones that have side effects), and the
> names of macros for manifest constants, are all in uppercase.
> >
> > The expansions of expression-like macros are either a single token or
> > have outer parentheses. If a macro is an inline expansion of a
> > function, the function name is all in lowercase and the macro has the same
> name all in uppercase. Right-justify the backslashes; it makes it easier to
> read. If the macro encapsulates a compound statement, enclose it in a do
> loop, so that it can be used safely in if statements.
> > Any final statement-terminating semicolon should be supplied by the
> macro invocation rather than the macro, to make parsing easier for pretty-
> printers and editors.
> >  #define MACRO(x, y) do {                                        \
> >          variable = (x) + (y);                                   \
> >          (y) += 2;                                               \
> >  }while (0)
>     ^ bad whitespace
> 
> it is important that all examples in documentation are perfect.
> 
> 
> > C Function Definition, Declaration and Use
> >
> > Prototypes
> >
> > It is recommended, but not required that all functions are prototyped
> somewhere.
> >
> > Any function prototypes for private functions (that is, functions not used
> elsewhere) go at the top of the first source module. Functions
> > local to one source module should be declared static.
> 
> I find prototypes for private functions to be redundant and error prone.
> The do nothing. Better to just put private functions in the correct order.
> 
> 
> You also need to raise the issue that all global names need to be prefaced by
> a unique string.
> I see places in drivers where global names leak out causing possible later
> name collision.
> 
> > Definitions
> > -----------
> >
> > The function type should be on a line by itself preceding the function. The
> opening brace of the function body should be on a line by itself.
> >  static char *
> >  function(int a1, int a2, float fl, int a4)
> >  {
> 
> Not a big fan of that style. Prefer it on same line.
> 
> 
> >
> > Indentation is a hard tab, that is, a tab character, not a sequence of spaces.
> 
> Also no spaces before tabs.
> 
> > NOTE General rule in DPDK, use tabs for indentation, spaces for alignment.
> > If you have to wrap a long statement, put the operator at the end of the
> line, and indent again. For control statements (if, while, etc.),
> > it is recommended that the next line be indented by two tabs, rather than
> one, to prevent confusion as to whether the second line of the
> > control statement forms part of the statement body or not. For non-
> control statements, this issue does not apply, so they can be indented
> > by a single tab. However, a two-tab indent is recommended in this case
> also to keep consistency across all statement types.
> >  while (really_long_variable_name_1 == really_long_variable_name_2 &&
> >      var3 == var4){
> >      x = y + z;      /* control stmt body lines up with second line of */
> >      a = b + c;      /* control statement itself if single indent used */
> >  }
> >
> >  if (really_long_variable_name_1 == really_long_variable_name_2 &&
> >          var3 == var4){  /* two tabs used */
> 
> No. Should line up with really_long_variable_name_1
> 
> >      x = y + z;          /* statement body no longer lines up */
> >      a = b + c;
> >  }
> >
> >  z = a + really + long + statement + that + needs +
> >          two + lines + gets + indented + on + the +
> >          second + and + subsequent + lines;
> >
> >
> > Do not add whitespace at the end of a line.
> >
> > Closing and opening braces go on the same line as the else keyword. Braces
> that are not necessary should be left out.
> >  if (test)
> >          stmt;
> >  else if (bar) {
> >          stmt;
> >          stmt;
> >  } else
> >          stmt;
> >
> >
> > Function Calls
> > --------------
> >
> > Do not use spaces after function names. Commas should have a space after
> them. No spaces after ``(`` or ``[`` or preceding the ``]`` or ``)`` characters.
> >  error = function(a1, a2);
> >  if (error != 0)
> >          exit(error);
> >
> >
> > Operators
> > ---------
> >
> > Unary operators do not require spaces, binary operators do. Do not use
> parentheses unless they are required for precedence or unless the
> > statement is confusing without them. Remember that other people may
> be more easily confused than you.
> >
> > Exit
> >
> > Exits should be 0 on success, or 1 on failure.
> >          exit(0);        /*
> >                           * Avoid obvious comments such as
> >                           * "Exit 0 on success."
> >                           */
> 
> +11
> 
> 
> >
> > Return Value
> > ------------
> >
> > If possible, functions should return 0 on success and a negative value on
> error. The negative value should be ``-errno`` if relevant, for example, ``-
> EINVAL``.
> >
> > Routines returning ``void *`` should not have their return values cast to any
> pointer type.
> > (Typecasting can prevent the compiler from warning about missing
> prototypes as any implicit definition of a function returns int - which, unlike
> "void *" needs a typecast to assign to a pointer variable.)
> > NOTE The above rule applies to malloc, as well as to DPDK functions.
> > Values in return statements should be enclosed in parentheses.
> 
> Sorry, this is a stupid BSDism
>  	return (-EINVAL);
> is ugly
> 
> >
> > Logging and Errors
> > ------------------
> >
> > In the DPDK environment, use the logging interface provided::
> >  #define RTE_LOGTYPE_TESTAPP1 RTE_LOGTYPE_USER1
> >  #define RTE_LOGTYPE_TESTAPP2 RTE_LOGTYPE_USER2
> >
> >  /* enable these logs type */
> >  rte_set_log_type(RTE_LOGTYPE_TESTAPP1, 1);
> >  rte_set_log_type(RTE_LOGTYPE_TESTAPP2, 1);
> >
> >  /* log in debug level */
> >  rte_set_log_level(RTE_LOG_DEBUG);
> >  RTE_LOG(DEBUG, TESTAPP1, "this is is a debug level message\n");
> >  RTE_LOG(INFO, TESTAPP1, "this is is a info level message\n");
> >  RTE_LOG(WARNING, TESTAPP1, "this is is a warning level message\n");
> >
> >  /* log in info level */
> >  rte_set_log_level(RTE_LOG_INFO);
> >  RTE_LOG(DEBUG, TESTAPP2, "debug level message (not displayed)\n");
> >
> >
> > In a userland program that is not a DPDK application, use err(3) or warn(3).
> Do not create your own variant.
> >          if ((four = malloc(sizeof(struct foo))) == NULL)
> >                  err(1, (char *)NULL);
> >          if ((six = (int *)overflow()) == NULL)
> >                  errx(1, "number overflowed");
> >          return (eight);
>                   ^ NO BSD style return ()
> 
> >
> > Branch Prediction
> > -----------------
> >
> > When a test is done in a critical zone (called often or in a data path) use the
> ``likely()`` and ``unlikely()`` macros. They are expanded
> > as a compiler builtin and allow the developer to indicate if the branch is
> likely to be taken or not. Example:
> >  #include <rte_branch_prediction.h>
> >  if (likely(x > 1))
> >    do_stuff();
> 
> You need to stress that likely() and unlikely() should be used sparingly.
> Many times the compiler knows better.
> 
> >
> >
> > Static Variables and Functions
> > ------------------------------
> >
> > All functions and variables that are local to a file must be declared as
> ``static`` because it can often help the compiler to do
> > some optimizations (such as, inlining the code).
> >
> > Functions that must be inlined have to be declared as ``static inline`` and
> can be defined in a .c or a .h file.
> >
> > Const Attribute
> > ---------------
> >
> > Particular care must be taken with the use of the ``const`` attribute. It
> should be used as often as possible when a variable is read-only.
> 
> 
> A couple more things:
>   no UPPER or CamelCase variable names or functions
> 
> The other thing (and Intel is the worst offender) is to avoid excessive inlining.
> For example, I am pretty sure that all rte_ring stuff really should not be
> inline,
> the compiler can do a good job (especially with LTO) even without inlining.