This is an archive of the discontinued LLVM Phabricator instance.

Fix Bug "28480 - cppcoreguidelines-pro-bounds-array-to-pointer-decay handling __PRETTY_FUNCTION__"
AbandonedPublic

Authored by ericLemanissier on Jul 10 2016, 6:44 AM.

Details

Summary

Ignore array to pointer decay for predefined macros. It is ok because there is no safer way to pass these macros to functions

Diff Detail

Event Timeline

ericLemanissier retitled this revision from to Fix Bug "28480 - cppcoreguidelines-pro-bounds-array-to-pointer-decay handling __PRETTY_FUNCTION__".
ericLemanissier updated this object.
aaron.ballman edited edge metadata.Jul 10 2016, 9:04 AM

I agree with your assessment that these should be safe, but the C++ Core Guidelines themselves don't call this case out as part of the enforcement (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#bounds3-no-array-to-pointer-decay). Have you also filed a pull request (or a bug report) there to see if the maintainers agree with this approach?

ericLemanissier abandoned this revision.Jul 12 2016, 3:24 AM

Well, C++ Core Guidelines advises in this case to correct function receiving PRETTY_FUNCTION: the parameter type should not be const char* ( https://github.com/isocpp/CppCoreGuidelines/issues/640 )
I'm not sure if it should be cstring_span, czstring_span, or czstring.
In case it is not possible to change this function, I guess the only solution is static_cast<const char*>(__PRETTY_FUNCTION)

Well, C++ Core Guidelines advises in this case to correct function receiving PRETTY_FUNCTION: the parameter type should not be const char* ( https://github.com/isocpp/CppCoreGuidelines/issues/640 )
I'm not sure if it should be cstring_span, czstring_span, or czstring.
In case it is not possible to change this function, I guess the only solution is static_cast<const char*>(__PRETTY_FUNCTION)

It's not that it's not possible to change this function; I actually think the change is pretty sensible. It's mostly that we want to make sure that the check we implement matches the expected behavior of the C++ Core Guidelines. I think a good approach is to propose this patch (and the test cases) to the C++ Core Guidelines folks via their GitHub tracker, and if they agree with the behavior of your patch, then we can go ahead and add the functionality under the assumption the core guidelines will be updated accordingly as well.

This comment was removed by ericLemanissier.

having studied cpp core guidelines in more depth, I completely understand that the problem in the case I described is that the function receiving PRETTY_FUNCTION takes a const char* parameter, whereas it should take a czstring or szstring_span. As a consequence this patch is not a good thing.

In case it is not possible to change this function, I guess the only solution is static_cast<const char*>(__PRETTY_FUNCTION)

The function I refer to in this sentence is not ProBoundsArrayToPointerDecayCheck::registerMatchers, it is the function in user code receiving PRETTY_FUNCTION.

Would it be an alternative to add the this patch as an option to the check (disabled by default)? Thus one could allow for example PRETTY_FUNCTION without having to disable this whole check while still letting it fully follow the core guidelines by default.