Calling a function from a dynamically loaded DLL (version 2)

Question

Please, have a look at the previous (bad) implementation and the review here.

After reading the suggested article at HowTo: Export C++ classes from a DLL, I have decided to use a plain C function approach. However, I want to use C++ code inside the DLL later. Anyway, the exported one will be a plain C function with the const char * argument.

In my earlier attempt, I tried to pass reference to the vector<string> argument, which is not possible or at least safe. The vector of strings was the result of parsing the line from a text file. Now, I am passing the line itself, and the parsing is done inside the DLL. The code of the caller and the type of the called function look like this:

typedef __declspec(dllimport) int (__cdecl *CONVERTPROC)(const char *);


int call_convert_from_dll(const std::string & dllname, const char * line)
{
    HINSTANCE hinstLib;
    CONVERTPROC convert;
    BOOL fRunTimeLinkSuccess = FALSE;

    // Get a handle to the DLL module.
    hinstLib = LoadLibrary(dllname.c_str());

    // If the handle is valid, try to get the function address.
    if (hinstLib != nullptr)
    {
        convert = (CONVERTPROC)GetProcAddress(hinstLib, "convert");

        // If the function address is valid, call the function.
        if (convert != nullptr)
        {
            fRunTimeLinkSuccess = TRUE;
            (convert)(line);
        }
        else {
            DWORD err = GetLastError();
            if (err == ERROR_PROC_NOT_FOUND)
                cerr << "procedure convert() was not found" << endl;
            else
                cerr << "runtime link failed: " << err << endl;
        }
        // Free the DLL module.
        FreeLibrary(hinstLib);
    }

    // If unable to call the DLL function, use an alternative.
    if (!fRunTimeLinkSuccess) {
        cerr << "Failed when using '" << dllname << "' library." << endl;
        return 1;
    }

    return 0;
}

The name of the called function (from the DLL) is passed as a string. Because of that, the extern "C" must be used to avoid identifier mangling.

The mentioned article also says that no exception should cross the boundary of the DLL module (if it is not the special case when both parts are written in C++ and compiled by the same version of the compiler). Because of that, the try/catch wraps the body of the called function. Any exception is transformed to the error code equal to 1:

extern "C" {          // we need to export the C interface

__declspec(dllexport) int __cdecl convert(const char * line)
{
    int err = 0;
    try {
        cout << "my.dll: '" << line << "'" << endl;
        vector<string> vs;
        StringTok(vs, line, " \t\n");
        err = xmain(vs);
    }
    catch (...) {
        return 1;
    }
    return err;
}

} // extern "C"

Can you see any other flaw in the code?

Answer 1

Certainly a better approach that the original. Only one 'flaw' that I noticed:

• You ignore the return value from your convert function.

But I do have a couple of comments:

• You may want to consider making line a const char * const (with associated other changes).

• Put the type declaration next to the first use. i.e., rather than

  HINSTANCE hinstLib;
  // several lines
  hinstLib = LoadLibrary(dllname.c_str());
  

  do

  HINSTANCE hinstLib = LoadLibrary(dllname.c_str());
  

• Since you're in C++, use bool \ false etc rather than BOOL.

• Depending on how frequently this would be called, you may want to adopt a RAII approach similar to Windows DLL Module Class. This could lead to some simplifications in the main flow of the code.
• LoadLibrary and GetProcAddress are documented as returning NULL rather than std::nullptr. It may be totally silly, but I'd stick with the documentation.
• You don't deal with the error case of LoadLibrary failing, with respect to GetLastError and giving a sensible error.
• Consider using a reinterpret_cast<CONVERTPROC> rather than (CONVERTPROC).
• You only return 1 or 0 from call_convert_from_dll - should it have a bool or enumerated return type?
• Depending on what the effect of failure is, you may want to consider using exceptions, catching them further up the chain rather than handling failure in this function.

For your convert function:

• The code implies the use of using namespace std. Please don't.
• Rather than using an err variable, you could return immediately. The function is short enough to be understandable.

• If any exceptions are likely to be thrown by inner code, catch them separately in the catch block and give details rather than 'catch all and dump'. Perhaps something like:

  catch (const std::exception& e) {
      std::cerr << "Convert exception: " << e.what() << std::endl;
      return 1;
  }
  catch (...) {
      std::cerr << "Convert unspecified exception" << std::endl;
      return 1;
  }