Solved Is style(9) due for some updates?

[unitrunker]

Reference: style(9)

The style(9) guide describes style for kernel source files. It is also applied to non-kernel code in base.

Here's an example where I think the advice of style(9) is obsolete.

There's the DRY (don't repeat yourself) principle which - according to wikipedia - has been around about 20 years.

Don't repeat yourself - Wikipedia

en.wikipedia.org

The idea is to avoid multiple parallel definitions (or implementations) of the same thing.

With regards to functions in C, style(9) advises this:

All functions are prototyped somewhere.

This has been applied not just to external functions but to local, static file scope functions.

Since bectl is a recent addition to base, I'll use it as an example.

Here are the local file scope function prototypes for bectl.c:

static int bectl_cmd_activate(int argc, char *argv[]);
static int bectl_cmd_check(int argc, char *argv[]);
static int bectl_cmd_create(int argc, char *argv[]);
static int bectl_cmd_destroy(int argc, char *argv[]);
static int bectl_cmd_export(int argc, char *argv[]);
static int bectl_cmd_import(int argc, char *argv[]);
#if SOON
static int bectl_cmd_add(int argc, char *argv[]);
#endif
static int bectl_cmd_mount(int argc, char *argv[]);
static int bectl_cmd_rename(int argc, char *argv[]);
static int bectl_cmd_unmount(int argc, char *argv[]);

All of these commands are implemented further down in the same source file. The implementation also declares the function name, parameters and return type. This means the prototypes are unnecessary. They don't help the compiler and don't make the code more readable. It's just a bit of text duplicated twice in the same file - all because style(9) says so.

This repetition is a nuisance - a small one - but still a nuisance.

Edit:

For userland code, place main() at the end of the source file (just as was done in the bectl example above).

 

[mark_j]

I'm not sure about the code, I haven't looked at it, but the function prototypes are declarations designed to prevent clang emitting warnings.

 

[ekvz]

While i wouldn't do it myself one has to admit that the simplicity off adding all declarations instead of figuring out which are needed has something going for it.

 

[a6h]

void f();int main(){f();}void f(){}

&

void f();void f(){}int main(){f();}

# cc c.c # => OK

int main(){f();}void f(){}

# cc c.c # => NOT OK

c.c:1:12: warning: implicit declaration of function 'f' is invalid in C99 [-Wimplicit-function-declaration]
int main(){f();}void f(){}
           ^
c.c:1:22: error: conflicting types for 'f'
int main(){f();}void f(){}
                     ^
c.c:1:12: note: previous implicit declaration is here
int main(){f();}void f(){}
           ^
1 warning and 1 error generated.

Normally ..................: function prototype => main() => function definition.
This case ..................: function prototype => function definition => main().
Conclusion ..............: Stylistic mistake! They should move def. after main()!
Recommendation : Chill out!

 

[memreflect]

DRY is a good principle, but it's not something that is easily followed in C due to how C works. Without function prototypes, the following nonworking code compiles fine with all warnings enabled. There will be some warnings issued about implicit function declaration, but that's it; all of the definitions of the functions are all perfectly compatible with the implicit K&R declarations of the form int NAME(); :

#include <stdio.h>
#include <string.h>

int main(void) {
    return foo(-40) + bat('a');
}

int foo(const char *s) {
    return (printf("%d\n", bar(s)) != 0);
}

int bar(int n) {
    if (n % 2 == 0)
        return n / 2;
    return n * 3 + 1;
}

int bat(const char *s) {
    return (int)strlen(s);
}

Sure, you could change the order of the function definitions, but that's impossible when you have a chain of circular references like A calling B, B calling C, and C calling A. And as the code base evolves, it's possible that you need to call a function further down in the file when you previously did not need to do so.

A long list of prototypes is less of a nuisance than bugs and time wasted figuring out how to reorder functions in a file (assuming placement would even make a difference).

 

[mark_j]

Clang defaults to C99, so unless you expressly specify C89 (-ansi), it will warn you.
Frankly, C99 is old stuff now, so I would not expect to see much C89 stuff unless you're pulling it from archives or running GCC prior to (guess) 6?

So, this DRY concept is in reality violated by the need to adhere to the C99 and later standards.

 

[unitrunker]

To be clear - I'm talking about a small refinement to what is already there. An exception to the existing one-size-fits-all language.

C is a one-pass compiler. It needs to see something defined before it can use it. That's true for structures and functions. That implies C has a natural order for functions inside a source file. Placing main before all other functions means you're going to write the function definitions twice - and forever fix parameter and name changes in two places.

C compilers traditionally default to the type 'int' when they see a new symbol - if only to allow the compiler to continue in hopes of covering more code to provide feedback. That's not behavior anyone wants to rely upon for running code.

