It is good that you explicitly annotated the function's definition with its required calling convention. Make sure that you do the same thing for the declaration in the public C++ header, and for the P/Invoke declaration (if applicable) in your .NET application.
Calling convention mismatches are a common source of bugs, and you don't want to rely on the correct compiler flags (specifying the default calling convention) having to be specified in order for the code to work correctly.
dpi = *reinterpret_cast<int *>(inEntryInfo->UserData);
hoists a giant red flag, for at least a couple of reasons:
dpi a global (module-scoped?) variable? It looks to me like it just needs to be a local variable. I'm assuming that you raised its scope so it would be usable within the
FrobnicateFoo functions? If so, you should pass it as a parameter to these functions. Always keep the scope of variables to the minimum level necessary—this is the same rule as in C#/VB.NET, of course.
reinterpret_cast business is dicey. Reinterpretation casts are always a bit dicey, and this one especially, since it doesn't have a reassuring comment nearby that explains how it is safe. :-) You didn't show us the definition of the
ENTRY_INFO type, so it's hard to confirm whether or not this actually is safe, but there really aren't that many cases where you can legitimately reinterpret a pointer to one primitive type as a pointer to another primitive type. If I had to guess, I'd say that
UserData is a
void*? If at all possible, you should just change it to be the
int* that you ultimately want. The only advantage of a
void* would be to achieve something akin to VB's Variant, but that requires some way to determine what the actual type is. Your code doesn't have that, it just assumes that the pointer is actually to an
int. Lots of different ways that can go wrong. Hard to say exactly how you should fix this without more information, but definitely know that this smells funny and very likely hides a bug.
Beyond the question of the cast, you are potentially dereferencing a null pointer, which is undefined behavior and definitely a bug. You need to be checking this explicitly, just like you are all of the other error conditions, unless you can guarantee that the caller will never pass you a null pointer (in which case, verify that assumption using an
As a .NET developer, I'm used to "Unicode is default unless there's a very good reason against it", and, thus, I just use
L"..." string literals and
...W API methods without giving it further thought.
This is completely correct. In fact, this doesn't have anything to do with .NET; it is more precisely a Windows thing. The Windows operating system uses Unicode internally (at least, all versions of NT do, which means everything modern and semi-modern), and there is no reason to ever use narrow/ANSI strings unless you need the code to run on Windows 9x. Therefore, your adopted practices make very good sense.
(std::wstring(...) + ... + ...).c_str() seems to be the simplest way in C++ to do string concatenations of wchar arrays (denoted as
...) without having to worry about memory allocations.
+ operator is the simplest way to concatenate strings in C++. However, it is not necessarily the most efficient. Use of the
+ operator is creating a bunch of temporaries, which the compiler may or may not be able to optimize out. In general, you should prefer the
+= operator to help stave off the proliferation of temporary objects (or the
append member function, which is slightly less terse, but may be easier to reason about because there are no operator precedence concerns).
Also, not having to worry about memory allocations may be a curse disguised as a blessing. In general, pre-allocating the required amount of space (using the
reserve member function) will net you better performance than letting the library code do incremental allocations as-needed, assuming of course that you have some idea in advance of how much space you will need.
That said, neither of these things matter if the code you're writing is not performance-sensitive. And obviously this isn't, since it's calling
OutputDebugString, which is a very slow function.
We're talking about a few bytes here. Isn't that a case of premature optimization? Or is it generally "good practice" in C++ to worry about such things?
Yes, it is probably "good practice" in C++ to worry about such things. A major reason for people to write code in C++ is because they care about efficiency.
I could use
wostringstream, but it would just make the code more complicated.
Well, that is arguable, and I see that πάντα ῥεῖ already made the argument. This is ultimately up to you: either will work, and since debug output is not performance-sensitive, there isn't a clear case to be made either way as far as I'm concerned. However, on the readability front, you can actually get a similar effect by using the
+= operator (which I'd recommended anyway—see code below).
I do want to quibble slightly with one thing πάντα ῥεῖ said, though—or at least, my interpretation of what he said… While it is, in general, completely true that more lines of code do not make a program slower or more complicated, using stream output probably is going to be slower than simple string concatenation. I would only resort to streams if I needed to format the output in some way.
Also, speaking of
OutputDebugString, this is almost certainly a function that you only want to call in debugging builds. It won't be conditionally compiled out like the equivalent
Debug functions in .NET; you have to do this manually. The simplest way is to use a preprocessor conditional, something like:
#endif // DEBUG
Another alternative would be to write your own
TRACE macro (MFC and other commonly used C++ libraries have one included) that either expands to a call to
OutputDebugString or to a no-op, depending on the build settings. For example:
#define TRACE(str) OutputDebugStringW(str)
#define TRACE(str) (void)(str)
#endif // DEBUG
There are all sorts of ways to make this more complicated, more flexible, and more useful. In fact, you might not decide that you don't want to base it on the definition of a
DEBUG preprocessor symbol at all, since you might want this debug trace available in an instrumented but otherwise optimized build. However, I would not recommend spewing a bunch of debug info in a shipping build. It is slow and unhelpful to the typical user.
If you really need this information for remote debugging, you should almost certainly be writing it into a log file, instead, since
OutputDebugString is effectively an expensive no-op if there is no debug listener attached! Investigate a logging framework for C++, or write your own if you have unlimited time on your hands. Either way, make it so that this "verbose" mode can be enabled as a run-time option at the request of your technical support staff.
One last point about
OutputDebugString: I personally believe that any string you pass to it should always end with a trailing new-line character. It doesn't get inserted automatically, so multiple back-to-back calls to
OutputDebugString—even in completely different modules—will result in cluttered and unreadable output otherwise. A
TRACE macro could certainly automatically append this
I know that failing silently is bad practice, but the signature of FooEntryPoint is part of the interface contract I'm given, so this is "by design".
Fair enough, but I have to again agree with πάντα ῥεῖ that you should be using an
assert here for ease of debugging—unless that is, failure is an expected scenario.
You should be aware, though, that idiomatic C++ would use exceptions for things like this that cannot reasonably be handled by the callee, just like idiomatic C#/VB.NET code. This wouldn't affect the signature (the function would still return
void), but it would certainly affect the interface, so I realize that may not be an option for you.
Instead of the
NULL macro that you would use in C code, modern C++ code should prefer to use
nullptr for a variety of reasons. You can basically use it as a drop-in replacement, with the advantage that it is a distinct type so it won't ever be ambiguous with the integer literal 0.
I also like to obsessively make every local variable that I'm not going to change in subsequent code
const. Although this doesn't really enable any additional optimizations, it makes it easier for me to reason about the code and increases the likelihood that the compiler will catch bugs. In fact, I'm of the opinion that
const should have been the default for locals, and you should have had to explicitly mark them as
mutable if you wanted that. As a case in point, I would make the
result variables explicitly
My last observation is almost purely stylistic, and probably at least somewhat controversial, but I would format the code differently to ensure only a single exit point. I'm not at all dogmatic about this, but I think in this particular case it is clearer to think about the operations you're doing in a chain or hierarchy, so it is natural to format them as a series of nested blocks:
const FARPROC address = GetFooAddress();
if (address != nullptr)
const NTSTATUS result = FrobnicateFoo(address);
std::wstring str = (L"Failed to frobnicate foo: ");
str += RtlGetLastErrorString();
str += L"\n";
#endif // DEBUG
OutputDebugStringW(L"Foo address not found.\n");
#endif // DEBUG
I'm neutral on whether the error condition (
address == NULL) should come first, or whether the success condition should come first. There are readability arguments both ways. I tend to prefer putting the success condition first to increase the likelihood of successful static branch prediction, but that's becoming a non-issue on current generations of processors that don't simply assume "not taken" for all forward conditional jumps. I just wrote a crazy-long answer about this that ultimately concludes it doesn't matter.