[cfe-commits] [PATCH] Fix diagnostic reporting of previous default statement.

Douglas Gregor dgregor at apple.com
Thu Dec 23 07:40:49 PST 2010


On Dec 22, 2010, at 9:25 PM, sashan wrote:

>> 
>> 
>> Comparing just based on the line number won't work, since the two cases could
>> be in different files, or could appear on the same line.
>> SourceManager::isBeforeInTranslationUnit() is the appropriate way to compare
>> two source locations.
>> 
>> That said, I don't think this is the right solution to the problem. I think
>> that, instead, we should be processing (and probably also storing) the list of
>> case/default statements in source order rather than reverse source order, as
>> we currently are. That would address both the problem you're seeing and the
>> other problems I had previously alluded to, where we get diagnostics about
>> case values in the opposite order we expect.
>> 
>> 	- Doug
> 
> Hi Doug
> 
> Thanks for the feedback. I've attached another patch to address the issue. The
> patch inserts the switch cases in source order using
> SourceManager::isBeforeInTranslation unit to determine the order they should be
> inserted in the list. As a result it fixes the diagnostic error/warning messages
> in the case of multiple labels in a switch statement, and the overflow warning
> you alluded to previously. It passes the clang tests.
> 
> Other than that there are few questions about the attached patch that I'm not
> sure about as well as the driving philosophy behind some of the code in clang
> that maybe you can answer.
> 
> 1. I've used dyn_cast to determine if the switch case is a DefaultStmt or a
> SwitchStmt. The reason for this is so I can call the DefaultStmt::getDefaultLoc
> or CaseStmt::getCaseLoc. I believe that it would be better to make a virtual
> function in the base class, SwitchCase::getSCLoc, to handle this.


The dyn_cast is fine here. We don't want a virtual because we're slowly eliminating virtuals from the AST.

Your newer patch is technically correct. However, it has severe performance implications for switch statements with many cases, because SwitchStmt::addSwitchCase() has gone from O(1) to O(N).

I had said before:

>> That said, I don't think this is the right solution to the problem. I think
>> that, instead, we should be processing (and probably also storing) the list of
>> case/default statements in source order rather than reverse source order, as
>> we currently are.


It's still not the right solution to look at the source locations. Just reverse the list and we'll have the cases in source order.

	- Doug



More information about the cfe-commits mailing list