2016-07-01 15:25 GMT+03:00 Daniel Pfeifer <dan...@pfeifer-mail.de>: > Hi Dāvis, > > On Fri, Jul 1, 2016 at 4:18 AM, Dāvis Mosāns <davis...@gmail.com> wrote: > > On Windows getenv uses ANSI codepage so it needs to be encoded to > > internally used encoding (eg. UTF-8). Here we use _wgetenv instead > > and encode that. > > Your change to the SystemTools::GetEnv function introduces memory > leaks, since you return a pointer to a new[]-ed array. > It seems impossible to refactor SystemTools::GetEnv to use _wgetenv > and provide interface compatability. > You should probably introduce a function that returns std::string and > uses GetEnvironmentVariableW internally. > > I know about memory leak, I intentionally left it this way for first version of patch as there are multiple ways how to implement it properly.
POSIX's getenv doesn't need to be freed, but here for Windows since we need to encode it to internal encoding we'll be returning a new pointer. I see two ways: 1. add SystemTools::FreeEnv which delete's if on Windows but does nothing otherwise; 2. change GetEnv to return std::unique_ptr<std:string> which will be automatically deleted once out of scope and we still can check if there wasn't such env if it's empty. To me 2nd option seems best.
-- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
