Anna, 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!
Adam On Fri, Apr 19, 2013 at 7:51 PM, Anna Zaks <[email protected]> wrote: > > On Apr 19, 2013, at 4:09 PM, Adam Schnitzer <[email protected]> wrote: > > 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? > > Adam, > > 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 > 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. > > 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. > (Note that the existing Malloc and SecKeyChainAPI checkers are similar to > this one, but generalizing them all in one could be a separate project.) > > As you are working on this, please, send incremental patches to the > cfe-commits list (and CC me and Jordan). > > Cheers, > Anna. > > Also, I'm not sure if anyone has had a chance to take a look yet, but I've > been having a little problem <http://llvm.org/bugs/show_bug.cgi?id=15774> with > the BoolAssignmentChecker. I'm a bit stuck at the moment, but would be > interested in extending that as well. > > > Adam > > On Thu, Apr 18, 2013 at 2:56 PM, Anna Zaks <[email protected]> wrote: > >> On Apr 17, 2013, at 8:33 PM, Adam Schnitzer <[email protected]> wrote: >> >> Jordan, Thank you very much for the feedback, I have a few comments. >> >> *(1) Despite being on our list, this is probably better suited to a >>> compiler warning.* >>> >>> >> 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. >> >> >> Sorry for having an under-specified checker in the list! >> >> >> >>> *(2) Despite being on our list, "unsigned" isn't actually the >>> interesting thing to check.* >>> >>> >> 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: >> >> int array[] = { >> -3, >> -2, >> -1, >> +1 >> }; >> >> 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: >> >> char a = 'A'; >> cout << a << " "; // print A >> cout << +a; // prints numerical value of 'A' >> >> >> 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. >> >> 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. >> >> 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. >> >> >> *(3) Macros and templates make this tricky.* >>> >> >> 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. >> >> >> 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. >> >> >> 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. >> >> On an unrelated note, do you have any recommendations for what might be a >> approachable second checker? >> >> >> 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.. >> >> Jordan, Anton, what do you think? >> >> >> Adam >> >> >> >> >>> >>> >> >>> On Apr 12, 2013, at 23:53 , Adam Schnitzer <[email protected]> wrote: >>> >>> This patch is an implementation of the >>> proposed "different.UnaryPlusWithUnsigned", which I implemented >>> as "alpha.deadcode.UnaryPlusWithUnsigned". >>> >>> It is implemented as a simple AST checker. However, it seems that unary >>> '+' is often removed from the AST >>> as dead code. So some of the basic test cases don't work. >>> >>> This is my first (real) patch, so any feedback or criticism is >>> appreciated. >>> >>> Adam Schnitzer >>> <UnaryPlusChecker.patch> >>> >>> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
