[PATCH] D80887: [clang-tidy] ignore builtin varargs from pro-type-vararg-check

[Aaron Ballman via Phabricator via cfe-commits]

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