[cfe-dev] [clang-tidy] Some possible contributions
Piotr Dziwinski via cfe-dev
cfe-dev at lists.llvm.org
Sun Oct 11 05:13:03 PDT 2015
Hello again!
Unfortunately, I did not have as much time as I thought I had, so my
work on porting my checks from colobot-lint to clang-tidy came to a
standstill.
However, I did not forget about it, and I had some time recently to work
on these patches:
- http://reviews.llvm.org/D13634 - updating documentation of
inconsistent declaration parameters check
- http://reviews.llvm.org/D13635 - new check: implicit bool casts
As I mentioned previously, my check for implicit bool casts, may not be
suited to some coding styles. Personally, I like to avoid implicit bool
casts whenever possible, as I find code that relies on them more
difficult to read. Sometimes this even leads to hidden bugs, or if not
bugs, then at least inconsitent code.
I would like to hear, what you think about that. Should some of these
styles, e.g. relying on pointer to bool conversion in "if (pointer) {}",
be allowed? Maybe I should add config option to enable/disable my check
in such cases?
As regards old-style function check, or more broadly: localizing
variables, (http://reviews.llvm.org/D12473), it turned out to be more
difficult than I thought at first, and still have not made much
progress. However, in the meantime, working on other pieces of code, I
have become more familiar with Clang codebase, so I think it will be
easier for me now. This is what I'll focus on from now on.
Best regards,
Piotr Dziwinski
On 2015-08-30 16:28, Marek Kurdej wrote:
>
> 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 <mailto: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
>>> <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/20151011/22c35d73/attachment.html>
More information about the cfe-dev
mailing list