Page MenuHomePhabricator

[clang-tidy] add check cppcoreguidelines-pro-type-vararg-use
ClosedPublic

Authored by mgehre on Oct 15 2015, 2:14 PM.

Details

Summary

This check flags all calls to c-style vararg functions.

Passing to varargs assumes the correct type will be read. This is
fragile because it cannot generally be enforced to be safe in the
language and so relies on programmer discipline to get it right.

This rule is part of the "Type safety" profile of the C++ Core
Guidelines, see
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#-type8-avoid-reading-from-varargs-or-passing-vararg-arguments-prefer-variadic-template-parameters-instead

Diff Detail

Repository
rL LLVM

Event Timeline

mgehre updated this revision to Diff 37520.Oct 15 2015, 2:14 PM
mgehre retitled this revision from to [clang-tidy] add check cppcoreguidelines-pro-type-vararg-use.
mgehre updated this object.
mgehre added a subscriber: cfe-commits.
sbenza added inline comments.Oct 16 2015, 10:12 AM
clang-tidy/cppcoreguidelines/ProTypeVarargUseCheck.cpp
20 ↗(On Diff #37520)

The guideline says that we should also issue a diagnostics for uses of va_list/va_start/va_arg.

test/clang-tidy/cppcoreguidelines-pro-type-vararg-use.cpp
14 ↗(On Diff #37520)

how does this handle SFINAE style ... uses?
The guideline mentions this case as "useful" so we should try to avoid warning on it.

mgehre added inline comments.Oct 16 2015, 12:21 PM
clang-tidy/cppcoreguidelines/ProTypeVarargUseCheck.cpp
20 ↗(On Diff #37520)
test/clang-tidy/cppcoreguidelines-pro-type-vararg-use.cpp
14 ↗(On Diff #37520)

I saw the note in the guidelines, but frankly I don't quite get the use case.
Do you have an example or reference for me?

sbenza added inline comments.Oct 19 2015, 9:54 AM
clang-tidy/cppcoreguidelines/ProTypeVarargUseCheck.cpp
20 ↗(On Diff #37520)

I see. They are split between vararg "def" and "use".

test/clang-tidy/cppcoreguidelines-pro-type-vararg-use.cpp
14 ↗(On Diff #37520)

... has the lowest rank for overload resolution.
You can use it as a default case.
Example:

template <typename T>
void CallFooIfAvailableImpl(T& t, decltype(t.foo())*) {
  t->foo();
}
template <typename T>
void CallFooIfAvailableImpl(T& t, ...) {
  // nothing
}
template <typename T>
void CallFooIfAvailable(T& t) {
  CallFooIfAvailableImpl(t, 0);
}

It is also useful when making traits.
Eg: https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Member_Detector#Solution_and_Sample_Code

mgehre updated this revision to Diff 37790.Oct 19 2015, 2:01 PM

Revert "[clang-tidy] add cert's VariadicFunctionDefCheck as cppcoreguidelines-pro-type-vararg-def"; warn about va_* in this check (to allow SFINAE)

sbenza added inline comments.Oct 19 2015, 2:36 PM
clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
26 ↗(On Diff #37790)

Is there a way to look for the standard type (and not the implementation defined __xxx type name)?

test/clang-tidy/cppcoreguidelines-pro-type-vararg.cpp
38 ↗(On Diff #37790)

isn't va_arg as useless as va_copy (without va_start)?

Actually, given that va_arg is the only one we can look for without using the private __va_xxx name, maybe we should only warn on that one.

sbenza added inline comments.Oct 19 2015, 2:38 PM
test/clang-tidy/cppcoreguidelines-pro-type-vararg.cpp
27 ↗(On Diff #37790)

You would still warn on the callers of this.
Maybe we should ignore any call if the variadic part is a single argument with value 0, which is what is commonly used for SFINAE checks.

mgehre updated this revision to Diff 37814.Oct 19 2015, 4:16 PM

Only flag va_arg and variadic call. Suppress warning if the only vararg argument is literal 0.

sbenza edited edge metadata.Oct 20 2015, 1:01 PM

Minor comments only.

clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
29 ↗(On Diff #37814)

No need to mention __builtin_va_start anymore.

test/clang-tidy/cppcoreguidelines-pro-type-vararg.cpp
31 ↗(On Diff #37814)

nit: t.foo() here vs t->foo() in the next line.

mgehre updated this revision to Diff 37925.Oct 20 2015, 2:45 PM
mgehre edited edge metadata.

removed __builtin_va_start; change f->foo() to f.foo()

sbenza accepted this revision.Oct 20 2015, 2:49 PM
sbenza edited edge metadata.

Thanks for check.

This revision is now accepted and ready to land.Oct 20 2015, 2:49 PM

Thanks for the good review!

clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
26 ↗(On Diff #37790)

I don't see it in the AST, because the other names are macros. But maybe there is some magic I don't know of.

This revision was automatically updated to reflect the committed changes.