Placing main at the top of a source file is a self-inflicted wound. I can see now I should have included placing main at the end of the source file as part of my suggestion.

The point I make is to alter the style(9) recommended form ...

static int A();
static int B();

static int 
A()
{
    /* */
}

static int 
B()
{
    A();
}

int 
main()
{
    B();
    return 0;
}

... to a slightly simpler form ...

static int 
A()
{
    /* */
}

static int 
B()
{
    A();
}

int 
main()
{
    B();
    return 0;
}

Again, this is for static functions confined to file scope. The bectl example places main at the end of the source file - which - IMO - is where it belongs.

Moving code around to cause it to NOT compile is not the point.

This isn't just confined to where you place main. For any public function that calls a static function, place the static function ahead of the public one.

Sure there will be recursive functions that call each other - one of those will need a prototype to compile. style(9) isn't going to tell you to write code that does not compile.

It's all these other gratuitous prototypes of static, file scope functions that are unnecessary.

This is also about risk. For external symbols - this is unavoidable - just because this is how C (and C++) works. It's why you have header files where you repeat the declaration part of a function (as a prototype). You can have a prototype not match its implementation. For C, this is possible if the implementation doesn't include the prototype's header file. For C++, this is possible because overloading symbols is allowed.

More than likely you'll get a link time failure. Fixing those errors also has a cost. In some rare cases, it will link but the code is just wrong. Now you're in a world of hurt.

A long list of prototypes is less of a nuisance than bugs and time wasted figuring out how to reorder functions in a file (assuming placement would even make a difference).

This is a fair counter-point. For existing code - no I don't want to reorder working code to shave a dozen lines. style(9) already acknowledges the presence of pre-existing code that does not follow its advice (bool vs. int or boolean_t for example). If someone happens to work in an area where this exists, the person may choose to do a modest face lift of the code - or not.

This tweak to style(9) would be for new code going forward. Existing code can stay as-is.

 

[mark_j]

How do you propose variadic functions are catered for? The same as for recursive? It's starting to get messy.
It also relies, too heavily, on the placement of code. This might be fine for a rather simplistic program, but more complex works could require a lot of extra (and I believe unreasonable) work.

This is why C has prototypes. It's also convention. If I am auditing code, I don't want to be scrolling down hundreds of lines to find a declaration. I would much rather look at them in the header or the top of the program. But, that's just my personal choice (and one my workplace enforces, strictly, ie all code must be compiled with -Wstrict-prototypes ).

NetBSD's style is contrary to yours, as well. FYI Reference: http://cvsweb.netbsd.org/bsdweb.cgi/src/share/misc/style?rev=HEAD&content-type=text/x-cvsweb-markup

 

[unitrunker]

Just for grins, I removed the static prototypes from bectl.c. After moving command_map (a table) and get_cmd_info (a function) down to just before main, it builds fine. So yes, it is possible. Would you want to apply this retroactively? Nah.

How do you propose variadic functions are catered for?

The use of varargs makes no difference here. If, for example, you have a file scope logging function with printf style arguments - just make sure you define it before using it. If your logging function is super-useful - it will go in a separate source file with its own header file - at which point it no longer applies here.

If I am auditing code, I don't want to be scrolling down hundreds of lines to find a declaration.

If you're auditing code, you will most definitely be scrolling down to see the code - with or without a prototype.

There appears to be another reason for the existing language in style(9). Though not expressly called for, many programs order their functions alphabetically or "logically" within a file. I'm not sure how this came to be or what value it provides. In most editors, you can type control-F (or :/ in vi) to find a function by name. However, this pattern seems entrenched. bectl is such an example. Such an ordering will seldom align with the "natural" order preferred by a C compiler.

At that point, you have to use prototypes everywhere. Bleh.

 

[mark_j]

Variadic functions are different because you must include a prototype. It's a rule contrary to your rule. If consistency is your goal, then this breaks it. If removing extraneous prototypes is your goal at the cost of consistency, I guess it fits your goal.

