: Clang Static Code Analyzer does not report 'Switch' Statement as a branch condition.

> Right; what I meant was that we'd prefer to make this work rather than adding switch statements to checkBranchCondition, but right now neither one works. Sorry for the inconvenience.
> 
> Jordan

I had this same problem and ended up using an ast-matcher in a clang tool instead to get the information I wanted.

I would be interested in helping add this feature though.

I found the bug report (18175), but could use some pointers to get started on a patch.

Thanks,
Zach

Hi, Zach. That's great to hear. The code lives in ExprEngine.cpp, and you can see in functions like ExprEngine::VisitUnaryOperator (ExprEngineC.cpp) that it basically just consists of a call to the CheckerManager to run the pre-statement checks, then a loop over the results to actually perform the evaluation. In ExprEngine::processBranch the loop is already in place, so you just need to add the second callback; ExprEngine::processSwitch also has a loop but may need a bit more restructuring. ExprEngine::processIndirectGoto is the last of these and probably needs a completely new loop added.

Please feel free to ask more specific questions, and when you're ready send the first iteration of your patch to cfe-commits.

Thanks for picking this up!
Jordan

Let me see if I understand the plan:

  1. Add a runCheckersForPreStmt() call to ExprEngine::processBranch
    right after the call to runCheckersForBranchStmt(), effectively
    chaining the two together. The loop can the evaluate the nodes
    returned from both calls?

  2. Add a runCheckersForPreStmt() call to the top of
    ExprEngine::processSwitch. The loop will then need to evaluate the
    returned nodes as well as explore and evaluate the switch body as it
    currently does?

  3. Add a runCheckersForPreStmt() call to
    ExprEngine::processIndirectGoto and add a loop to evaluate any
    returned nodes.

Zach

That sounds right. Let me know if you have any more questions!

Jordan

I am finally getting a chance to look at this again. For a use-case/test-case I have added runCheckersForPreStmt to processSwitch and modified the UndefBranchChecker.cpp code to also look at SwitchStmt’s so I get warning about this kind of thing:

int a[2];

switch (a[1]) { …

undef_test.c:7:5: warning: Branch condition evaluates to a garbage value
switch (a[1]) {
^~~~~~ ~~~~

Does this sound like a reasonable test case?
Should I add the UndefBranchChecker.cpp changes and corresponding test case in misc-ps.c to the patch I submit?

Are there other use-cases I should be considering?

Zach

Let’s do it in two patches, but yes, that sounds like a great test case and new feature to add. It might be nice to add it to TraversalDumper as well—it’s just a debug checker that rarely comes in handy, but as Prashant noted it is a surprising inconsistency.

Thanks!
Jordan

+1