<div dir="ltr">Hi Piotr,<div><br></div><div>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.</div><div><br></div><div>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 <a href="https://secure.phabricator.com/book/phabricator/article/arcanist/" target="_blank">arcanist</a> tool, which I personally find convenient.<br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Aug 30, 2015 at 12:49 PM, Piotr Dziwinski via cfe-dev <span dir="ltr"><<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>></span> wrote:<br><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">
<div text="#000000" bgcolor="#FFFFFF">
I posted these two patches for review:<br>
- inconsistent declaration parameter name:
<a href="http://reviews.llvm.org/D12462" target="_blank">http://reviews.llvm.org/D12462</a><br>
- old style function: <a href="http://reviews.llvm.org/D12473" target="_blank">http://reviews.llvm.org/D12473</a><br>
<br>
Should I also add reviewers to these reviews? </div></blockquote><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"><div text="#000000" bgcolor="#FFFFFF">
<br>
Best regards,<br>
Piotr Dziwinski<div><div><br>
<br>
<div>On 2015-08-28 18:14, Piotr Dziwinski
wrote:<br>
</div>
<blockquote type="cite">
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 href="https://llvm.org/bugs/show_bug.cgi?id=21992" target="_blank"></a><a href="https://llvm.org/bugs/show_bug.cgi?id=21992" target="_blank">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>On 2015-08-28 10:16, Marek Kurdej
wrote:<br>
</div>
<blockquote 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>On 2015-08-28 10:16, Marek Kurdej
wrote:<br>
</div>
<blockquote 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. </blockquote></div></div></div></blockquote><div>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.</div><div><br></div><div>-- Alex</div><div><br></div><div> </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"><div text="#000000" bgcolor="#FFFFFF"><div><div><blockquote type="cite">(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>On 2015-08-28 10:16, Marek Kurdej
wrote:<br>
</div>
<blockquote 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 href="mailto:cfe-dev@lists.llvm.org" target="_blank"></a><a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>><br>
To: <a href="mailto:cfe-dev@lists.llvm.org" target="_blank">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 href="https://github.com/colobot/colobot" rel="noreferrer" target="_blank">https://github.com/colobot/colobot</a>).
This tool, which I named colobot-lint (<a href="https://github.com/colobot/colobot-lint" target="_blank"></a><a href="https://github.com/colobot/colobot-lint" target="_blank">https://github.com/colobot/colobot-lint</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>
</blockquote>
<br>
</div></div></div>
<br>_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
<br></blockquote></div><br><br>
</div></div></div>