Anna,<div><br></div><div>That makes sense, and sounds like a great project to get started on. I will notify cfe-dev and send in incremental patches. Thanks for all the help!</div><div><br></div><div>Adam<br><div><div><br><div class="gmail_quote">
On Fri, Apr 19, 2013 at 7:51 PM, Anna Zaks <span dir="ltr"><<a href="mailto:ganna@apple.com" target="_blank">ganna@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><br><div><div class="im"><div>On Apr 19, 2013, at 4:09 PM, Adam Schnitzer <<a href="mailto:adamschn@umich.edu" target="_blank">adamschn@umich.edu</a>> wrote:</div><br><blockquote type="cite">
<div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">Anna, working on the StreamChecker sounds like a great idea. In terms of work left to do on it, I'm assuming it's mostly cleanup and testing? It also seems like the pattern of ensuring a resource should be opened/closed once seems relevant beyond streams. Maybe it would make sense to refactor it to include a generic base checker which could be reused?<div>
<br></div></div></blockquote></div>Adam,</div><div><br></div><div>There are 2 files: SimpleStreamChecker.cpp, which was used for the talk and StreamChecker.cpp, which has been written before the talk and does not follow some of the "good practices" on how a checker should be written. I would take SimpleStreamChecker as the base and extend it to handle more APIs, like the ones StreamChecker is checking. Another enhancement would be to make a bug report more expressive. For example, you could mention where a file was opened when reporting a resource leak, etc. Finally, you would run the checker over a bunch of real code and see what else needs to be </div>
<div>improved. Please, send an email to the cfe-dev list stating that you are going to work on this checker as I've recommended this as a starter project to several people.</div><div><br></div><div>It is true that there are other APIs that require similar checks. However, I would start with a specific checker and generalize it later on. You could try to develop it so that it could be easily generalized.</div>
<div>(Note that the existing Malloc and SecKeyChainAPI checkers are similar to this one, but generalizing them all in one could be a separate project.) </div><div><br></div><div>As you are working on this, please, send incremental patches to the cfe-commits list (and CC me and Jordan).</div>
<div><br></div><div>Cheers,</div><div>Anna.<div><div class="h5"><br><blockquote type="cite"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div>
Also, I'm not sure if anyone has had a chance to take a look yet, but I've been having a<span> </span><a href="http://llvm.org/bugs/show_bug.cgi?id=15774" target="_blank">little problem</a><span> </span>with the BoolAssignmentChecker. I'm a bit stuck at the moment, but would be interested in extending that as well.</div>
</div></blockquote><blockquote type="cite"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div><br></div><div>Adam<br><br><div class="gmail_quote">
On Thu, Apr 18, 2013 at 2:56 PM, Anna Zaks<span> </span><span dir="ltr"><<a href="mailto:ganna@apple.com" target="_blank">ganna@apple.com</a>></span><span> </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 style="word-wrap:break-word"><div><div><div>On Apr 17, 2013, at 8:33 PM, Adam Schnitzer <<a href="mailto:adamschn@umich.edu" target="_blank">adamschn@umich.edu</a>> wrote:</div><br><blockquote type="cite"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
Jordan, Thank you very much for the feedback, I have a few comments.<div><br></div><div><div class="gmail_quote"><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 style="word-wrap:break-word"><div><i>(1) Despite being on our list, this is probably better suited to a compiler warning.</i></div><div><br></div></div></blockquote><div><br></div><div>I agree, this warning might be better as a compiler warning. I chose to implement this checker as a mainly to learn a bit about the analyzer. This one was on the list and seemed like a good place to get started.</div>
</div></div></div></blockquote><div><br></div></div><div>Sorry for having an under-specified checker in the list!</div><div><div><br></div><blockquote type="cite"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div class="gmail_quote"><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 style="word-wrap:break-word">
<div><i>(2) Despite being on our list, "unsigned" isn't actually the interesting thing to check.</i></div><div><br></div></div></blockquote><div><br></div><div>When I was reading the checker suggestion, I interpreted the purpose to be a more conservative version of a check for unary '+', which, arguably, is often dead code. For example, I have seen structures like this fairly commonly:</div>
<div><br></div><div>int array[] = {</div><div> <span> </span>-3,</div><div> <span> </span>-2,</div><div> <span> </span>-1,</div><div> <span> </span>+1</div><div>};</div><div><br></div><div>Where the '+' is used for alignment, which we wouldn't want to warn about. However, if that array was changed to unsigned, it might be a legitimate warning. I thought the assumption was there's at least a decent chance a unary '+' on unsigned is dead code. The place where I most commonly it pop up was legitimate:</div>
<div><br></div><div>char a = 'A';</div><div>cout << a << " "; // print A</div><div>cout << +a; // prints numerical value of 'A'</div><div><br></div></div></div></blockquote><div>
<br></div></div>This is in line with what Jordan had mentioned. If we are writing a checker/warning that warns on redundant operations (or operations that have no effect), we would not warn in this case as there will be a promotion.</div>
<div><br></div><div>It should be possible to write a check/warning that finds cases where the unary plus has no effect by examining the AST. It could be a candidate for a compiler warning, since the check could be fast and does not require path-sensitive program exploration. Generally, compiler warnings are better because they reach more users. If you are interested, you could reach out to the clang community and see if there is an interest in such a warning. You could also write it as a checker first, see what is the false positive rate and rewrite this as a compiler warning is it seems useful.</div>
<div><div><br><blockquote type="cite"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote"><div>But I hadn't considered the checker was intended to target idempotent or erroneous promotions. If that is the intent, then it seems challenging to decide whether an expression is dead code, or to "force a load", as you put it.</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"><div style="word-wrap:break-word">
<i>(3) Macros and templates make this tricky.</i></div></blockquote><div><br></div><div>I thought the that this might have been the reason why the checker was listed as a potential checker, rather than a compiler warning. It does seem like a fairly "noisy" warning. I have run it through some student code. Unfortunately all warnings it produced were false positives, with the exception of one situation similar to the one above.</div>
<div><br></div></div></div></blockquote><div><br></div></div><div>If you are interested in writing the warning, you could look at your results and see if the suggested changes would get rid of the false positives.</div><div>
<br><blockquote type="cite"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote"><div><br></div><div>At this point, I'd be fine with throwing this checker out, as its utility does seem quite limited. If anyone has any ideas on how this checker can be improved to be more useful, I would be interested to hear.</div>
<div><br></div><div>On an unrelated note, do you have any recommendations for what might be a approachable second checker?</div></div></div></blockquote><div><br></div></div>I think the i++ checker that you've proposed originally would be good. You could also productize the StreamChecker, which would be path-sensitive and not too difficult. Note sure if anyone else is working on that..</div>
<div><br></div><div>Jordan, Anton, what do you think?</div><div><br><blockquote type="cite"><div style="letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote">
<div><br></div><div>Adam</div><div><br></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 style="word-wrap:break-word"> </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 style="word-wrap:break-word">
<div></div><br><div><div><div>On Apr 12, 2013, at 23:53 , Adam Schnitzer <<a href="mailto:adamschn@umich.edu" target="_blank">adamschn@umich.edu</a>> wrote:</div><br></div><blockquote type="cite"><div>This patch is an implementation of the proposed "different.UnaryPlusWithUnsigned", which I implemented<div>
as "alpha.deadcode.UnaryPlusWithUnsigned".<div><br></div><div>It is implemented as a simple AST checker. However, it seems that unary '+' is often removed from the AST</div></div><div>as dead code. So some of the basic test cases don't work.</div>
<div><br></div><div>This is my first (real) patch, so any feedback or criticism is appreciated.</div><div><br></div><div>Adam Schnitzer</div></div><span><UnaryPlusChecker.patch></span></blockquote></div></div></blockquote>
</div></div></blockquote></div></div></blockquote></div></div></div></blockquote></div></div></div><br></div></blockquote></div><br></div></div></div>