[clang-tidy] Some possible contributions

Clang Frontend
#1

Hello,

I am the author of a static analysis tool using Clang's LibTooling which I wrote for the open-source Colobot project (https://github.com/colobot/colobot). This tool, which I named colobot-lint (https://github.com/colobot/colobot-lint), is written using a framework loosely based on the code of clang-tidy.

Now that my little project has matured enough, I think I may contribute some of its code back to clang-tidy. However, before I create patches and send them to cfe-commits, I'd like to hear some discussion on whether you think my code is generic enough and useful enough to be included in clang-tidy.

For now I only propose the following two patches based on what I think is the most generic of what I wrote. My tool does a lot more, but I won't bore you with details here (if you're curious, it's documented in README files).

Patch proposal #1: add check for inconsistent parameter names in function declarations
This will check for instances of function (re-)declarations which differ in parameter names. For example: void foo(int a, int b, int c); in header file and void foo(int d, int e, int f) { /*...*/ } in code module file.
This check may be useful to enforce consistency in a large project, keeping declaration and definition always in sync. In extreme case, I think it may even prevent some class of bugs, as declaration may not get updated during refactoring and then a person writing code against the outdated interface, may get the wrong idea of what parameters to pass to the function.

Patch proposal #2: add check for instances of old C-style functions
This will check for instances of legacy functions which use old C-style convention of declaring all parameters at the beginning of the function:
void foo()
{
     int a, i;
     /* and later, after many lines in between, there is first use of declared variables: */
     a = bar();
     for (i = 0; i < 10; ++i) { /*...*/ }
}
It may be useful for people who have to maintain old codebases and want to find instances of such functions to refactor them to "modern" C++ style, declaring variables in the place where they are needed. This in fact is exactly what we're doing in Colobot project and I imagine there are other projects like that.

Please let me know if you think I should proceed with submitting these patches. I can also prepare other patches if you think some other parts of my code would be useful.

Best regards,
Piotr Dziwinski

#2

Hello,

I am the author of a static analysis tool using Clang's LibTooling which
I wrote for the open-source Colobot project
(https://github.com/colobot/colobot). This tool, which I named
colobot-lint (https://github.com/colobot/colobot-lint), is written using
a framework loosely based on the code of clang-tidy.

Now that my little project has matured enough, I think I may contribute
some of its code back to clang-tidy. However, before I create patches
and send them to cfe-commits, I'd like to hear some discussion on
whether you think my code is generic enough and useful enough to be
included in clang-tidy.

For now I only propose the following two patches based on what I think
is the most generic of what I wrote. My tool does a lot more, but I
won't bore you with details here (if you're curious, it's documented in
README files).

Patch proposal #1: add check for inconsistent parameter names in
function declarations
This will check for instances of function (re-)declarations which differ
in parameter names. For example: void foo(int a, int b, int c); in
header file and void foo(int d, int e, int f) { /*...*/ } in code module
file.
This check may be useful to enforce consistency in a large project,
keeping declaration and definition always in sync. In extreme case, I
think it may even prevent some class of bugs, as declaration may not get
updated during refactoring and then a person writing code against the
outdated interface, may get the wrong idea of what parameters to pass to
the function.

Patch proposal #2: add check for instances of old C-style functions
This will check for instances of legacy functions which use old C-style
convention of declaring all parameters at the beginning of the function:
void foo()
{
     int a, i;
     /* and later, after many lines in between, there is first use of
declared variables: */
     a = bar();
     for (i = 0; i < 10; ++i) { /*...*/ }
}
It may be useful for people who have to maintain old codebases and want
to find instances of such functions to refactor them to "modern" C++
style, declaring variables in the place where they are needed. This in
fact is exactly what we're doing in Colobot project and I imagine there
are other projects like that.

Please let me know if you think I should proceed with submitting these
patches. I can also prepare other patches if you think some other parts
of my code would be useful.

I also think both checkers make a lot of sense, nice ideas!

Cheers,
Kevin

#3

In article <55DF408A.5080208@gmail.com>,
    Piotr Dziwinski via cfe-dev <cfe-dev@lists.llvm.org> writes:

Patch proposal #1: add check for inconsistent parameter names in
function declarations

Nice!

Patch proposal #2: add check for instances of old C-style functions

Also nice!

I've subscribed to the reviews in phabricator. If you'd like to
discuss ideas/implementation for localizing variables in "old C-style
functions", I'm happy to participate in that either on-list or
off-list.

#4

@Legalize Adulthood: Thanks, I'll soon try to come up with some ideas for localizing variables and I will post some proposal.

Meanwhile, in case of my inconsistent parameter name check, I ran into some difficulties related to templates, as I wrote in a comment in the review.

Take this code for example:

   template<typename T>
   void foo(T a);

   template<>
   void foo(int b) {}

I want to find here mismatch between parameter name in template declaration and specialization. In my current implementation I get the following output:

   src.cpp:4:16: warning: function 'foo' has other declaration with different parameter name [readability-inconsistent-declaration-parameter-name]
   template<>void foo(int b) {}
                ^
   src.cpp:4:16: note: other declaration seen here
   template<>void foo(int b) {}
                ^
   src.cpp:4:24: note: parameter 1 is named 'b' here, but 'a' in other declaration
   template<>void foo(int b) {}

So it seems to work fine, but shows wrong location for the other declaration.

I traced it down to what I think is incorrect location reporting, or at least confusing behavior of getLocation() function.

When I visit redeclarations of template specialization, I get something that I assume is generated function declaration from the earlier template declaration. When I call dump() on it, I get:

   FunctionDecl 0x1a4dad0 <src.cpp:2:1, col:13> line:4:16 foo 'void (int)'
   >-TemplateArgument type 'int'
   `-ParmVarDecl 0x1a4da10 <line:2:10, col:12> col:12 a 'int':'int'

So everything seems fine, we get the template declaration argument name, which we can check. But when I output that diagnostic, I call getLocation(), and I get 'src.cpp:4:16', so the location of template specialization, not template declaration.

Now, of course I can work around it by calling getPrimaryTemplate()->getLocation(), but it seems a sort of hack, which applies only in this one case. So I have to ask: isn't there a universal way of getting correct location that works in all cases?

Also, another thing I would like to see in the output, would be a nice way of printing function name with template specialization arguments in diagnostic, for example: "function 'foo<int>' has other declaration (...)". Is there a standard way of doing this, or do I have to write my code for that?

Best regards,
Piotr Dziwinski