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

update write_accessors in generate.py for bool #56

Closed
wants to merge 1 commit into from

Conversation

Naoki-Hiraoka
Copy link

@Naoki-Hiraoka Naoki-Hiraoka commented Nov 25, 2017

既出でしたら申し訳ありません。

1.irteusgl$ setq a (instance std_msgs::bool :init)
#<std_msgs::bool #X4dc8028>
2.irteusgl$ send a :data
nil
3.irteusgl$ send a :data t
t
4.irteusgl$ send a :data nil 
t
5.irteusgl$ send a :data
t

generate.pyによって作成されるeuslispのコードでは、引数にnilをいれるとデータが変化しない仕様になっており、上記のようにデータをnilに変更することができません。generate.pyによって下記のようなコードが生成されるためです。

(:data
   (&optional __data)
   (if __data (setq _data __data)) _data)

引数にnilを入れてもデータが変化しない仕様にはメリットもありますが、データの型がboolだった場合に限っては、一度trueにすると二度とfalseに戻せなくなってしまうというデメリットが大きいと感じます。データの型がboolだった場合に限り下記のようなコードを生成するようにしたいと思いwrite_accessors関数を書き換えてみたのですが、いかがでしょうか。

(:data
   (&rest __data)
   (if __data (setq _data (car __data))) _data)

@k-okada k-okada requested a review from furushchev November 26, 2017 02:15
@k-okada
Copy link
Member

k-okada commented Nov 26, 2017

確かに.このパターンは時々あって,
https://github.com/jsk-ros-pkg/jsk_robot/blob/master/jsk_baxter_robot/baxtereus/baxter-util.l#L5
みたいに対応しているところが多い気がします.

std_msgs::Bool は時々使うと思うけど,今までどうしていたのかな? @furushchev
あたらしくインスタンス使えば,この状況には陥らない,ということなのかな.

Copy link
Member

@furushchev furushchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for late response
If I understand correctly, this problem is related to euslisp/EusLisp#240?
I just created a pull request for addressing this (euslisp/EusLisp#254) , so you can now choose the solution from both side.
(Both are good to me though taking argument with &rest allows multiple arguments which is actially invalid)

If my pull request is merged, another solution for this problem should look like:

(:data
   (&optional (__data nil __data_supplied_p))
   (if __data_supplied_p (setq _data __data)) _data)

@furushchev
Copy link
Member

std_msgs::Bool は時々使うと思うけど,今までどうしていたのかな? @furushchev

趣味の違いだと思いますが,僕はいままで,bool型みたいなプリミティブに近い型は送らない使い方をしていたので,問題に思っていませんでした.(気づいてませんでした)

@Naoki-Hiraoka
Copy link
Author

お返事ありがとうございます。
euslisp/EusLisp#254
のmerge後になると思いますが、

(:data
   (&optional (__data nil __data_supplied_p))
   (if __data_supplied_p (setq _data __data)) _data)

を使用するバージョンを作成しました。

Copy link
Member

@furushchev furushchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Naoki-Hiraoka ありがとうございます!git rebaseしてコミットメッセージを1つのわかりやすいものにしていただけると嬉しいです.

@Naoki-Hiraoka
Copy link
Author

git rebaseしました。勉強になります。

k-okada added a commit to k-okada/geneus that referenced this pull request Jan 9, 2019
k-okada added a commit to k-okada/geneus that referenced this pull request Jan 9, 2019
@k-okada k-okada mentioned this pull request Jan 9, 2019
k-okada added a commit that referenced this pull request Jan 9, 2019
@k-okada
Copy link
Member

k-okada commented Jan 9, 2019

Thanks for contribution
Committed with test code by #61

Closed via #61

@k-okada k-okada closed this Jan 9, 2019
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