<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jan 23, 2010, at 11:59 AM, Michal wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Thank you both for replies. Attaching a newer, but not yet finished version.<br><br><br><blockquote type="cite"><br></blockquote><blockquote type="cite">+ // Iterate over Enum values and check which are missing in switch<br></blockquote><blockquote type="cite">+ CaseValsTy::const_iterator CI = CaseVals.begin();<br></blockquote><blockquote type="cite">+ for(EnumDecl::enumerator_iterator EI = ED->enumerator_begin(); EI !=<br></blockquote><blockquote type="cite">ED->enumerator_end(); EI++) {<br></blockquote><blockquote type="cite">+ if(CI == CaseVals.end() || EI->getInitVal().slt(CI->first))<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Why use "slt" here? The values aren't guaranteed to be signed.<br></blockquote>I misunderstood slt code. I already asked here how to compare ASPInts but<br>I am not sure whether it is appropriate to use your answer here. Are you ok with<br>adding another function that copies and extends an ASPInt, and calling it for<br>each comparison?<br>As for now I am using ordinary <,==, etc, which obviously breaks if enum has no<br>negative values.<br></div></blockquote><div><br></div><div>Every enumeration has an underlying integral type. You can ask that type whether it is signed or unsigned, and use ASTContext's getTypeSize() to determine its size (in bits). From that information, construct the APSInts and compare them with ==.</div><br><blockquote type="cite"><div><blockquote type="cite"> - There may be multiple enumeration constants with the same value<br></blockquote><blockquote type="cite">(FirstDecl = 1, VarDecl = 1, FunctionDecl = 2, etc.); please make sure that<br></blockquote><blockquote type="cite">we do not warn more than once if both FirstDecl and VarDecl are missing, and<br></blockquote><blockquote type="cite">that the presence of such enumerators does not cause spurious warnings.<br></blockquote><blockquote type="cite">(std::unique could eliminate duplicates in the list of enumerators).<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> - Try to make sure that recovery is good; if we complain about the second<br></blockquote><blockquote type="cite">enumerator missing, it shouldn't cause a cascade of failures.<br></blockquote><blockquote type="cite"><br></blockquote>Not sure if I understood you correctly, if a switch misses more than two<br>different values should we report only the first two? If so, I think<br>that at least<br>there should be a warning notifying that there are more, but just not<br>printed, missing<br>enum constants.<font class="Apple-style-span" color="#000000"><font class="Apple-style-span" color="#144FAE"><br></font></font></div></blockquote><br></div><div>Sorry, I wasn't clear. All I meant is that, when I looked at the previous patch, it looked like it would get confused after seeing the first missing enum value and then print warnings about missing enum values that were actually there. I may have misunderstood the code.</div><div><br></div><div><div>+ EnumVals.append(ED->enumerator_begin(), ED->enumerator_end());</div><div>+ std::sort(EnumVals.begin(), EnumVals.end(), CmpEnumVals);</div><div><br></div><div>Good, this is sorting by enumerator values. Please use std::stable_sort so that we get a deterministic sort across platforms, ensuring that we always complain about the first missing enumerator with a given value. For example, if we have:</div><div><br></div><div> enum Blah {</div><div> DeclRefExpr = 20,</div><div> // ...</div><div> FirstExpr = 20</div><div> };</div><div><br></div><div>we'll complain about DeclRefExpr when it's missing, not FirstExpr.</div><div><br></div><div><div>+ Diag(CI->second->getLHS()->getExprLoc(), diag::not_in_enum) << ED->getNameAsCString();</div><div><br></div><div>Instead of ED->getNameAsCString(), please use ED->getDeclName(). It doesn't matter for enums (which can't have funky names), but it's good style.</div><div><br></div><div>This patch is looking good. I look forward to reviewing the final version.</div><div><br></div></div></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div></body></html>