diffblue / cbmc

C Bounded Model Checker
https://diffblue.github.io/cbmc
Other
848 stars 264 forks source link

Use of anonymous namespaces requires further work/discussion #932

Closed tautschnig closed 7 years ago

tautschnig commented 7 years ago

See the post-merge discussion in #929, in particular @peterschrammel's statement.

pkesseli commented 7 years ago

Do we need to make classes (or other kinds of declarations) local to a translation unit?

Yes, I do that frequently. Simple example is if I'm using a small expr_visitort implementation which is only relevant in the current .cpp file, I put it in an anonymous namespace.

Any pointers to that overuse? It's placement at the beginning of a function definition tells you exactly one thing: this function is local to the translation unit.

I'm referring to its differing semantics if it's in a namespace scope vs. when applied to class members. Also, I just find it repetitive:

static int x = 0;
static double y = 1;
static const char DATA[] = "asdf";

Instead, I prefer:

namespace
{
int x = 0;
double y = 1;
const char DATA[] = "asdf";
}

I'm ok with that, as long as a code review on github will show the beginning of the anonymous namespace together with the function definition. Which effectively limites the number of lines to 5.

Limiting their size to 5 lines seems excessive. If we require the semantics of every piece of code to be contained within 5 lines of context, we need to stop using a lot of C++ features (e.g. class access specifiers, virtual functions, ...) :-) .

pkesseli commented 7 years ago

For what it's worth, I don't think it makes sense to refactor out all uses of static for TU-local content. The above comment shows some examples where anonymous namespaces are intuitive or even necessary, but I wouldn't ban the use of static for this purpose either. There are cases, such as single variables or functions, where static is the simpler choice.

I suggest we form a basic guideline along the following:

Basically saying if you only have a single variable or function then use static, otherwise you're allowed to use anonymous namespaces.

CC: @peterschrammel

tautschnig commented 7 years ago

I believe this should be closed via #1031.

pkesseli commented 7 years ago

That's correct. In the CEGIS project, I currently apply the following policy:

The current CBMC coding standard permits more liberal use of anonymous namespaces, but I've come around to prefer static for functions and variables after trying it for a while.