intel / IntelSEAPI

IntelSEAPI
201 stars 53 forks source link

itt_notify.hpp: UNICODE support broken and diverges from Intel ITT API #11

Open sqz opened 7 years ago

sqz commented 7 years ago
#ifdef _WIN32
    #define UNICODE_AGNOSTIC(name) name##A
#else
    #define UNICODE_AGNOSTIC(name) name
#endif

This code is odd because it appears to attempt force strings to always be narrow by ... platform detection.

Yet the underlying library, Intel ITT API, will ignore this attempt and do its own checks, specifically and correctly against the UNICODE or _UNICODE defines.

If a client application is being compiled on Windows and has the (_)UNICODE defines set, regardless of whether narrow or wide strings are used, it will cause a compile error. This is because if wide strings are used, the wrong character type is passed. If narrow strings are used, the rest of the header is not consistently using UNICODE_AGNOSTIC, e.g. the function __itt_metadata_str_add() in the template specializations of Task<bool>.AddArg<>()

So, either itt_notify.hpp needs to be consistent with Intel ITT API by removing the UNICODE_AGNOSTIC() check and providing a wide string specialization for .AddArg() or it retains the strange UNICODE_AGNOSTIC platform check code and uses it throughout to force narrow string only support.

If the latter design is chosen, then this should be documented very clearly as a significant difference from the underlying library.

Note. In addition, the following from the Intel ITT API documentation:

"The Intel ITT API contains a subset of functions working with strings ASCII (UTF-8) or Unicode (UTF-16). ASCII functions are wrappers for correspondent Unicode functions, and consequently they utilize more CPU clock ticks than their Unicode analogues."

red1939 commented 7 years ago

I have to agree - it recently drove me crazy. Having a UNICODE VS project forces the macro to be wchar_t and that breaks as AddArg takes char. Why should I be prohibited from using char in UNICODE application?