On 11.03.2014 20:58, Jordan Rose wrote:
On Mar 6, 2014, at 13:16 , Daniel Fahlgren <dan...@fahlgren.se
<mailto:dan...@fahlgren.se>> wrote:
Hi Jordan,
Thanks for reviewing this.
On Wed, 2014-03-05 at 14:38 -0800, Jordan Rose wrote:
I'm sorry, I managed to miss this one. Please feel free to include me
in the To or CC fields when you have analyzer patches.
I'm glad to get this check! It definitely seems useful for copy-paste
checking. Thanks for doing the timing checks in advance.
Comments:
+ if (Stmt1 && Stmt2) {
+ const Expr *Cond1 = I->getCond();
+ const Stmt *Else = Stmt2;
+ while (const IfStmt *I2 = dyn_cast<IfStmt>(Else)) {
If you use dyn_cast_or_null here, then you can drop the check for a
missing 'else' at the end of the loop (and at the beginning of the loop
too, though that's less interesting).
I've kept the check at the beginning, mostly because I think it makes
the code more readable.
+ PathDiagnosticLocation ELoc =
PathDiagnosticLocation::createBegin(Cond2,
+ BR.getSourceManager(), AC);
Please use the regular Stmt-based constructor for
PathDiagnosticLocation. We want to consider the whole condition to be
the location, not just the beginning.
+ BR.EmitBasicReport(AC->getDecl(), Checker,
+ "Identical conditions",
+ categories::LogicError,
+ "identical condition as a previous one",
ELoc, Sr);
This isn't quite valid English: two things can "be identical", and one
thing can "be identical to" another thing, but one thing can't be
"identical as" another thing. The "one" also makes me
uncomfortable...it feels weird in a compiler tool. How about
"expression is identical to previous condition"? (I'm cheating by using
"expression" to mean "condition" in this case, but it sounds better to
not repeat "condition".)
As far as test cases go, please include some cases where
- the duplicated condition is not last
- the duplicated condition is not first
- there are multiple pairs of duplicated conditions in the chain
Valid points as usual. Attached is an updated version of the patch.
Btw, who should I poke to get the list of potential checkers updated?
The page still lists different.IdenticalExprBinOp,
different.IdenticalStmtThenElse and different.CondOpIdenticalReturn as
without progress.
Ah, that's also me, probably. The original list was set up by Anna and
Anton Yartsev, for the most part, but Anna is still out and I don't
think Anton wants to be the general maintainer for it. The fastest way
to get it updated is to send me a diff; the entire analyzer site is in
www/analyzer/.
Patch committed as r203585!
Jordan
Hm, the list is really long outdated. I'll update it one of these days.
--
Anton
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
