[cmake-developers] [PATCH v2] Improve encoding handling on Windows

[Mike Gelfand]

For std::c(in|out|err) vs. std::wc(in|out|err), here's an implementation
of std::streambuf which we currently use in our projects:
http://stackoverflow.com/a/21728856/583456. You could either try using
it as is or use it as a base for your own implementation; in any case,
it's better than forcing people to use one or another stream based on
current OS.

Not sure what currently accepted CMake coding style dictates, but
several variables could be made const (e.g. `envVarSet` in
`cmExtraEclipseCDT4Generator::AddEnvVar`).

Apart from notes above and a few indentation issues which may be fixed
during merge, below are some more observations.

On 07/03/2016 03:29 AM, Dāvis Mosāns wrote:
> --- a/Source/CTest/cmCTestCurl.cxx
> +++ b/Source/CTest/cmCTestCurl.cxx
> @@ -238,12 +238,11 @@ void cmCTestCurl::SetProxyType()
>          this->HTTPProxyType = CURLPROXY_SOCKS5;
>        }
>      }
> -    if (cmSystemTools::GetEnv("HTTP_PROXY_USER")) {
> -      this->HTTPProxyAuth = cmSystemTools::GetEnv("HTTP_PROXY_USER");
> -    }
> -    if (cmSystemTools::GetEnv("HTTP_PROXY_PASSWD")) {
> +    cmSystemTools::GetEnv("HTTP_PROXY_USER", this->HTTPProxyAuth);
> +    std::string passwd;
> +    if (cmSystemTools::GetEnv("HTTP_PROXY_PASSWD", passwd)) {
>        this->HTTPProxyAuth += ":";
> -      this->HTTPProxyAuth += cmSystemTools::GetEnv("HTTP_PROXY_PASSWD");
> +      this->HTTPProxyAuth += passwd;
>      }
>    }
>  }
I guess the logic was a bit flawed to begin with. The function doesn't
look reenterable so I would presume that it's being called with empty
`this->HTTP*` variables. Adding asserts wouldn't hurt (though not in
terms of this patch).

> --- a/Source/cmBuildCommand.cxx
> +++ b/Source/cmBuildCommand.cxx
> @@ -109,10 +109,7 @@ bool cmBuildCommand::TwoArgsSignature(std::vector<std::string> const& args)
>    const char* cacheValue = this->Makefile->GetDefinition(define);
>  
>    std::string configType = "Release";
> -  const char* cfg = getenv("CMAKE_CONFIG_TYPE");
> -  if (cfg && *cfg) {
> -    configType = cfg;
> -  }
> +  cmSystemTools::GetEnv("CMAKE_CONFIG_TYPE", configType);
>  
>    std::string makecommand =
>      this->Makefile->GetGlobalGenerator()->GenerateCMakeBuildCommand(
Here you have a slight change in logic. Assuming environment variable
could be present but empty, I would rewrite this piece to:

std::string configType;
if (!cmSystemTools::GetEnv("CMAKE_CONFIG_TYPE", configType) ||
configType.empty())
  configType = "Release";

You already have this code pattern some place else.

> --- a/Source/cmCLocaleEnvironmentScope.cxx
> +++ b/Source/cmCLocaleEnvironmentScope.cxx
> @@ -31,8 +31,8 @@ cmCLocaleEnvironmentScope::cmCLocaleEnvironmentScope()
>  
>  std::string cmCLocaleEnvironmentScope::GetEnv(std::string const& key)
>  {
> -  const char* value = cmSystemTools::GetEnv(key);
> -  return value ? value : std::string();
> +  std::string value;
> +  return cmSystemTools::GetEnv(key, value) ? value : std::string();
>  }
>  
>  void cmCLocaleEnvironmentScope::SetEnv(std::string const& key,
Assuming there's NRVO in place, ignoring return value of
`cmSystemTools::GetEnv` call might be a better choice; if the call
fails, `value` stays empty, so you could return `value` regardless.

> --- a/Source/cmSetCommand.cxx
> +++ b/Source/cmSetCommand.cxx
> @@ -31,7 +31,11 @@ bool cmSetCommand::InitialPass(std::vector<std::string> const& args,
>      putEnvArg += "=";
>  
>      // what is the current value if any
> -    const char* currValue = getenv(varName);
> +    std::string scurrValue;
> +    const char* currValue = 0;
> +    if (cmSystemTools::GetEnv(varName, scurrValue)) {
> +      currValue = scurrValue.c_str();
> +    }
>      delete[] varName;
>  
>      // will it be set to something, then set it
Could be changed to std::string + bool instead. Two pointer checks which
follow will then use the bool value, and `strcmp()` call will be
replaced with plain `currValue != argv[1]`.

> --- a/Source/cmake.cxx
> +++ b/Source/cmake.cxx
> @@ -1708,8 +1702,8 @@ int cmake::CheckBuildSystem()
>    // the make system's VERBOSE environment variable to enable verbose
>    // output. This can be skipped by setting CMAKE_NO_VERBOSE (which is set
>    // by the Eclipse and KDevelop generators).
> -  bool verbose = ((cmSystemTools::GetEnv("VERBOSE") != CM_NULLPTR) &&
> -                  (cmSystemTools::GetEnv("CMAKE_NO_VERBOSE") == CM_NULLPTR));
> +  bool verbose = ((cmSystemTools::HasEnv("VERBOSE")) &&
> +                  (!cmSystemTools::HasEnv("CMAKE_NO_VERBOSE")));
>  
>    // This method will check the integrity of the build system if the
>    // option was given on the command line.  It reads the given file to
Extra parentheses weren't exactly needed before, and are certainly
useless now ;)

> --- a/Source/cmcmd.cxx
> +++ b/Source/cmcmd.cxx
> @@ -701,8 +701,8 @@ int cmcmd::ExecuteCMakeCommand(std::vector<std::string>& args)
>        // verbose output. This can be skipped by also setting CMAKE_NO_VERBOSE
>        // (which is set by the Eclipse and KDevelop generators).
>        bool verbose =
> -        ((cmSystemTools::GetEnv("VERBOSE") != CM_NULLPTR) &&
> -         (cmSystemTools::GetEnv("CMAKE_NO_VERBOSE") == CM_NULLPTR));
> +        ((cmSystemTools::HasEnv("VERBOSE")) &&
> +         (!cmSystemTools::HasEnv("CMAKE_NO_VERBOSE")));
>  
>        // Create a cmake object instance to process dependencies.
>        cmake cm;
Same as above. In fact, this is a copy-paste code, someone should add a
helper I suppose.

> --- a/Source/kwsys/SystemTools.cxx
> +++ b/Source/kwsys/SystemTools.cxx
> @@ -456,22 +454,45 @@ void SystemTools::GetPath(std::vector<std::string>& path, const char* env)
>      }
>  }
>  
> -const char* SystemTools::GetEnv(const char* key)
> +bool SystemTools::GetEnv(const char* key, std::string& result)
>  {
> -  return getenv(key);
> +#if defined(_WIN32)
> +  std::wstring wkey = Encoding::ToWide(key);
> +  wchar_t* wv = _wgetenv(wkey.c_str());
> +  if (wv) {
> +    result = Encoding::ToNarrow(wv);
> +    return true;
> +  }
> +  return false;
> +#else
> +  const char* v = getenv(key);
> +  if(v)
> +    {
> +    result = v;
> +    return true;
> +    }
> +  else
> +    {
> +    return false;
> +    }
> +#endif
>  }
Short-circuiting would be nice, no point in code like `if (...) { ...
return A; } else { ... return B; }`.

