[clang-tidy] Add old style function check

[piotrdz]

Summary

This is another patch from my tool colobot-lint.

This adds a new check misc-old-style-function, which checks for instances of functions written in legacy C style.

As before, I hope I did everything according to LLVM style, including testcases and documentation.

If time allows, I will post another patch to extend this check with FixIt hints to localize variable declarations.

[Eugene.Zelenko]

I think extern "C" functions should be kept with (void).

I may be mistaken, but looks like code doesn't check for in C++ mode.

[Eugene.Zelenko]

Just for reference:

• Previous attempt: D7639.

• Bugzilla request.

[piotrdz]

@eugene: I don't understand, what does declaring function with "void" argument have in common with this review? I only check here variable declarations inside functions.

Maybe you meant my other review for inconsistent declaration parameter names? If so, this is how it behaves currently:

$ cat test.c
void allright();
void allright(void);

void notallright(int a);
void notallright(int b);

$ clang-tidy -checks='-*,readability-inconsistent-declaration-parameter-name' test.c -- -std=c11
1 warning generated.
/work/clang-trunk/test.c:4:6: warning: function 'notallright' has other declaration with different parameter name(s) [readability-inconsistent-declaration-parameter-name]
void notallright(int a);
     ^
/work/clang-trunk/test.c:5:6: note: other declaration seen here
void notallright(int b);

So I see no reason to add specific checks for functions with "void" parameter, unless you see something wrong with this behavior?

[cfe-commits]

@Aaron: Yes, I'm aware of that. I wanted to show that my check does not
take this into account.

In C++ this code is equivalent, so I think nothing should be reported,
unless we really want to get rid of that void, but I suppose this other
check does it already.

And when it comes to C, in well-formed C code, we should never see those
two definitions of allright() and allright(void), as they are
declarations of different functions. If I try to add empty bodies {} to
define them, this example won't even compile.

So anyway, I still don't understand the problem here, unless somebody
explains it in clearer terms.

Best regards,
Piotr Dziwinski

[Eugene.Zelenko]

Sorry, I was mislead by check name.

[alexfh]

A high-level comment:

It seems that the scope of the check is artificially made too narrow. The check could actually look at any POD variables declared unnecessarily far from their initialization and usages. And here the value of the check would also be much higher, if it can automatically fix the code.

I'll review the code more thoroughly later.

[alexfh]

Apparently, I forgot to submit the comments a looong time ago. Sorry for the delay.

In D12473#236401, @alexfh wrote:

A high-level comment:

It seems that the scope of the check is artificially made too narrow. The check could actually look at any POD variables declared unnecessarily far from their initialization and usages. And here the value of the check would also be much higher, if it can automatically fix the code.

I'll review the code more thoroughly later.

It looks like addressing this comment will significantly change the code, so a more thorough review should follow that step.

A few initial comments though.

clang-tidy/misc/OldStyleFunctionCheck.cpp

29

I think, this class could be replaced with an AST matcher (functionDecl(forEachDescendant(stmt( ... )))) that you could run using the clang::ast_matchers::match() function. Where the ... part would contain the matcher-based implementation of the isInteresting method.

The result would be a more local and clear code that expresses the constraints on the AST nodes you're interested in.

76

80 column limit is violated here and in a few other places. Consider clang-format'ting the code.

83

This kind of a comment is not common in llvm/clang code.

149

I'd use the if (T* x = ...) ... style for this and the next condition.

171

raw_string_ostream is a more common way to format text, and it should be more effective.

docs/clang-tidy/checks/misc-old-style-function.rst

25

Please add a newline at the end of file.

[piotrdz]

@alexfh: Ah, I forgot about this review. I will mark it as abandoned, because I have already started work on new check for localizing variables. It will have the wider scope that Eugene proposed originally, that is to move variable declarations closer to their first use. If you're curious how the code looks, you can see it on my Github fork: https://github.com/piotrdz/clang-tools-extra, as I said in discussion on cfe-dev some time ago. I will submit this new check in a new review once I finish it.