toeb / cmakepp

An Enhancement Suite for the CMake Build System
Other
436 stars 37 forks source link

cmakepp custom return() command gives warnings in some cases #127

Open Manu343726 opened 8 years ago

Manu343726 commented 8 years ago

Hi Tobias,

I'm having some issues related to the way you handled #3: Your return() command overrides the default CMake one so if you do a naked return() invocation in a module for example (As many CMake built-in modules do) you end up with warnings about the module not having a parent scope.

I would consider using a CACHE variable for __ans instead. I know this is not as performant as PARENT_SCOPE, but most scripts using cmakepp have already a noticeable slowdown by the temporary file creation for dynamic function invocation and the like. I don't specially care about performance when using cmakepp, I care about expressiveness and the huge amount of extra tools I have. So picking UX in favor of performance would be fine for me. What do you think?

Thanks.

toeb commented 8 years ago

Hi Manu,

You are right: the UX should be the prime focus for cmakepp and I too have had the issue with the warning that no PARENT_SCOPE exists when on the root level of scopes. There is no reliable way for detecting wether or not a PARENT_SCOPE exists and no way to suppress this warning to my knowledge (the latter would be the solution I would prefer most if it were possible)

There should not be too many places that __ans is exposed (in some performance critical code I use __ans directly without going through the ans() function. so a big search and replace on the cmakepp codebase should suffice in changing it. I would probably use the get_property(GLOBAL) set of functions to set this field. The behaviour of return however changes a bit: If a function a within a function b returns a value with set_property and the function b does not return a value it will look like b returned what a returned and this may be unintended.

Manu343726 commented 8 years ago

If a function a within a function b returns a value with set_property and the function b does not return a value it will look like b returned what a returned

Well, let's follow C++ conventions and call this "Undefined Behavior" ;) I don't expect a void function (A function that is not expected to return nothing, in the cmakepp context) to have a return value, so trying to get its return value has undefined behavior. We can document it as "If the function doesn't call return(<return value>), that is _it doesn't return , the value of ans() is undefined. Note returning an empty value (return()) is considered a return statement, so the value of ans() is well defined (Is set to be empty)."_

toeb commented 8 years ago

seems consistent (kinda) ans usages in cmakepp: 274 matches across 125 files

Manu343726 commented 8 years ago

that's ans() command, right? Not raw __ans.

toeb commented 8 years ago

no, I do mean raw __ans. I sometimes optimized the performance by not using ans() but rather __ans directly. The problem is that the main performance bottleneck in cmake is the function call. It does not really matter if you call a complex regex or read a small file compared to calling an empty function. (citation needed) that was my experience at least. So I started replacing all ${__ans} with get_property(__ans GLOBAL PROPERTY __ans) calls. I'll have to look what will happen with the performance...

I also noticed that some things might become faster because I do not have to set(__ans PARENT_SCOPE) at the end of functions which return the last function return value any more (cmakepp version of tail call optimization ;))

Manu343726 commented 8 years ago

no, I do mean raw ans. I sometimes optimized the performance by not using ans() but rather ans directly.

Ah, I see.

So I started replacing all ${__ans} with get_property(ans GLOBAL PROPERTY ans)

really? My idea was to submit a PR this weekend with the changes :)

(cmakepp version of tail call optimization ;))

Haha, we can start sketching an optimizing backend for your cmake transpiler :P

toeb commented 8 years ago

Ahh well if your already doing it then i'd be happy to accept the PR :D if I do it I do not know how long it will take :/