aaron.ballman added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp:21
+// FIXME: Add any more builtin variadics that shouldn't trigger this
+static constexpr StringRef AllowedVariadics[] = {
----------------
njames93 wrote:
> aaron.ballman wrote:
> > njames93 wrote:
> > > aaron.ballman wrote:
> > > > How would we know which builtins should or should not trigger this? Can
> > > > we add that information to this comment (or, better yet, just fix the
> > > > fixme up front)?
> > > Good point, its more a case that I don't know all the variadic builtins
> > > clang supports. Those ones I included are just the ones you find in the
> > > gcc documentation. Shall I just remove the Fixme completely, If there is
> > > another one that's been missed we can address that later
> > Taking a look at Builtins.def shows quite a few more that likely should be
> > added to the list. I think we should probably have the initial commit
> > covering anything that's a C standard library function/macro that ends with
> > "." in its builtin type signature where the C API isn't variadic. For
> > instance, `__builtin_isfinite`, `__builtin_isinf`, etc. I'm fine if we
> > don't vet every builtin we support (that's a large amount of work), but we
> > should be able to cover the most common cases where there's a specification
> > to compare against.
> ```
> BUILTIN(__builtin_isgreater , "i.", "Fnct")
> BUILTIN(__builtin_isgreaterequal, "i.", "Fnct")
> BUILTIN(__builtin_isless , "i.", "Fnct")
> BUILTIN(__builtin_islessequal , "i.", "Fnct")
> BUILTIN(__builtin_islessgreater , "i.", "Fnct")
> BUILTIN(__builtin_isunordered , "i.", "Fnct")
> BUILTIN(__builtin_fpclassify, "iiiiii.", "Fnct")
> BUILTIN(__builtin_isfinite, "i.", "Fnct")
> BUILTIN(__builtin_isinf, "i.", "Fnct")
> BUILTIN(__builtin_isinf_sign, "i.", "Fnct")
> BUILTIN(__builtin_isnan, "i.", "Fnct")
> BUILTIN(__builtin_isnormal, "i.", "Fnct")
> BUILTIN(__builtin_signbit, "i.", "Fnct")
> BUILTIN(__builtin_constant_p, "i.", "nctu")
> BUILTIN(__builtin_classify_type, "i.", "nctu")
> BUILTIN(__builtin_va_start, "vA.", "nt")
> BUILTIN(__builtin_stdarg_start, "vA.", "nt")
> BUILTIN(__builtin_assume_aligned, "v*vC*z.", "nc")
> BUILTIN(__builtin_fprintf, "iP*cC*.", "Fp:1:")
> BUILTIN(__builtin_printf, "icC*.", "Fp:0:")
> BUILTIN(__builtin_snprintf, "ic*zcC*.", "nFp:2:")
> BUILTIN(__builtin___snprintf_chk, "ic*zizcC*.", "Fp:4:")
> BUILTIN(__builtin___sprintf_chk, "ic*izcC*.", "Fp:3:")
> BUILTIN(__builtin___fprintf_chk, "iP*icC*.", "Fp:2:")
> BUILTIN(__builtin___printf_chk, "iicC*.", "Fp:1:")
> BUILTIN(__builtin_prefetch, "vvC*.", "nc")
> BUILTIN(__builtin_shufflevector, "v." , "nct")
> BUILTIN(__builtin_convertvector, "v." , "nct")
> BUILTIN(__builtin_call_with_static_chain, "v.", "nt")
> BUILTIN(__builtin_annotation, "v.", "tn")
> BUILTIN(__builtin_add_overflow, "b.", "nt")
> BUILTIN(__builtin_sub_overflow, "b.", "nt")
> BUILTIN(__builtin_mul_overflow, "b.", "nt")
> BUILTIN(__builtin_preserve_access_index, "v.", "t")
> BUILTIN(__builtin_nontemporal_store, "v.", "t")
> BUILTIN(__builtin_nontemporal_load, "v.", "t")
> BUILTIN(__builtin_os_log_format_buffer_size, "zcC*.", "p:0:nut")
> BUILTIN(__builtin_os_log_format, "v*v*cC*.", "p:0:nt")
> BUILTIN(__builtin_ms_va_start, "vc*&.", "nt")```
> That's all the variadics inside `Builtin.def` that are called `__builtin...`.
> Would you suggest including all those apart from the va, print or format
> related ones
I would include all of those that are not defined as being variadic in the C,
C++, or POSIX standards. So things like `__builtin_isgreater` make sense to
include, but `__builtin_isinf_sign` is a bit less clear to me. So a large part
of that list seems like stuff we'd want to include, but not all of it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80887/new/
https://reviews.llvm.org/D80887
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
