1
\$\begingroup\$

I'm a .NET (C#/VB) developer who knows a little C and happens to need to write a native C++ Windows DLL.

Please have a look at the following code and tell me if the string operations are idiomatic C++:

int dpi;

void __stdcall FooEntryPoint(ENTRY_INFO* inEntryInfo)
{
    dpi = *reinterpret_cast<int *>(inEntryInfo->UserData);

    OutputDebugStringW((std::wstring(L"User Data supplied: ") + std::to_wstring(dpi)).c_str());

    FARPROC address = GetFooAddress();
    if (address == NULL)
    {
        OutputDebugStringW(L"Foo address not found.\n");
        return;
    }

    NTSTATUS result = FrobnicateFoo(address);
    if (FAILED(result))
    {
        OutputDebugStringW((std::wstring(L"Failed to frobnicate foo: ") + RtlGetLastErrorString() + L"\n").c_str());
    }
}

My thoughts on the matter:

  • 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.
  • (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. I could use wostringstream, but it would just make the code more complicated.
  • 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".

Does that make sense or am I violating any good C++ practices here?

Note: The code does not need to be portable.

| improve this question | | | | |
\$\endgroup\$
  • 3
    \$\begingroup\$ What is dpi? Is that a global variable? \$\endgroup\$ – Cody Gray Dec 30 '16 at 11:42
  • \$\begingroup\$ @CodyGray: Yes. Sorry, fixed. That happens when you fooify only part of your code... \$\endgroup\$ – Heinzi Dec 30 '16 at 12:22
  • 2
    \$\begingroup\$ @RubberDuck: You're obviously free to VTC as you please, but please consider the fact that I explicitly did not fooify the parts I want feedback on and the question does not contain "purely generic, hypothetical code", as mentioned in the help center. I do get your point, but I'd be a bit pragmatic here - do you really think dumping a wall of unrelated code (which just happens to be in the vicinity of the code in question) or adding a long explanation of context with is irrelevant to the question (but required to understand the unfooed code snippet) improves the question? \$\endgroup\$ – Heinzi Dec 30 '16 at 13:41
  • 3
    \$\begingroup\$ @RubberDuck: I just found this meta answer by following the link in the VTC reason. I did not find it in the help center. It all makes sense now, and I will heed the advice for my next question. I'm a bit hesitant to rewrite my question now since I don't want to invalidate the good answers I got. How do you propose I proceed? \$\endgroup\$ – Heinzi Dec 30 '16 at 13:51
  • 3
    \$\begingroup\$ @Heinzi given you received very good answers, I would take their advice and clean up your real code. At that point, you can either let sleeping dogs lie or post a new question if you'd like further feedback. Thanks for being so understanding. \$\endgroup\$ – RubberDuck Dec 30 '16 at 13:54
3
\$\begingroup\$

Some thoughts:

  • 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.

  • This line:

    dpi = *reinterpret_cast<int *>(inEntryInfo->UserData);
    

    hoists a giant red flag, for at least a couple of reasons:

    1. Why is 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 GetFooAddress and/or 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.

    2. That 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.

    3. 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 assert).

  • 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.

    Indeed, the + 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:

    #ifdef DEBUG
        OutputDebugStringW(...)
    #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:

    #ifdef DEBUG
        #define TRACE(str)    OutputDebugStringW(str)
    #else
        #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 \n, though.

  • 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 address and result variables explicitly const.

  • 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);
        if (FAILED(result))
        {
        #ifdef DEBUG
            std::wstring str = (L"Failed to frobnicate foo: ");
            str += RtlGetLastErrorString();
            str += L"\n";
            OutputDebugStringW(str.c_str());
        #endif  // DEBUG
        }
    }
    else
    {
    #ifdef 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.

| improve this answer | | | | |
\$\endgroup\$
  • 1
    \$\begingroup\$ "hoists a giant red flag, for at least a couple of reasons" Yes that's right. And I was about to mention that as well. Though it seems the code implements a predefined C-API plugin interface. In such cases you don't really have a choice. \$\endgroup\$ – πάντα ῥεῖ Dec 30 '16 at 12:54
  • \$\begingroup\$ @πάνταῥεῖ: "Though it seems the code implements a predefined C-API plugin interface, [...]" - exactly. That's also the reason why dpi is a global variable: It's required at a later invocation of another method. (Yes, the order of execution is guaranteed by the interface contract.) \$\endgroup\$ – Heinzi Dec 30 '16 at 13:28
  • \$\begingroup\$ @Heinzi Let me gues: Printer resolution? \$\endgroup\$ – πάντα ῥεῖ Dec 30 '16 at 13:34
  • \$\begingroup\$ @πάνταῥεῖ: Screen resolution. I'm trying to solve this issue by hooking into GetDeviceCaps. First tests are very promising. \$\endgroup\$ – Heinzi Dec 30 '16 at 13:45
  • \$\begingroup\$ @Heinzi I'm assuming you are trying to do this for an application for which you do not have access to the source code? I'm skeptical of whether or not that is going to work. GetDeviceCaps doesn't control the DPI, it just returns the DPI for the device context that you pass to it. It will have no effect on the device context itself. I am rather shocked that you're getting results that seem promising. No amount of hooking GetDeviceCaps will affect the assumptions that the system is making... \$\endgroup\$ – Cody Gray Dec 30 '16 at 13:50
2
\$\begingroup\$
  • 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.

Nothing wrong with that so far. That's why the c++ standard supports the wide string literals and functions.

  • (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. I could use wostringstream, but it would just make the code more complicated.

Here I have to disagree. Using wostringstream would improve the readability a lot, and help to avoid creating intermediary wstring instances all the time.

Compare

 OutputDebugStringW((std::wstring(L"User Data supplied: ") + std::to_wstring(dpi)).c_str());

or

 OutputDebugStringW((std::wstring(L"Failed to frobnicate foo: ") + RtlGetLastErrorString() + L"\n").c_str());

with

 std::wostringstream oss;
 oss << L"User Data supplied: " << dpi;
 OutputDebugStringW(oss.str().c_str());

 // ...

 oss.str(L"");
 oss << L"Failed to frobnicate foo: " << RtlGetLastErrorString() << L"\n";
 OutputDebugStringW(oss.str().c_str());

from point of view of readability and clearness. Just writing a few more lines doesn't make the code more complicated.

  • 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".

Well, you should at least assert that the passed pointers are valid. E.g. using

 std::assert(inEntryInfo);

before dereferencing it

 int* pdpi = reinterpret_cast<int *>(inEntryInfo->UserData);
 std::assert(pdpi);
 dpi = *pdpi;
| improve this answer | | | | |
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.