Fix: TraceLoggingDynamic.h failing under /Zc:preprocessor on msvc#80
Fix: TraceLoggingDynamic.h failing under /Zc:preprocessor on msvc#80Robo210 merged 3 commits intomicrosoft:mainfrom
Conversation
fbccdfb to
4621bbb
Compare
76c8f7f to
7de3f5a
Compare
| #ifndef _tld_ASSERT | ||
| #if TLD_DEBUG | ||
| #define _tld_ASSERT(exp, str) ((void)(!(exp) ? (__annotation(L"Debug", L"AssertFail", L"TraceLogging: " L#exp L" : " L##str), DbgRaiseAssertionFailure(), 0) : 0)) | ||
| #define _tld_ASSERT(exp, str) ((void)(!(exp) ? (__annotation(L"Debug", L"AssertFail", L"TraceLogging: " #exp L" : " #str), DbgRaiseAssertionFailure(), 0) : 0)) |
There was a problem hiding this comment.
I'm not sure which widening trick is most portable outside msvc.
https://learn.microsoft.com/en-us/cpp/preprocessor/preprocessor-experimental-overview?view=msvc-170#lval
suggests something like 3 options.
#define WIDE(str) L##str
#define STRING2(str) WIDE(#str)
Might be more robust than this to older compilers? but I'm not sure how much of a concern that is given the current non-conformance of the header.
There was a problem hiding this comment.
Happy to use a different one if you have preferences.
| #ifndef _tld_ASSERT | ||
| #if TLD_DEBUG | ||
| #define _tld_ASSERT(exp, str) ((void)(!(exp) ? (__annotation(L"Debug", L"AssertFail", L"TraceLogging: " L#exp L" : " L##str), DbgRaiseAssertionFailure(), 0) : 0)) | ||
| #define _tld_ASSERT(exp, str) ((void)(!(exp) ? (__annotation(L"Debug", L"AssertFail", L"TraceLogging: " #exp L" : " #str), DbgRaiseAssertionFailure(), 0) : 0)) |
There was a problem hiding this comment.
Removing the L seems reasonable. Why did we also change the str from "prefix with L" to "convert to string"?
There was a problem hiding this comment.
tl;dr: because I screwed up.
I'll be honest, I'm not a macro or preprocessor wizard. I'm just tilting at windmills over here experimenting with seeing if we could maybe possibly get a large, 1st party codebase (cough SQL cough) to use header units instead of PCH, so that we can enter this decade.
So human error on my part. I saw many, many places where macros like L##x were not playing nice because the old preprocessor was happy to pretend they actually just were L x.
Some of those also had evaluation order shenanigans going on... sometimes it seems like the old preprocessor likes to treat
`MY_MACRO((X))
As if it was:
#define DEFER(...) __VA_ARGS__
MY_MACRO(DEFER(X))
Though I could be wrong about that, I've spent the last two days traipsing my way through ~300 files full of particularly cursed macros.
And I hadn't had any problems with TraceLoggingProvider.h, so I hadn't spotted the similarity.
idigdoug
left a comment
There was a problem hiding this comment.
Unless there was a reason to change the behavior, please change #str to str to preserve existing behavior.
Comparable code in TraceLoggingProvider.h looks like this:
#define _tlg_ASSERT(exp, str) ((void)(!(exp) ? (__annotation(L"Debug", L"AssertFail", L"TraceLogging: " _tlg_LSTRINGIZE(exp) L" : " str), TLG_RAISEASSERTIONFAILURE(), 0) : 0))
b913183 to
8a57b0a
Compare
Done, good catch. An aside on TraceLoggingProvider.h - it's probably academic (since I assume that's usually only ever compiled with gcc and clang and the like, and I also assume the macros in question don't expect to be passed zero varadiac arguments. But the old msvc preprocessor is lax about comma elision around variadic arguments - other compilers will only elide Thought I'd mention it just in case. |
L#expandL##strwith#expand#str, since there is already a proceeding string literal that they will be combined with; per https://learn.microsoft.com/en-us/cpp/preprocessor/preprocessor-experimental-overview?view=msvc-170#lval, as well as via Compiler Explorer , this should compile with both the old and new preprocessor (as well as being more compatible with clang, gcc, and so on.Also replace the parentheses shenanigans (that really shouldn't compile!) with immediately evaluated Lambda shenanigans that are conformant preprocessor compatible. I'm assuming TraceLoggingDynamic.h already requires C++11, but if it doesn't, we could make TLD_DEBUG default to 0 for pre-C++11 compiler versions.Closes #79