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

[Marek Kurdej via cfe-dev]

Yes, normally you should look who has committed recently and add them as
reviewers. I've added alexfh (Alexander Kornienko), cc'ed, who works on
clang-tidy.

niedz., 30 sie 2015, 12:49 Piotr Dziwinski użytkownik <piotrdz at gmail.com>
napisał:

> 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
>> 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/c77d3e29/attachment.html>