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?
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
