search

guibec / rpgcraft

RPGCraft - Minecraft / Terraria / RPGMaker mashup
MIT License
8 stars 2 forks source link

[static-analysis] Update printf attributes and fix reported problems #114

Open jstine35 opened 5 years ago

jstine35 commented 5 years ago

Proper analysis has only been done for ajek-framework on clang so far.

Visual Studio static analysis is still pretty dodgy. It doesn't catch things like std::string being passed into a printf function, presumably because Visual C++ still auto-converts to .c_str() when passing those into any varadic.

robinlavallee commented 5 years ago

I really doubt VS "Visual C++ still auto-converts to `.c_str()" since this would have side-effects.

jstine35 commented 5 years ago

@robinlavallee Microsoft VC compiler team has no fear of side-effects. It's really a thing. It's bitten my teams in so many projects where we'd build code that compiled and worked fine on Windows and then clang sees this std::string parameter in a varadic and is like "you can't do that, bro!".

And actually it doesn't have side effects because technically by the way the standard is defined, the behavior observed when passing a class into a varadic that doesn't have implicit conversion to int/float/void* is either not allowed or undefined. (probably undefined). Implicit conversion to char* doesn't necessarily interfere with the expectation undefined behavior, it just happens to be undefined behavior that conveniently does what the programmer intended. ;)

But that said, I'm not sure that's what's happening here either. In this case I was using xString, which is my wrapper for std::string. So automatic conversion shouldn't be a thing anyway. I guess microsfot just doesn't warn on what's actually the most common error in authoring printf. (sigh)

This is interesting tho:

According to the C++ standard, varadic parameters should accept classes that implicitly convert to void* or int or double. xString can implicitly convert to char* which is analogous to void* so, according to the docs, it actually should work fine without the c_str().

I'm not actually familiar with this rule though and cppreference.com isn't giving it a C++ version number. Dunno if that means it's old or new and poorly documented. Also CLANG complained, rather than implicitly convert to char*. But I don't know what version of the C++ language it's set to use. (MSVC is set to C++17 current). It would be cool if I could remove the need for .c_str() entirely thanks to some C++ language update. I'll investigate later !

I'll confess I tried to use varadic templates for a couple things and I honestly can't figure them out. Too effing complicated and too much weirdly abbreviated syntax. The online attempts to use them to replace printf are all pure comedy too.