> ...
>
> -bool SystemTools::GetEnv(const char* key, std::string& result)
> +bool SystemTools::HasEnv(const char* key)
>  {
> +#if defined(_WIN32)
> +  std::wstring wkey = Encoding::ToWide(key);
> +  wchar_t* v = _wgetenv(wkey.c_str());
> +#else
>    const char* v = getenv(key);
> +#endif
>    if(v)
>      {
> -    result = v;
>      return true;
>      }
>    else
Looks like simple `return v != CM_NULLPTR;` to me.

> ...
>
> @@ -4820,12 +4851,11 @@ int SystemTools::GetTerminalWidth()
>      {
>      width = -1;
>      }
> -  columns = getenv("COLUMNS");
> -  if(columns && *columns)
> +  if(SystemTools::GetEnv("COLUMNS", columns))
>      {
>      long t;
>      char *endptr;
Yet another small change in behavior. Needs ` && !columns.empty()`
added. Although I see why you may have omitted it intentionally...
Granted, saving one call to strtol may not be worth it.

> --- a/Source/kwsys/SystemTools.hxx.in
> +++ b/Source/kwsys/SystemTools.hxx.in
> @@ -839,10 +839,10 @@ public:
>    /**
>     * Read an environment variable
>     */
> -  static const char* GetEnv(const char* key);
> -  static const char* GetEnv(const std::string& key);
>    static bool GetEnv(const char* key, std::string& result);
>    static bool GetEnv(const std::string& key, std::string& result);
> +  static bool HasEnv(const char* key);
> +  static bool HasEnv(const std::string& key);
>  
>    /** Put a string into the environment
>        of the form var=value */
As other people seem to be concerned with API compatibility (in kwsys in
particular), it might be better to leave existing "const char*
SystemTools::GetEnv(const char* key)" and "const char*
SystemTools::GetEnv(const std::string& key)" signatures around but mark
them with deprecation pragma to discourage their use?

> --- a/Source/kwsys/testSystemTools.cxx
> +++ b/Source/kwsys/testSystemTools.cxx
> @@ -848,9 +848,9 @@ static bool CheckPutEnv(const std::string& env, const char* name, const char* va
>                      << "\") failed!" << std::endl;
>      return false;
>      }
> -  const char* v = kwsys::SystemTools::GetEnv(name);
> -  v = v? v : "(null)";
> -  if(strcmp(v, value) != 0)
> +  std::string v = "(null)";
> +  kwsys::SystemTools::GetEnv(name, v);
> +  if(strcmp(v.c_str(), value) != 0)
>      {
>      std::cerr << "GetEnv(\"" << name << "\") returned \""
>                      << v << "\", not \"" << value << "\"!" << std::endl;
As you're using std::string now, `if (v != value)` might be a better
(cleaner) alternative.

Regards,
Mike