Re: [cmake-developers] [PATCH v2] Improve encoding handling on Windows
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 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
> 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&
> args)
>// verbose output. This can be skipped by also setting CMAKE_NO_VERBOSE
>// (which is set by the Eclipse and KDevelo
Re: [cmake-developers] cmake -E capabilities
Brad King wrote: > On 07/01/2016 05:11 AM, Tobias Hunger wrote: >>> For "multiconfig", there is the cmGlobalGenerator::IsMultiConfig method. >> I do not see why this information is need to set up a cmake project. I >> need two directories and a generator. Do I need to know what the >> generator produces at that point? > > If we don't foresee a use case for it we don't need to add it now. The > example given in the issue was just a quick demo IIUC and not intended to > be authoritative result of deep design thought. I can confirm this. >> If "-A" is the way to go, then I would like to try my hand at adding >> a method to retrieve all valid platforms for a generator. CMake has >> a lot of code to find those names (or hardcodes known names), so why >> not pass it on to the user? > > If it is reasonably straightforward to do then go for it. That would > be nice to offer. Does Visual Studio have a drop-down menu or similar for this? If yes, then such a list would be good to have I think. Thanks, Steve. -- 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
Re: [cmake-developers] cmake -E capabilities
Tobias Hunger wrote: > Either we should have multiConfig return a list of configuration names > that will be generated or I do not see any need to have the information in > the first place. I think when using qmake with QtCreator, the user can choose the configuration at build time by selecting it from the left-side menu. Such a feature would also work with cmake projects if the user chooses to use the XCode generator on mac or VS generator on Windows (or if someday we have a multi-config Ninja generator or so). However, I don't think that is an immediate goal, and that information can be added later easily if a client does need it. Thanks, Steve. -- 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
Re: [cmake-developers] cmake -E capabilities
Tobias Hunger wrote:
> Hello CMake Developers!
>
> As laid out in the last mail thread about daemon-mode in CMake (for your
> reference:
> http://public.kitware.com/pipermail/cmake-developers/2016-June/028777
> .html ), Stephen and me agreed that we needed a way for IDEs to figure out
> which generators are available to cmake and more static information built
> into CMake.
Here's a gmane link to the thread for reference:
http://thread.gmane.org/gmane.comp.programming.tools.cmake.devel/16782
> There is actually a bug report about the need for this feature here:
> https://gitlab.kitware.com/cmake/cmake/issues/15462
>
> This is my attempt to solve the issue:
> https://github.com/hunger/CMake/commits/cmake-capabilities
Thanks for working on this.
> The output looks like this:
>> cmake -E capabilities --pretty-print
> {
> "generators" : [
> {
> "extraGenerators" : [],
Interesting. I didn't realize the "Watcom WMake" generator does not work
with 'extra' generators.
> "KDevelop3",
This generator should probably be removed/hidden by now. It has
confused users of more-recent KDevelop versions.
> What do you think? What else should we report here?
It looks like a good start. The intention of the output is to satisfy needs
that consumers like you have, and I guess it does have what you need.
> Compared to the bug report mentioned above the fields "multiconfig" and
> "recursive" are missing. I could not figure out how to get that
> information:-/
When I made the example in the issue tracker, 'recursive' meant 'you can cd
to a directory and run the CMAKE_MAKE_PROGRAM there'. At the time, it was
possible to do that when using the Makefiles generator, but not the Ninja
generator.
I think that capability has since been added for the Ninja generator, but I
don't know if it is possible with the Xcode and VS generators.
Would that be a useful thing to expose here in your view?
> I would also welcome some code review on the patch.
1) For consistency you should change
GetRegisteredGenerators
to
this->GetRegisteredGenerators
whether that is a preferred style is orthogonal to the fact that it's
consistently used in cmake code.
2) CMake has to build with toolchains which do not provide
std::unordered_map. See uses of CMake_HAVE_CXX_UNORDERED_MAP for existing
code which uses either std::unordered_map or std::unordered_map. (Yes there
is room for improvement there, but such improvement is orthogonal to your
branch).
3) Similarly, CMake has to build with compilers which do not support
cxx_range_for or cxx_auto_type. For the cmServer implementation that may or
may not be the case, but within the cmake class, that's the way it is.
4) It seems that the
this->Generators
member of the cmake class has the names of all generators (without 'extra'
generators). It seems unfortunate to not use that container and instead
parse the names out of the names from the GetRegisteredGenerators call by
splitting on the ' - '. It leads to hard coded magic expressions like
'separator + 3', so it should be avoided if possible.
Is there another way of determining the extra generators supported by a
given generator and avoiding parsing a string which we just generated? Can
const std::vector& supportedGlobalGenerators =
extraGenerator->GetSupportedGlobalGenerators();
be used somehow? Can you first loop over this->Generators to get the
'normal' generators, then loop over the extra generators, call that method
to match things up and output the result?
Or would it make sense to refactor the container members in the cmake class
themselves to make this information more easily available for this use-case?
5) You use the term 'appendix' for the version, but 'suffix' is the more-
commonly used name for that concept.
Thanks,
Steve.
--
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
Re: [cmake-developers] [PATCH v2] Improve encoding handling on Windows
2016-07-03 13:04 GMT+03:00 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. > > Huge thanks for review! Will fix mentioned issues in next version of patch. Also I'll implement this solution with std::streambuf as it's much better way and it's actually not that much work I thought it would be. -- 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
