<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hello again!<br>
<br>
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.<br>
<br>
However, I did not forget about it, and I had some time recently to
work on these patches:<br>
- <a href="http://reviews.llvm.org/D13634">http://reviews.llvm.org/D13634</a>
- updating documentation of inconsistent declaration parameters
check<br>
- <a href="http://reviews.llvm.org/D13635">http://reviews.llvm.org/D13635</a>
- new check: implicit bool casts<br>
<br>
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.<br>
<br>
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?<br>
<br>
As regards old-style function check, or more broadly: localizing
variables, (<a moz-do-not-send="true"
href="http://reviews.llvm.org/D12473" target="_blank">http://reviews.llvm.org/D12473</a>),
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.<br>
<br>
Best regards,<br>
Piotr Dziwinski<br>
<br>
<br>
<div class="moz-cite-prefix">On 2015-08-30 16:28, Marek Kurdej
wrote:<br>
</div>
<blockquote
cite="mid:CAPoJmgy8O=RuzmZFogNjzmC3+w8xTVK9DnoyiX+KHLbqkUX_yQ@mail.gmail.com"
type="cite">
<p dir="ltr">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.</p>
<br>
<div class="gmail_quote">
<div dir="ltr">niedz., 30 sie 2015, 12:49 Piotr Dziwinski
użytkownik <<a moz-do-not-send="true"
href="mailto:piotrdz@gmail.com" target="_blank">piotrdz@gmail.com</a>>
napisał:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF"> I posted these two
patches for review:<br>
- inconsistent declaration parameter name: <a
moz-do-not-send="true"
href="http://reviews.llvm.org/D12462" target="_blank"><a class="moz-txt-link-freetext" href="http://reviews.llvm.org/D12462">http://reviews.llvm.org/D12462</a></a><br>
- old style function: <a moz-do-not-send="true"
href="http://reviews.llvm.org/D12473" target="_blank">http://reviews.llvm.org/D12473</a><br>
<br>
Should I also add reviewers to these reviews?<br>
<br>
Best regards,<br>
Piotr Dziwinski</div>
<div text="#000000" bgcolor="#FFFFFF"><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 moz-do-not-send="true"
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.
(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
moz-do-not-send="true"
href="mailto:cfe-dev@lists.llvm.org"
target="_blank"><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"
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
moz-do-not-send="true"
href="https://github.com/colobot/colobot"
rel="noreferrer" target="_blank"><a class="moz-txt-link-freetext" href="https://github.com/colobot/colobot">https://github.com/colobot/colobot</a></a>).
This tool, which I named colobot-lint (<a
moz-do-not-send="true"
href="https://github.com/colobot/colobot-lint"
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>
</blockquote>
<br>
</div>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>