<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hello,<br>
<br>
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.<br>
<br>
I also got a private reply from Eugene Zelenko, who pointed me to
his feature request for localizing variables:
<a class="moz-txt-link-freetext" href="https://llvm.org/bugs/show_bug.cgi?id=21992">https://llvm.org/bugs/show_bug.cgi?id=21992</a>.<br>
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.<br>
<br>
<div class="moz-cite-prefix">On 2015-08-28 10:16, Marek Kurdej
wrote:<br>
</div>
<blockquote
cite="mid:CAPoJmgw39qyJUoi57L6ZnBXxqcxk_UKggvY3EsfXxcPar3gkAw@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote"><br>
<div>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.</div>
</div>
</div>
</div>
</blockquote>
<br>
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.<br>
<br>
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.<br>
However, this check also restricts pointer -> bool casts, so it
would find issue with such code:<br>
void foo(int* ptr)<br>
{<br>
if (ptr) // warning: implicit int* -> bool cast<br>
{ /* ... */ }<br>
}<br>
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.<br>
<br>
<br>
<div class="moz-cite-prefix">On 2015-08-28 10:16, Marek Kurdej
wrote:<br>
</div>
<blockquote
cite="mid:CAPoJmgw39qyJUoi57L6ZnBXxqcxk_UKggvY3EsfXxcPar3gkAw@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote"><br>
<div>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)?<br>
</div>
</div>
</div>
</div>
</blockquote>
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.)<br>
<br>
<br>
Best regards,<br>
Piotr Dziwinski<br>
<br>
<div class="moz-cite-prefix">On 2015-08-28 10:16, Marek Kurdej
wrote:<br>
</div>
<blockquote
cite="mid:CAPoJmgw39qyJUoi57L6ZnBXxqcxk_UKggvY3EsfXxcPar3gkAw@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div>Hi Piotr,</div>
<div><br>
</div>
<div>I think that it would be great to add your checks to
clang-tidy.</div>
<div>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.</div>
<div><br>
</div>
<div>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)?</div>
<div><br>
</div>
<div>Regards,</div>
<div>Marek Kurdej</div>
<div><br>
</div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">----------
Wiadomość przekazana dalej ----------<br>
From: Piotr Dziwinski via cfe-dev <<a
moz-do-not-send="true"
href="mailto:cfe-dev@lists.llvm.org"><a class="moz-txt-link-abbreviated" href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a></a>><br>
To: <a moz-do-not-send="true"
href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a><br>
Cc: <br>
Date: Thu, 27 Aug 2015 18:53:30 +0200<br>
Subject: [cfe-dev] [clang-tidy] Some possible
contributions<br>
Hello,<br>
<br>
I am the author of a static analysis tool using Clang's
LibTooling which I wrote for the open-source Colobot
project (<a moz-do-not-send="true"
href="https://github.com/colobot/colobot"
rel="noreferrer" target="_blank">https://github.com/colobot/colobot</a>).
This tool, which I named colobot-lint (<a
moz-do-not-send="true"
href="https://github.com/colobot/colobot-lint"
rel="noreferrer" target="_blank"><a class="moz-txt-link-freetext" href="https://github.com/colobot/colobot-lint">https://github.com/colobot/colobot-lint</a></a>),
is written using a framework loosely based on the code of
clang-tidy.<br>
<br>
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.<br>
<br>
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).<br>
<br>
<br>
Patch proposal #1: add check for inconsistent parameter
names in function declarations<br>
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.<br>
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.<br>
<br>
<br>
Patch proposal #2: add check for instances of old C-style
functions<br>
This will check for instances of legacy functions which
use old C-style convention of declaring all parameters at
the beginning of the function:<br>
void foo()<br>
{<br>
int a, i;<br>
/* and later, after many lines in between, there is
first use of declared variables: */<br>
a = bar();<br>
for (i = 0; i < 10; ++i) { /*...*/ }<br>
}<br>
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.<br>
<br>
<br>
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.<br>
<br>
Best regards,<br>
Piotr Dziwinski<br>
</blockquote>
</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>