[PATCH] Enable checkUndefinedButUsed() unconditionally at end of TU

[Richard Smith]

On Sun, Jun 8, 2014 at 7:23 AM, Alp Toker <alp at nuanti.com> wrote:

> Right now we only emit the "function/variable has internal linkage but is
> not defined" warning at end of TU if no errors were encountered.
>
> However we probably shouldn't skip checkUndefinedButUsed() so liberally.
> It causes lack of test coverage (doesn't appear in any -verify +
> expected-error test) and loss of useful information for users.
>
> Best guess is that it came to be this way back when bodies used to get
> skipped liberally in error recovery. Nowadays we're tracking definitions
> effectively and should be able to enable the checks unconditionally.
>
> Simple enough?


I think there are other reasons for this. For instance:

inline void foo();
void bar() { foo(); }
int foo() {}

... doesn't get an 'internal linkage but is not defined' error currently; I
think it would with the change you're proposing.

It seems like the justification for this change is that users lose
information (the test coverage problem can be solved in ways that aren't
visible to users). I think that's fair, but it's very hard to distinguish
real problems from false ones here.

Perhaps we could try to distinguish between "top-level" errors (which might
affect the set of definitions available) and errors within function
definitions and the like. Thoughts?

Now for the bad news..
>
> There are 26 tests relying on not using undefined inline functions:
>
> Failing Tests (26):
>     Clang :: CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
>     Clang :: CXX/dcl.dcl/dcl.spec/dcl.constexpr/p8.cpp
>     Clang :: CXX/drs/dr2xx.cpp
>     Clang :: CXX/drs/dr3xx.cpp
>     Clang :: CXX/drs/dr4xx.cpp
>     Clang :: CXX/expr/expr.const/p2-0x.cpp
>     Clang :: CXX/expr/expr.prim/expr.prim.lambda/p19.cpp
>     Clang :: CXX/stmt.stmt/stmt.iter/stmt.ranged/p1.cpp
>     Clang :: CXX/temp/temp.arg/temp.arg.nontype/p1.cpp
>     Clang :: CXX/temp/temp.arg/temp.arg.type/p2.cpp
>     Clang :: CXX/temp/temp.fct.spec/temp.deduct/temp.deduct.partial/
> p11.cpp
>     Clang :: Modules/namespaces.cpp
>     Clang :: Modules/redecl-add-after-load.cpp
>     Clang :: Parser/cxx-ambig-paren-expr.cpp
>     Clang :: Parser/recovery.cpp
>     Clang :: Sema/array-init.c
>     Clang :: Sema/inline.c
>     Clang :: Sema/invalid-decl.c
>     Clang :: SemaCXX/anonymous-struct.cpp
>     Clang :: SemaCXX/explicit.cpp
>     Clang :: SemaCXX/expression-traits.cpp
>     Clang :: SemaCXX/lambda-expressions.cpp
>     Clang :: SemaCXX/new-delete.cpp
>     Clang :: SemaCXX/overload-call.cpp
>     Clang :: SemaCXX/typo-correction.cpp
>     Clang :: SemaTemplate/deduction.cpp
>
> For each we need to figure out the right fix, which varies case by case:
>
>  a) add a definition body {}
>  b) de-inline the declaration
>  c) add expected-warning {{}}
>  d) or just run -Wno-undefined-internal in tests where it doesn't matter
>
> * (c) and (d) will be problematic when the warning becomes a hard error as
> planned in Sema FIXMEs.
>
> So, if we want to apply the patch fixing the issue we'll need to update
> all those tests to valid syntax.
>

=( The size of this cleanup for me swings the cost/benefit tradeoff
somewhat towards "do nothing". The diagnostic will still get produced once
the other errors have been fixed. But if someone wants to run with this,
sure...


> The attached patch implements the fix and demonstrates each one of the
> strategies to get started. Thoughts/volunteers? :-)
>
> Related to:
>   PR19910 - Don't suppress unused/undefined warnings when there are errors
>   http://llvm.org/bugs/show_bug.cgi?id=19910
>
> Historical context:
>   PR5933 - bogus unused variable warning after error
>   http://llvm.org/bugs/show_bug.cgi?id=5933
>
> Alp.
>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140624/2e0f7e7c/attachment.html>