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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