As to code auditing, you often DO NOT need to look at the make-up of the function, especially at the preliminary level, so viewing the prototype is preferable. For example, I can look at the code and see it's got 100 prototypes in there just by looking at the top of it. Or going off diffs, it's a quick look at the top to see changes made to calling functions. Time is money. (I'm speaking not from the perspective of FreeBSD code base but how I interact with our code-base). Personal preferences are personal preferences I guess.

The logical approach to function position is probably based on the flow of writing the code. You start with main() and progress through. Not always, but most often. I've never seen an edict for alphabetically ordering code functions. That seems plainly odd.

Personally I don't see any problem with a swathe of prototypes or declarations at the top of the code. Is it a deal breaker if it doesn't continue that way? Only those coding in that style can truly tell you.

 

[unitrunker]

Given the prevalence of alphabetizing functions inside a file, I've abandoned the idea. It was - for me at least - a useful exercise to understand the how and why.

Variadic functions are different because you must include a prototype.

Pretty sure that's not true. Example taken from here.

/* va_start example */
#include <stdio.h>      /* printf */
#include <stdarg.h>     /* va_list, va_start, va_arg, va_end */

void PrintFloats (int n, ...)
{
  int i;
  double val;
  printf ("Printing floats:");
  va_list vl;
  va_start(vl,n);
  for (i=0;i<n;i++)
  {
    val=va_arg(vl,double);
    printf (" [%.2f]",val);
  }
  va_end(vl);
  printf ("\n");
}

int main ()
{
  PrintFloats (3,3.14159,2.71828,1.41421);
  return 0;
}

Your convention may require a prototype - the compiler does not.

I've never seen an edict for alphabetically ordering code functions. That seems plainly odd.

style(9) says to alphabetize the prototypes and #include'd header files. This seems to have been generalized to ordering functions too. I've seen this mandated in other languages so it's not new or unique to the FreeBSD code base.

 

[Mjölnir]

For public functions, the prototype has to be in the interface definition (header file). So in case you're auditing another module, you'll read only that. Still then, in many cases you have to dive into another module, but then only into parts of it, and quickly read over some of it's private functions. Then you do want prototypes, because as mark_j pointed out, it's much more convenient.

 

[unitrunker]

This must be a preference thing. When reviewing a PR of Java or C# code in BB, I don't feel like I'm missing out due to a lack of prototypes and header files.

 

[mark_j]

Given the prevalence of alphabetizing functions inside a file, I've abandoned the idea. It was - for me at least - a useful exercise to understand the how and why.



Pretty sure that's not true. Example taken from here.

/* va_start example */
#include <stdio.h>      /* printf */
#include <stdarg.h>     /* va_list, va_start, va_arg, va_end */

void PrintFloats (int n, ...)
{
  int i;
  double val;
  printf ("Printing floats:");
  va_list vl;
  va_start(vl,n);
  for (i=0;i<n;i++)
  {
    val=va_arg(vl,double);
    printf (" [%.2f]",val);
  }
  va_end(vl);
  printf ("\n");
}

int main ()
{
  PrintFloats (3,3.14159,2.71828,1.41421);
  return 0;
}

Your convention may require a prototype - the compiler does not.



style(9) says to alphabetize the prototypes and #include'd header files. This seems to have been generalized to ordering functions too. I've seen this mandated in other languages so it's not new or unique to the FreeBSD code base.

In fact the C99 standard says otherwise:

For  call  to  a  function  without  a  function  prototype  in  scope  where  the  function  is
defined  with  a  function  prototype,  either  the  prototype  ends  with  an  ellipsis  or  the
types  of  the  arguments  after  promotion  are  not  compatible  with  the  types  of  the
parameters (6.5.2.2).

This is under the heading, "undefined behavior". See also:
https://stackoverflow.com/questions/2575153/must-declare-function-prototype-in-c ("an additional note")

Of course, this is just being pedantic.

 

[unitrunker]

Of course, this is just being pedantic.

It was worth mentioning. I had to read that quote three times and then look at some examples.

More pedantic-ness:

1832 If the declarator includes a parameter type list, the list also specifies the types of all the parameters;
1833 such a declarator also serves as a function prototype for later calls to the same function in the same translation unit.

I read the above to mean that a fully defined function - code and all - also serves as a prototype. If B calls A and - either an A prototype appears before B or a full A definition appears before B - the prototype requirement is satisfied.

There's a subtlety here between a declaration, a prototype and a definition. Here's a 'declaration'. Note the absence of parameters.

int archaic_declaration_with_unknown_parameters();

I don't use function declarations. I don't think anyone has in over 30 years. If you go to the trouble of specifying the function name and return type - go ahead and fill in the parameters. Use 'void' if there are no parameters. This gets a bit messy when mixing C and C++.

int proper_prototype(void);

That declaration distinction must go back to when C function parameters were written like this:

int archaic_definition_with_parameters(x, y)
    int x;
    int a;
{
    return x * y ;
}

I'd forgotten about this last form since, well, it was deprecated a very very long time ago.

(Thanks mark_j - good discussion)

 

[mark_j]

Your original hypothesis is sound, albeit more confusing/less straight-forward than stating "All functions are prototyped somewhere."
That statement is so unary; do it or... do it.
IMO the biggest hurdle you will need to confront is habit, convention and style used in other projects around the world. If you only work on FreeBSD, this is fine, but other projects (I gave the example of NetBSD) demand, I would guess, the convention:
"All functions require a prototype".
Good luck, you argued your case well.