How can I refactor this to use an inline function or template instead of a macro?

Question

Hello, everyone :)

I have a useful macro here:

#include <algorithm>
#include <vector>
#include <string>
#include <boost/algorithm/string.hpp>
#include <Windows.h>

namespace Path {

bool Exists(const std::wstring& path)
{
    DWORD result = GetFileAttributesW(path.c_str());
    return result != INVALID_FILE_ATTRIBUTES;
}

// THIS IS THE MACRO IN QUESTION!
#define PATH_PREFIX_RESOLVE(path, prefix, environment) \
if (boost::algorithm::istarts_with(path, prefix)) { \
    ExpandEnvironmentStringsW(environment, buffer, MAX_PATH); \
    path.replace(0, (sizeof(prefix)/sizeof(wchar_t)) - 1, buffer); \
    if (Exists(path)) return path; \
}

std::wstring Resolve(std::wstring path)
{
    using namespace boost::algorithm;
    wchar_t buffer[MAX_PATH];
    trim(path);
    if (path.empty() || Exists(path)) return path;

    //Start by trying to see if we have a quoted path
    if (path[0] == L'"') {
        return std::wstring(path.begin() + 1, std::find(path.begin() + 1, path.end(), L'"'));
    }

    //Check for those nasty cases where the beginning of the path has no root
    PATH_PREFIX_RESOLVE(path, L"\\", L"");
    PATH_PREFIX_RESOLVE(path, L"?\?\\", L"");
    PATH_PREFIX_RESOLVE(path, L"\\?\\", L"");
    PATH_PREFIX_RESOLVE(path, L"globalroot\\", L"");
    PATH_PREFIX_RESOLVE(path, L"system32\\", L"%systemroot%\\System32\\");
    PATH_PREFIX_RESOLVE(path, L"systemroot\\", L"%systemroot%\\");

    static std::vector<std::wstring> pathExts;
    if (pathExts.empty()) {
        #define MAX_ENVVAR 32767
        wchar_t pathext[MAX_ENVVAR];
        DWORD length = GetEnvironmentVariableW(L"PATHEXT", pathext, MAX_ENVVAR);
        if (!length) WindowsApiException::ThrowFromLastError();
        split(pathExts, pathext, std::bind2nd(std::equal_to<wchar_t>(), L';'));
        pathExts.insert(pathExts.begin(), std::wstring());
    }
    std::wstring::iterator currentSpace = path.begin();
    do {
        currentSpace = std::find(currentSpace, path.end(), L' ');
        std::wstring currentPath(path.begin(), currentSpace);
        std::wstring::size_type currentPathLength = currentPath.size();
        typedef std::vector<std::wstring>::const_iterator ExtIteratorType;
        for(ExtIteratorType it = pathExts.begin(); it != pathExts.end(); it++) {
            currentPath.replace(currentPathLength, currentPath.size() - currentPathLength, *it);
            if (Exists(currentPath)) return currentPath;
        }
        if (currentSpace != path.end())
            currentSpace++;
    } while (currentSpace != path.end());

    return path;
}

}

It's used about 6 times within the scope of a single function (that's it), but macros seem to have "bad karma" :P

Anyway, the problem here is the sizeof(prefix) part of the macro. If I just replace this with a function taking a const wchar_t[], then the sizeof() will fail to deliver expected results.

Simply adding a size member doesn't really solve the problem either. Making the user supply the size of the constant literal also results in a mess of duplicated constants at the call site.

Any ideas on this one?

Answer 1

Pass the array by reference, using a template to infer the length. I'll go looking for an example, but basically:

template<size_t N>
bool func(const char (&a)[N], blah, blah) { ... }

EDIT: Someone explained it here: http://heifner.blogspot.com/2008/04/c-array-size-determination.html

Answer 2

Have you tried replacing sizeof(prefix)/sizeof(wchar_t) with wcslen(prefix)?

Answer 3

Why not just make it a regular function that uses wcslen() to get the length of the parameter you're interested in. There's enough stuff going on in that macro/function that I imagine there's little value trying to force it to be inlined. The 'overhead; of the wcslen() call and processing is almost certainly not going to be a bottleneck.

The only trick in the macro that you really should be concerned with is (as GMan pointed out) the return from the within the macro that's hidden when the macro is invoked.

Just have the thing be a function that returns a success/fail, and you can return if the function succeeds:

bool PathPrefixResolve( std::wstring& path, wchar_t const* prefix, wchar_t const* environment)
{
    wchar_t buffer[MAX_PATH];

    if (boost::algorithm::istarts_with(path, prefix)) {
        ExpandEnvironmentStringsW( environment, buffer, MAX_PATH);

        std::wstring tmp( path);

        tmp.replace(0, wcslen( prefix), buffer);
        if (Exists(tmp)) {
            path = tmp;
            return true;
        }
    }

    return false;
}

to use the function:

//Check for those nasty cases where the beginning of the path has no root
if (PathPrefixResolve2(path, L"\\", L"")) return path;
if (PathPrefixResolve2(path, L"?\?\\", L"")) return path;
if (PathPrefixResolve2(path, L"\\?\\", L"")) return path;
if (PathPrefixResolve2(path, L"globalroot\\", L"")) return path;
if (PathPrefixResolve2(path, L"system32\\", L"%systemroot%\\System32\\")) return path;
if (PathPrefixResolve2(path, L"systemroot\\", L"%systemroot%\\")) return path;

Given what the processing that's occurring in the macro, I don't think you need to be worried about function call overhead.

Also, your macro implementation has some behavior which I think is probably a bug - if the path starts with L"\\?\\" that means it also starts with L"\\" and your first invocation of the macro:

PATH_PREFIX_RESOLVE(path, L"\\", L"");

will change the path variable. As the program gets maintained and additional prefixes get added, the problem could be seen with other path prefixes. This bug isn't in the function version, since the function changes the path parameter only when there's a verified match.

However, there's still possibly an issue when dealing with the L"\\?\\" and L"\\" prefixes in that both might be a match - you need to make sure you pass in the prefixes that might match more than once in 'priority' order.