[cmake-developers] CMakeParseArguments: Do not skip empty arguments

[Alexander Neundorf]

On Wednesday 06 November 2013, Daniele E. Domenichelli wrote:
> On 05/11/13 20:51, Alexander Neundorf wrote:
> >> I don't know if this is to be considered a change of behaviour though,
> >> but I'd rather consider it a bug, and I would like to see it fixed in
> >> the next bugfix release... what do you think?
> > 
> > cmake_parse_arguments() is used in quite a few places in the meantime, so
> > I don't really feel good about changing its behaviour in general.
> > Maybe an option for the macro what it should do with empty values ?
> 
> Is there any of the other place where cmake_parse_arguments() has to
> deal with with empty arguments? 

We cannot know what users are doing with it.

> I believe that someone would have
> complained about that. From what I've seen, modules that are supposed to
> deal with empty arguments (like ExternalProject that can have arguments
> like INSTALL_COMMAND "") do not use CMakeParseArguments...
> 
> I don't think that anyone would use something like
>   SINGLE "" VALUE
> to assign VALUE to the SINGLE option...
> Also something like
>   SINGLE "${EMPTY_VARIABLE}" NEXT_ARG
> would assign to FOO_SINGLE the value NEXT_ARG
> and that is very likely to be an unintended behaviour.
> 
> Perhaps someone could use something like
>   MULTI "${V1}" "${EMPTY_VARIABLE}" "${V2}"
> and expect to get a list containing only V1 and V2. Nonetheless if he is
> using foreach(arg ${list}), the behaviour won't change because empty
> list elements are skipped. if instead he is using "foreach(arg IN LIST
> list)", he is probably expecting to receive empty arguments, since the
> documentation clearly states so.
> 
> Adding an option in the macro arguments would be a bit of a
> non-compatible change, since the name of the option could no longer be
> used as one of the "OPTION/SINGLE/MULTI" value names.

This is all correct.
It seems unlikely that changing this will actually break something.
Adding an option like "DONT_SKIP_EMPTY_ARGUMENTS", e.g at the second argument 
position, means that this keyword cannot be used as an option name by users 
anymore. By making the keyword longer or more detailled, this becomes less 
likely. E.g. I'd assume nobody is using "CMAKE_DONT_SKIP_EMPTY_ARGUMENTS".

Adding proper named argument handling to cmake_parse_arguments() itself is 
somewhat complicated since it can't make use of cmake_parse_arguments() ;-)
 
The macro could check ${CMAKE_MINIMUM_REQUIRED_VERSION} whether 2.8.13/3.0.0 
is required, and switch behaviour based on that. Or via a policy. This would 
be the normal wy to handle such a change.
I'm not sure whether this wouldn't be a bit much for a change which most 
likely doesn't break anything anyway.

But... I've had my share of changes which seemed to be safe, and then somebody 
came along with a setup which was broken by this change nevertheless.

So, I'll have a slightly bad feeling if you commit it without additional 
checks. Probably it will be ok.
If you commit it with a policy, everything should be fine.

Alex