On Jan 28, 2010, at 5:02 PM, Tanya Lattner wrote:
> Doug,
>
> Thanks so much for the feedback. I've attached an updated patch, but also a
> couple questions about your review below:
>
> On Jan 26, 2010, at 8:05 PM, Douglas Gregor wrote:
>
>>
>> +def warn_unused_function : Warning<"unused function %0">,
>> + InGroup<UnusedFunction>, DefaultIgnore;
>>
>> 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).
>>
>
> Sorry, bit new at this. For static functions declared but not defined,
> doesn't that fall under "missing-prototypes" instead?
GCC's manual claims that it is under -Wunused-function, here:
http://gcc.gnu.org/onlinedocs/gcc-4.4.3/gcc/Warning-Options.html#Warning-Options
> 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.
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.
> For the C++ functions in anonymous namespace, I assume I just check
> "isInAnonymousNamespace" and thats good? So I would check for that OR static.
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.
>> 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:
>>
>> static int f0() { return 17; }
>> int x = sizeof(f0());
>>
>> 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.
>>
>
> 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?
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.
>> I wonder if this warning should have a Fix-It that removes the function
>> entirely :)
>
> Yes, but its possible someone may not want it removed? :)
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.
A few detailed comments:
Index: test/Sema/warn-unused-function.c
===================================================================
--- test/Sema/warn-unused-function.c (revision 0)
+++ test/Sema/warn-unused-function.c (revision 0)
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -fsyntax-only -Wunused-function -verify %s
+
+void foo() {}
+static void f2() {}
+static void f1() {f2();} // expected-warning{{unused}}
+
+static int f0() { return 17; }
+int x = sizeof(f0());
It would be good to also check a few cases with multiple declarations of the
function, e.g.,
static void f3();
extern void f3() { } // this is static, should complain
inline static void f4();
void f4() { } // inline, should not complain
+
+ // If there were any unused static functions, deserialize them and add to
+ // Seam's list of unused static functions.
Typo "Seam"
+
+ // Keep track of static, non-inlined function definitions that
+ // have not been used. We will warn later.
+ if (!NewFD->isInvalidDecl() && IsFunctionDefinition
+ && !isInline && SC == FunctionDecl::Static
+ && !NewFD->isUsed())
+ UnusedStaticFuncs.push_back(NewFD);
+
As I mentioned above, I suggest using NewFD->getLinkage() ==
NamedDecl::InternalLinkage.
Also, I suggest using NewFD->isInlined() rather than isInline, because the call
will also consider previous declarations.
+
+ // Remove used function from our list of static unused functions to prevent
+ // the unused function warning from triggering.
+ if (Function->getStorageClass() == FunctionDecl::Static) {
+ std::vector<FunctionDecl*>::iterator I =
std::find(UnusedStaticFuncs.begin(),
+
UnusedStaticFuncs.end(),
+ Function);
+ if (I != UnusedStaticFuncs.end())
+ UnusedStaticFuncs.erase(I);
+ }
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).
- Doug_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
