[PATCH] D58091: Customize warnings for missing built-in type

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

I did address the comments but I will wait until I hear back on the "warning vs warning + note question".

In D58091#1397586 <https://reviews.llvm.org/D58091#1397586>, @jyknight wrote:

> I think this warning (-Wbuiltin-requires-header) doesn't really make sense as its own warning.
>
> [...]
>
> I think for a declaration, if we cannot construct the appropriate type, we should be treating all declarations as an incompatible redeclaration, and explain why in an attached note, like:
>
>   warning: incompatible redeclaration of library function 'exit' [-Wincompatible-library-redeclaration]
>   note: missing declaration of type 'jmp_buf' for argument 1 of standard function signature.
>
>
> For a usage, we could emit something like:
>
>   warning: implicit declaration of library function 'setjmp' [-Wimplicit-function-declaration]
>   note: missing declaration of type 'jmp_buf' for argument 1.
>   note: include the header <setjmp.h> or explicitly provide a declaration for 'setjmp'
>


I do not have strong feelings about this, either way is fine with me. However, I lack the 
clang expertise to make such a change happen anytime soon which makes this patch
(with actual fix for the warning on pthread_create) my prefered first step.



================
Comment at: clang/lib/Sema/SemaDecl.cpp:1971
           << Context.BuiltinInfo.getName(ID);
+      return nullptr;
+    }
----------------
rsmith wrote:
> It'd be nice to produce `note_include_header_or_declare` here. (Ideally, that note should be suppressed if we're transitively in a header with the right name already, but I think it'll be clear enough what's wrong even if we produce the note unconditionally.)
I did add the "include the header" part in the warning now. Does that make sense and address your issue or do you think we should have a separate note?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58091/new/

https://reviews.llvm.org/D58091