[CMake] function and raise_scope commands

[Miguel A. Figueroa-Villanueva]

On Dec 28, 2007 12:52 PM, Miguel A. Figueroa-Villanueva wrote:
> On Dec 28, 2007 11:36 AM, Ken Martin wrote:
> > > > > 1. CMake crashes if I use the same variable name as the argument and
> > > > > raise the scope later. That is, for the following function:
>
> <snip>
>
> > Specifically
> >
> > Variable  value
> > --------------------
> > is_changed  is_changed
> >   set(${is_changed} "some_result")
> > is_changed some_result
> >   raise_scope(${is_changed})
> >
> > well this last line becomes raise_scope(some_result) and since there is no
> > local variable named some_result it yerks. I have fixed the crash and it now
> > prints a nice warning message (on my local copy of cmake, not checked in
> > yet) but I'm not sure that is all the solution. I think a safer fix may be
> > to change the raise scope command to look like the following:
> >
> > raise_scope(var_name value var_name value ...)
> >
> > with a convention that you do not set variables that are to be returned (aka
> > passed by reference). In this case that would be is_changed. You leave
> > is_changed alone and only use it in the raise scope command. So the
> > resulting code would look like.
> >
> > function(track_find_variable cache_variable is_changed)
> >    set(changed "some_result")
> >    raise_scope(${is_changed} "${changed}")
> > endfunction(track_find_variable)
> >
> > which is safer. But I am still want to think about it a bit before I commit
> > the change.
>
> Well, I think the above is fine, but it's the convention of not using
> the variable that is solving the problem not the command syntax. That
> is, if you do a set(is_changed ...) before the raise_scope you still
> get into trouble.
>
> I think printing a warning when the argument contains the same name as
> the variable, and just have the convention of putting an underscore in
> front of the argument should work as well:
>
> function(track_find_variable _cache_variable _is_changed)
>
> At least, that is what I'm doing and it is simple to stick to it.

Hello Ken,

I see that you have applied the changes to cmake/cvs. With the new
syntax of raise_scope I now run to an unintuitive behaviour, which
should at least be emphasized in the documentation.

The problem is that when you do a raise_scope(var value), you don't
get a local copy of the variable. This is fine when you have direct
access to the variable, but when you are doing things in a loop it
gets into problematic behavior. I guess it is easier to demonstrate
with an example (pseudo-code/simplified version of my actual code):

# valid args: MAIN_INPUT;BIBFILES;...
foreach(arg ${ARGN})
  if(${arg} ... is valid ...)
    raise_scope(${arg} ${current_arg_list})
    set(current_arg_list)
  else(${arg} ... is valid ...)
    list(APPEND current_arg_list ${arg})
  endif(${arg} ... is valid ...)
endforeach(LIB)

# now some are not only valid, but required
if(${MAIN_INPUT} ...is empty...)     # PROBLEM: here the local
MAIN_INPUT will always be empty
  message(FATAL_ERROR "You need to provide a MAIN_INPUT file!")
endif(${MAIN_INPUT} ...is empty...)


Of course, you can solve this by doing a local set of MAIN_INPUT_LOCAL
(if you set a local variable with the same name, MAIN_INPUT, then we
are facing the same problem we were solving) in parallel with the
raise_scopes. That is:

# valid args: MAIN_INPUT;BIBFILES;...
foreach(arg ${ARGN})
  if(${arg} ... is valid ...)
    set(LOCAL_${arg} ${current_arg_list})
    raise_scope(${arg} ${current_arg_list})
    set(current_arg_list)
  else(${arg} ... is valid ...)
    list(APPEND current_arg_list ${arg})
  endif(${arg} ... is valid ...)
endforeach(LIB)

# now some are not only valid, but required
if(${LOCAL_MAIN_INPUT} ...is empty...)     # PROBLEM: here the local
MAIN_INPUT will always be empty
  message(FATAL_ERROR "You need to provide a MAIN_INPUT file!")
endif(${LOCAL_MAIN_INPUT} ...is empty...)

Again, I think this behaviour is a quite unintuitive and should be
well documented, at least.

--Miguel