support unicode in paths

[xfc1939]

Fixes #984

[codecov-commenter]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (931323d) 64.14% compared to head (0932ea5) 64.32%.
Report is 8 commits behind head on master.

Files	Patch %	Lines
src/logging.cc	80.00%	3 Missing and 4 partials ⚠️
src/utilities.cc	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #998      +/-   ##
==========================================
+ Coverage   64.14%   64.32%   +0.18%     
==========================================
  Files          17       17              
  Lines        3327     3330       +3     
  Branches     1125     1126       +1     
==========================================
+ Hits         2134     2142       +8     
+ Misses        766      760       -6     
- Partials      427      428       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sergiud]

Thanks for the PR. I need some time to look through the changes.

In the meantime, could you please rebase onto current head and resolve the conflicts?

[sergiud]

Now that I had a careful look at the implementation, I see two major issues:

1. Switching to std::wstring under UNICODE definition will break existing user code. This is something we definitely want to avoid. Support for wide strings should be transparent.
2. Many of the functions are duplicated with respect to wide strings. For maintenance purposes, the std::basic_string code should be unified using templates to encapsulate the logic into a single function or method.

[xfc1939]

Now I have modified the DefaultDir() function by used template. Could you give me some suggestions.

template<class CharT>
static const std::basic_string<typename std::enable_if_t<std::is_same_v<CharT, wchar_t>, CharT>> DefaultLogDir() {
        std::wstring env = getenvw((wchar_t*)TEXT("GOOGLE_LOG_DIR"));
        if (!env.empty()) {
            return env;
        }
        env = getenvw((wchar_t*)TEXT("TEST_TMPDIR"));
        return env;
}

template<class CharT>
static const std::basic_string<typename  std::enable_if_t<std::is_same_v<CharT, char>, CharT>> DefaultLogDir() 
{
    const char* env;
    env = getenv("GOOGLE_LOG_DIR");
    if (env != nullptr && env[0] != '\0') {
        return env;
    }
    env = getenv("TEST_TMPDIR");
    if (env != nullptr && env[0] != '\0') {
        return env;
    }
    return "";
}

template<class CharT>
static const std::basic_string<typename  std::enable_if_t<!std::is_same_v<CharT, char> && !std::is_same_v<CharT, wchar_t>, CharT>> DefaultLogDir()
{
    return {};
}

[sergiud]

In the code above, DefaultDir sill uses duplicated logic while the only difference is the use of getenv for ANSI and Unicode strings. It is therefore more meaningful to provide a unified overload for the corresponding functions.

Something like this should do the job:

namespace details {
using std::getenv; // char variant
const wchar_t* getenv(const wchar_t* name) {
    return _wgetenv(name);
}
} // namespace internal

template<class Ch, std::size_t N>
std::basic_string<Ch> DefaultLogDir(const std::array<const Ch*, N>& names) {
    for (const Ch* name : names) {
        if ((const Ch* var = details::getenv(name)) != nullptr) {
            return var;
        }
    }
    return {};
}

Now you only need to specialize the literals:

template<class Ch>
struct LogDirEnvVar;

template<>
struct LogDirEnvVar<char> {
    constexpr static std::array<const char*, 2> names() noexcept {
        return {"GOOGLE_LOG_DIR", "TEST_TMPDIR"};
    }
};

template<>
struct LogDirEnvVar<wchar_t> {
    constexpr static std::array<const wchar_t*, 2> names() noexcept {
        return {L"GOOGLE_LOG_DIR", L"TEST_TMPDIR"};
    }
};

Eventually, you can define

template<class Ch>
decltype(auto) DefaultLogDir() {
    return DefaultLogDir(LogDirEnvVar<Ch>::names());
}

and use it as follows:

auto dir1 = DefaultLogDir<char>();
auto dir2 = DefaultLogDir<wchar_t>();

However, I'm not sure this is really necessary here. Can't you just always use the wide string variant here?

[xfc1939]

In the code above, DefaultDir sill uses duplicated logic while the only difference is the use of getenv for ANSI and Unicode strings. It is therefore more meaningful to provide a unified overload for the corresponding functions.

Something like this should do the job:

namespace details {
using std::getenv; // char variant
const wchar_t* getenv(const wchar_t* name) {
    return _wgetenv(name);
}
} // namespace internal

template<class Ch, std::size_t N>
std::basic_string<Ch> DefaultLogDir(const std::array<const Ch*, N>& names) {
    for (const Ch* name : names) {
        if ((const Ch* var = details::getenv(name)) != nullptr) {
            return var;
        }
    }
    return {};
}

Now you only need to specialize the literals:

template<class Ch>
struct LogDirEnvVar;

template<>
struct LogDirEnvVar<char> {
    constexpr static std::array<const char*, 2> names() noexcept {
        return {"GOOGLE_LOG_DIR", "TEST_TMPDIR"};
    }
};

template<>
struct LogDirEnvVar<wchar_t> {
    constexpr static std::array<const wchar_t*, 2> names() noexcept {
        return {L"GOOGLE_LOG_DIR", L"TEST_TMPDIR"};
    }
};

Eventually, you can define

template<class Ch>
decltype(auto) DefaultLogDir() {
    return DefaultLogDir(LogDirEnvVar<Ch>::names());
}

and use it as follows:

auto dir1 = DefaultLogDir<char>();
auto dir2 = DefaultLogDir<wchar_t>();

However, I'm not sure this is really necessary here. Can't you just always use the wide string variant here?

Thanks for your suggestions; it's a very nice thing for me. Regarding your question, I don't have much experience with platforms other than Windows, but I will try to do it.

[sergiud]

@xfc1939 Please do not close the PRs to create a new one. Simply update your branch and push the changes. Thank you.

[xfc1939]

@xfc1939 Please do not close the PRs to create a new one. Simply update your branch and push the changes. Thank you.

all right, I have updated my branch and sumited here.