bloomberg / bde

Basic Development Environment - a set of foundational C++ libraries used at Bloomberg.
Apache License 2.0
1.68k stars 318 forks source link

C++ standard violations regarding pointers to function casting #203

Closed lboquillon closed 8 years ago

lboquillon commented 8 years ago

Some compilers (Clang and GCC) issue a warning regarding a C++ standard non-compliance when compiling the BDE bsls. You can see in the C++ standard (ISO/IEC 14882, 2003 version), section 5.2.10 [reinterpret-cast] that conversions from pointers to function to pointers to object are not listed as legal, and so they are disallowed. The violations to this rule occur in a single line of code, where a pointer to function is cast to void* using reinterpret_cast. As an attachment error.txt , there is a collection of the complete warning messages issued by compiler. The offending line is bsls/blss_log.h:293, at the column 74:

    bsls::AtomicOperations::setPtrRelease(&s_logMessageHandler,
                                          reinterpret_cast<void*>(handler));

(in this context, handler is a Log::LogMessageHandler)

Additionally, in bsls/bsls_assert.cpp:98, at the column 44 have similar issue with C style cast:

bsls::AtomicOperations::setPtrRelease(&s_handler, (void *) function);

There are projects where contractual constraints demand that any third-party library in use does not issue any warning judged to be severe. This violation makes the library unusable for said projects.

Is there any possibility to address this issue in the near future?

Thanks in advance for any feedback. Leonardo Boquillón.

mversche commented 8 years ago

Depends what you mean by addressed. We can and will remove the compiler warning.

We are not likely to remove the assumption that pointers to function and pointers to object are of the same size. The assumption is true for our supported platforms, as verified by the test driver. I believe it is required by both Windows and POSIX, and becomes conditionally supported with C++11 (i.e., it is not guaranteed portable, but implementations are expected to support it unless documented otherwise).

dgutson commented 8 years ago

What about at least provide encapsulated facilities such as: (not compiled)

template <class IntegralType>
void* pointer_cast(IntegralType value)
{
    static_assert(sizeof(IntegralType) == sizeof(void*), "Wrong cast from integer to pointer");
    static_assert(std::is_integral<IntegralType>::value, "Casting from a non-integral type to pointer");
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wpedantic"
    return reinterpret_cast<void*>(value);
#pragma GCC diagnostic pop
}

so it is easily searchable later, and the warning can be turned off inside that function

mversche commented 8 years ago

Someone will look at this next week. I expect we will provide a casting utility similar to pointer_cast (but meeting our coding guidelines). The first cut will probably try casting through intptr_t as a means of suppressing the warning, and use the pragma as a fallback if that doesn't work.

dgutson commented 8 years ago

Thanks. Please consider the gcc version for the pragma: "pedantic" is deprecated and replaced by Wpedantic since version 4.8 (see https://gcc.gnu.org/gcc-4.8/changes.html). Let us know if we can help by sending some patch.

hyrosen commented 8 years ago

I've posted an internal pull request, https://bbgithub.dev.bloomberg.com/bde/bde/pull/142. Once it's accepted, we can push a version to github.

lboquillon commented 8 years ago

Hi @hyrosen,

Some news about it?

Thanks

hyrosen commented 8 years ago

The changes went into new pull requests {https://bbgithub.dev.bloomberg.com/bde/bde/pull/176} {https://bbgithub.dev.bloomberg.com/bde/bde/pull/180} that have been merged. The results are already visible here on github, e.g., {https://github.com/bloomberg/bde/blob/master/groups/bsl/bsls/bsls_assert.cpp#L101}

mversche commented 8 years ago

Per above.