[cfe-dev] [clang-tidy] Some possible contributions

[Piotr Dziwinski via cfe-dev]

Hello,

Thanks for all replies so far. I'm glad that you agree that my code 
would be useful. I'll post some patches for review sometime during this 
weekend.

I also got a private reply from Eugene Zelenko, who pointed me to his 
feature request for localizing variables: 
https://llvm.org/bugs/show_bug.cgi?id=21992.
This feature request would be partly implemented by my check for old 
C-style functions. To fully implement the proposed behavior, I would 
need to extend my code with FixIt hints to enable automated refactoring. 
Since that would require quite a bit of work, I would propose to first 
commit only my basic check for finding such functions and once I find 
enough time, I would add support for refactoring.

On 2015-08-28 10:16, Marek Kurdej wrote:
>
> I've had a quick look at colobot-lint and, IMO, it would be nice to 
> add some other checks, e.g. NakedNew, NakedDelete, ImplicitBoolCast 
> may be of interest.

Regarding NakedNew and NakedDelete, I wasn't sure if there were similar 
checks already implemented or proposed for implementation. I think that 
if I should post a patch here, I would join these two checks to single 
check called for instance ManualMemoryManagement.

As for ImplicitBoolCast, I think it's also worth to consider. It's very 
useful in case of such unfortunate "features" of C++ as implicit bool -> 
float casts, which  caused quite a bit of problems in Colobot project at 
one point.
However, this check also restricts pointer -> bool casts, so it would 
find issue with such code:
void foo(int* ptr)
{
    if (ptr) // warning: implicit int* -> bool cast
    {  /* ... */ }
}
I personally don't like such style, so I added this restriction, but 
there is nothing really wrong in such code. I guess it's more a matter 
of taste, so perhaps this conversion can be enabled/disabled with 
appropriate config option.


On 2015-08-28 10:16, Marek Kurdej wrote:
>
> BTW, does your InconsistentDeclarationParameterNameRule handles unused 
> parameters (i.e. those that have names in the declaration, but are 
> marked /*unused*/ or something along these lines in the definition)?
No, it does not. Unused (unnamed) parameters are currently ignored in 
comparison. I wanted to check for such parameters which names are 
commented out, but unless I'm missing something, there is no easy way 
right now to match parameter declaration to a comment next to it, as we 
have access to comments only during the preprocessing phase. Of course, 
I can sort of "hack" it, by getting character buffer from SourceManager 
and scanning characters of source code, but it seems a very crude way to 
handle this. (Well, I was forced to use such mechanism for my 
BlockPlacement check, but that's another topic.)


Best regards,
Piotr Dziwinski

On 2015-08-28 10:16, Marek Kurdej wrote:
> Hi Piotr,
>
> I think that it would be great to add your checks to clang-tidy.
> I've had a quick look at colobot-lint and, IMO, it would be nice to 
> add some other checks, e.g. NakedNew, NakedDelete, ImplicitBoolCast 
> may be of interest.
>
> BTW, does your InconsistentDeclarationParameterNameRule handles unused 
> parameters (i.e. those that have names in the declaration, but are 
> marked /*unused*/ or something along these lines in the definition)?
>
> Regards,
> Marek Kurdej
>
>
>     ---------- Wiadomość przekazana dalej ----------
>     From: Piotr Dziwinski via cfe-dev <cfe-dev at lists.llvm.org
>     <mailto:cfe-dev at lists.llvm.org>>
>     To: cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
>     Cc:
>     Date: Thu, 27 Aug 2015 18:53:30 +0200
>     Subject: [cfe-dev] [clang-tidy] Some possible contributions
>     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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150828/a111d0a7/attachment.html>