<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jan 28, 2010, at 5:02 PM, Tanya Lattner wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div 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></div></blockquote><div><br></div><div>GCC's manual claims that it is under -Wunused-function, here:</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> <a href="http://gcc.gnu.org/onlinedocs/gcc-4.4.3/gcc/Warning-Options.html#Warning-Options">http://gcc.gnu.org/onlinedocs/gcc-4.4.3/gcc/Warning-Options.html#Warning-Options</a></span></div><div><br></div><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><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;">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></div></div></div></blockquote><div><br></div><div>I'm sure it's part of the -Wunused-function warning, but I don't feel strongly that we need this part of the warning. It certainly doesn't have to be in the first commit of -Wunused-function.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><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;">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></div></div></blockquote><div><br></div><div>Yes, that'll work, although there's an easier way: just check for any function with internal linkage using NamedDecl::getLinkage(). That will cover static functions and functions in anonymous namespaces, along with any wonky way we can end up with a function not visible outside of the translation unit.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><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></div></div></div></blockquote><div><br></div><div>It's possible, but I've had a change of heart on this. Why did the user bother defining the function 'f0' if the definition was never used? It seems legitimate to warn here.</div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><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></div></div></blockquote></div><div><br></div><div>A Fix-It hint here is overkill, especially given the sizeof(f0()) case above, where the function name is uttered but the definition isn't used.</div><div><br></div>A few detailed comments:<div><br></div><div><div>Index: test/Sema/warn-unused-function.c</div><div>===================================================================</div><div>--- test/Sema/warn-unused-function.c<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 0)</div><div>+++ test/Sema/warn-unused-function.c<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 0)</div><div>@@ -0,0 +1,8 @@</div><div>+// RUN: %clang_cc1 -fsyntax-only -Wunused-function -verify %s</div><div>+</div><div>+void foo() {}</div><div>+static void f2() {} </div><div>+static void f1() {f2();} // expected-warning{{unused}}</div><div>+</div><div>+static int f0() { return 17; }</div><div>+int x = sizeof(f0());</div><div><br></div><div>It would be good to also check a few cases with multiple declarations of the function, e.g.,</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>static void f3();</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>extern void f3() { } // this is static, should complain</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>inline static void f4();</div><div> <span class="Apple-tab-span" style="white-space:pre"> </span>void f4() { } // inline, should not complain</div><div><br></div><div><br></div><div><div>+ </div><div>+ // If there were any unused static functions, deserialize them and add to</div><div>+ // Seam's list of unused static functions.</div><div><br></div><div>Typo "Seam"</div><div><br></div><div><div>+ </div><div>+ // Keep track of static, non-inlined function definitions that</div><div>+ // have not been used. We will warn later.</div><div>+ if (!NewFD->isInvalidDecl() && IsFunctionDefinition </div><div>+ && !isInline && SC == FunctionDecl::Static</div><div>+ && !NewFD->isUsed())</div><div>+ UnusedStaticFuncs.push_back(NewFD);</div><div>+ </div><div><br></div><div>As I mentioned above, I suggest using NewFD->getLinkage() == NamedDecl::InternalLinkage.</div><div>Also, I suggest using NewFD->isInlined() rather than isInline, because the call will also consider previous declarations.</div></div></div><div><br></div><div><div>+ </div><div>+ // Remove used function from our list of static unused functions to prevent</div><div>+ // the unused function warning from triggering.</div><div>+ if (Function->getStorageClass() == FunctionDecl::Static) {</div><div>+ std::vector<FunctionDecl*>::iterator I = std::find(UnusedStaticFuncs.begin(),</div><div>+ UnusedStaticFuncs.end(),</div><div>+ Function);</div><div>+ if (I != UnusedStaticFuncs.end())</div><div>+ UnusedStaticFuncs.erase(I);</div><div>+ }</div><div><br></div><div>This is linear in the number of static function definitions. I don't know if that's a problem in practice, but I have an alternative suggestion: don't bother to remove anything from UnusedStaticFuncs here. Instead, at the beginning of Sema::ActOnEndOfTranslationUnit(), go through the list once to remove all of the static function definitions that are marked "used" (or for which any of their redeclarations was used).</div><div><br></div></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div></div></body></html>