<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Doug,<div><br></div><div>Thanks so much for the feedback. I've attached an updated patch, but also a couple questions about your review below:</div><div><br><div><div>On Jan 26, 2010, at 8:05 PM, Douglas Gregor wrote:</div><br><blockquote type="cite"><div><br>+def warn_unused_function : Warning<"unused function %0">,<br>+ InGroup<UnusedFunction>, DefaultIgnore;<br><br>GCC's -Wunused-function has a bit more behavior than is in your patch. Obviously, you don't have to implement it all, but please leave FIXMEs for cases that aren't handled (static functions declared but not defined, C++ functions in anonymous namespaces).<br><br></div></blockquote><div><br></div><div>Sorry, bit new at this. For static functions declared but not defined, doesn't that fall under "missing-prototypes" instead? </div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "><font class="Apple-style-span" face="Helvetica" size="3"><span class="Apple-style-span" style="font-size: 12px; "><br></span></font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "><font class="Apple-style-span" face="Helvetica" size="3"><span class="Apple-style-span" style="font-size: 12px;">Right now I put static functions that are definitions onto a list in ActOnFunctionDeclarator. I could change this to be all static functions if you think that isn't covered by the other warning.</span></font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "><font class="Apple-style-span" face="Helvetica" size="3"><span class="Apple-style-span" style="font-size: 12px;"><br></span></font></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "><font class="Apple-style-span" face="Helvetica" size="3"><span class="Apple-style-span" style="font-size: 12px;">For the C++ functions in anonymous namespace, I assume I just check "</span></font><span class="Apple-style-span" style="white-space: pre; "><a class="code" href="http://clang.llvm.org/doxygen/classclang_1_1Decl.html#c5a3f06d897185fdf43fb6dd4ba0ae49" style="color: rgb(0, 0, 255); text-decoration: none; background-color: rgb(242, 242, 255); cursor: pointer; font-weight: normal; "><font class="Apple-style-span" face="Helvetica" size="3"><span class="Apple-style-span" style="font-size: 12px;">isInAnonymousNamespace</span></font></a><font class="Apple-style-span" face="Helvetica" size="3"><span class="Apple-style-span" style="font-size: 12px;">"</span></font></span><span class="Apple-style-span" style="white-space: pre; "><font class="Apple-style-span" face="Helvetica" size="3"><span class="Apple-style-span" style="font-size: 12px;"> and thats good? So I would check for that OR static.</span></font></span></div></div><div><br></div><div><br></div><blockquote type="cite"><div>+ <br>+ for (DeclContext::decl_iterator<br>+ D = Context.getTranslationUnitDecl()->decls_begin(),<br>+ DEnd = Context.getTranslationUnitDecl()->decls_end();<br>+ D != DEnd;<br>+ ++D) {<br>+ // Check for unused functions.<br>+ if (FunctionDecl *FD = dyn_cast<FunctionDecl>(*D)) {<br>+ if (!FD->isUsed() && !FD->hasAttr<UnusedAttr>()<br>+ && (FD->getStorageClass() == FunctionDecl::Static)) {<br>+ Diag(FD->getLocation(), diag::warn_unused_function) << FD->getDeclName();<br>+ }<br>+ }<br>+ }<br><br>This is going to be expensive, because it's walking the entire translation unit. When precompiled headers are involved, it gets *really* expensive, because it will end up deserializing every top-level declaration to perform this walk.<br><br>Instead, I suggest that ActOnFunctionDef recognize when we're defining a static function (that has not yet been used) and put it into a list of such functions. Whenever a static function is marked used for the first time, we remove it from that list. At the end of the translation unit, just walk that list and complain about everything on it. The most annoying part about this is that the list will need to be saved to the PCH file, just like tentative definitions are :(<br><br></div></blockquote><div><br></div>Makes perfect sense. I have made these changes.<br><br><blockquote type="cite"><div>There are two more cases to think about. First, we shouldn't warn about inline static functions. Second, we should figure out what using one of these static functions as an unevaluated operand should silence the warning. For example:<br><br> static int f0() { return 17; }<br> int x = sizeof(f0());<br><br>The "used" bit on f0 won't get set, so I'm guessing we'll end up warning about "f0" being unused. The list-of-unused-static-functions approach I mentioned above could avoid this problem, if we don't want the warning there. I don't really have a strong opinion on the matter.<br><br></div></blockquote><div><br></div><div>Well, this "false" warning is popping up (I added it to the test case). I'm guessing there isn't a way to prevent this?</div><br><blockquote type="cite"><div>I wonder if this warning should have a Fix-It that removes the function entirely :)<br></div></blockquote><div><br></div><div>Yes, but its possible someone may not want it removed? :)</div><div><br></div><div>-Tanya</div><div><br></div></div></div></body></html>