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

[Piotr Dziwinski via cfe-dev]

I posted these two patches for review:
  - inconsistent declaration parameter name: http://reviews.llvm.org/D12462
  - old style function: http://reviews.llvm.org/D12473

Should I also add reviewers to these reviews?

Best regards,
Piotr Dziwinski

On 2015-08-28 18:14, Piotr Dziwinski wrote:
> 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>
>>     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/20150830/f5265799/attachment.html>