[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:420-422
+def ext_implicit_function_decl_c99 : ExtWarn<
+  "implicit declaration of function %0 is invalid in C99 and later and "
+  "unsupported in C2x">, InGroup<ImplicitFunctionDeclare>, DefaultError;
----------------
rsmith wrote:
> The context for someone reading this diagnostic is that they're building in C99 / C11 / C17 mode, used an undeclared function, and (by default) saw this error. In that context, I think it may not be especially relevant to them that Clang would not even allow the error to be disabled if they built in C2x mode, so I'd be inclined to remove the "and unsupported in C2x" part from this diagnostic. Also, they probably didn't *intend* to implicitly declare a function, so perhaps we could replace this diagnostic with something less implementation-oriented and more user-oriented:
> 
> "call to undeclared function %0; ISO C99 and later do not support implicit function declarations"
> 
> (The phrasing "ISO C99 and later do not support" is what we usually use when we want to imply "but perhaps Clang does support" -- keeping in mind that this diagnostic might appear as either an error or a warning, and we want phrasing that works both ways.)
> 
> That said: this is orthogonal to the changes you're making here, so don't consider this comment to be blocking.
> In that context, I think it may not be especially relevant to them that Clang would not even allow the error to be disabled if they built in C2x mode, so I'd be inclined to remove the "and unsupported in C2x" part from this diagnostic.

The reason I had it worded that way was to justify why it's an error instead of a warning.

> "call to undeclared function %0; ISO C99 and later do not support implicit function declarations"

I think this is shorter and equally as clear as what I had. I'll make the change and push it up again for precommit CI to run over (I would not be surprised if I missed a few places to update because of loose `-verify` checking behavior that will no longer match the new wording).


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:698-700
 def ext_implicit_lib_function_decl : ExtWarn<
   "implicitly declaring library function '%0' with type %1">,
   InGroup<ImplicitFunctionDeclare>;
----------------
rsmith wrote:
> Hm, why is this one an `ExtWarn`? Are we really allowed to reject implicit declarations of library functions (in `-std=c89 -pedantic-errors` mode)?
No, we're not. But `ext_implicit_lib_function_decl` was already `ExtWarn`; this new one is the diagnostic you get only in C99 and later.

If you'd like, I can make the C89 one into a `Warning` though (either now or in a follow-up). However, we should probably also think about whether we want C++ to be a default error or not. In this patch it does not default to an error.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:15319-15321
   // OpenCL v2.0 s6.9.u - Implicit function declaration is not supported.
   else if (getLangOpts().OpenCL)
     diag_id = diag::err_opencl_implicit_function_decl;
----------------
rsmith wrote:
> Should we even be calling this function in OpenCL mode? It seems a bit inconsistent that we avoid calling this in C++ and C2x mode, and that we call it but error in OpenCL mode.
> 
> Maybe there should be a function on `LangOptions` to ask if implicit function declarations are permitted in the current language mode, to make it easy to opt out the right cases? (Happy for this to be a follow-on change if you agree.)
I agree that it does seem inconsistent. I can look into making that change in a follow-up.


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

https://reviews.llvm.org/D122983