On Thu, 2020-04-16 at 15:22 +0200, Wolfram Sang wrote:
>
> +if(ENABLE_STACKTRACE)
> + set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "'Debug' because of
> stacktrace" FORCE)
> +endif()
Agreed on the motivation for the activity, but not on the
implementation. CMAKE_BUILD_TYPE is an internal control variable
of the cmake process. Its "Debug" value is an appropriate choice
here. The "CACHE STRING" keywords keep the choice user adjustable
and FORCE probably is required or else the (repeated) assignment
to the special "protected" variable won't take effect.
The part that I disagree with is the help text. This one is for
the parameter itself, here for CMAKE_BUILD_TYPE. Not for its
current value, here "Debug". Have checked what non-forced cmake
caches contain, they got
"Choose the type of build (None, Debug, Release, RelWithDebInfo, MinSizeRel)."
I guess your patch should stick with this help text. Because
"Debug" is not the only valid and appropriate choice. Anything
with debug info might be acceptable.
Wait, is the "RelWithDebInfo" as good a choice, and already gets
enforced in the absence of a user spec? And is accepting the user
spec -- when provided -- the most appropriate activity? Does it
mean that strictly speaking this specific change is not needed?
virtually yours
Gerhard Sittig
--
If you don't understand or are scared by any of the above
ask your parents or an adult to help you.
_______________________________________________
sigrok-devel mailing list
sigrok-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sigrok-devel
