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

Mon Aug 31 08:47:48 PDT 2015

Hi Piotr,

Nice that you have some clang-tidy checks to contribute. I'm glad to review
the patches. Please assign clang-tidy reviews to me and put cfe-commits to
the "Subscribers" field.

On a side note: it's convenient to be able to see more context in the
diffs. Depending on your setup, you could use `diff -U10000`, `svn diff
--diff-cmd diff -x -U10000`, `git diff -U10000` or a similar option, or use
the arcanist
<https://secure.phabricator.com/book/phabricator/article/arcanist/> tool,
which I personally find convenient.

On Sun, Aug 30, 2015 at 12:49 PM, Piotr Dziwinski via cfe-dev <
cfe-dev at lists.llvm.org> wrote:

> 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>
> 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.
>
> Clang's AST can't contain every little piece of information about the
code, as it would inevitably lead to a blow up in the memory usage. Thus
sometimes we have to resort to re-lexing parts of the buffer to fetch
missing pieces. It's a normal practice in clang-based tools and here it's a
reasonable way to find parameter names in comments. Actually, there's the
getCommentsInRange method in clang-tidy/misc/ArgumentCommentCheck.cpp that
does almost what you need. If you find it useful for your check, it can be
moved to clang-tidy/utils/ and reused from there.

-- Alex

> (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>
>> 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>
>> 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
>>
>
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150831/75975b95/attachment.html>