Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow greedy property evaluation #284

Merged
merged 4 commits into from
Oct 2, 2021
Merged

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Oct 1, 2021

This can be used, to redefine a property from its previous value, e.g. for normalization:

<xacro:property name="prop" value="${prop.lower()}" greedy="true"/>

Without this option, due to lazy evaluation, it would be an attempt to recursively evaluate the property.
@gavanderhoorn + @guihomework: Please comment, particularly about the attribute name. Alternatively, we could use lazy="false".

UPDATE: I decided for the attribute lazy_eval="false" now.

TODO

  • Add unit tests
  • Point out the new attribute in case of a 1-hop circular evaluation.

@guihomework
Copy link

@rhaschke I discovered the possible terrible consequences of lazy evaluation today, and due to that, broke code.
I have no idea if the lazy evaluation happening behind the hood is known to basic users, so I wonder if they would ever know they need to explicitly set a new attribute, and in which case exactly.
I had to double check the translation of "greedy" (english is not my first language) but still cannot relate the adjective "greedy" to the absence of laziness.
if this is all about allowing to redefine properties, I would use "redefine" = true.

@rhaschke
Copy link
Contributor Author

rhaschke commented Oct 1, 2021

If this is all about allowing to redefine properties, I would use "redefine" = true.

No, it is more general than that. It just enforces immediate evaluation of the value expression of the to-be-defined property.

I discovered the possible terrible consequences of lazy evaluation today.

Could you become more precise? What was so terrible about lazy evaluation?
Lazy evaluation is crucial to have! Otherwise, you can't declare macro parameter defaults and properties using variables that would get defined only later.

@guihomework
Copy link

If this is all about allowing to redefine properties, I would use "redefine" = true.

No, it is more general than that. It just enforces immediate evaluation of the value expression of the to-be-defined property.

then lazy=false seems appropriate

I discovered the possible terrible consequences of lazy evaluation today.

Could you become more precise? What was so terrible about lazy evaluation?

I suppose because I am used to C++ compilation helping me find my mistakes, and when there are some mistakes in property definition that do not get evaluated (lazy evaluation), xacro expansion and error checkings do not show up those mistakes. I suppose the problem is not in lazy evaluation but on the person who needs to account for its consequences before hand.

This can be used, to redefine a property from its previous value, e.g. for normalization:
```xml
<xacro:property name="prop" value="${prop.lower()}" greedy="true"/>
```
Without this option, due to lazy evaluation, it would be an attempt to
recursively evaluate the property.
... if lazy evaluation results in circular resolution
@rhaschke rhaschke changed the base branch from noetic-devel to melodic-devel October 2, 2021 18:51
@rhaschke
Copy link
Contributor Author

rhaschke commented Oct 2, 2021

Re-trigger CI

@rhaschke rhaschke closed this Oct 2, 2021
@rhaschke rhaschke reopened this Oct 2, 2021
@rhaschke rhaschke merged commit fb4d227 into ros:melodic-devel Oct 2, 2021
@rhaschke rhaschke deleted the greedy-eval branch October 2, 2021 19:35
@gavanderhoorn
Copy link
Contributor

Looks like a straightforward enough addition.

UPDATE: I decided for the attribute lazy_eval="false" now.

👍 to this. As evidenced by the comments posted by @guihomework, the term greedy is not universally understood :)

lazy_eval might also be unfamiliar, but at least it gives a hint as to what it controls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants