search

uml-robotics / ROS.NET

ROS.NET: ROS Client Library for Windows Development in C#
BSD 2-Clause "Simplified" License
93 stars 52 forks source link

YAMLParser: Why can't strings be constants? #51

Open YaseenAlk opened 6 years ago

YaseenAlk commented 6 years ago

I was reading through some of the code in GenerationGuts and noticed the following block of code (lines 1322-1325):

if (!type.Equals("string", StringComparison.InvariantCultureIgnoreCase)) { prefix = "const "; }

According to blame, the code came from this commit. I understand that most of the YAMLParser code is over 4 years old, and is overdue for a rewrite, but I'm just wondering what the reasoning was for this explicit case. I personally like using string constants in a static context, so I commented out the string check. Running YAMLParser against common_msgs seems to be fine. Are there any weird edge cases that could arise from this decision?

Thank you!

nuclearmistake commented 6 years ago

Try having a constant string in your definition and see how it behaves with or without the check? Are constantly strings allowed by other ROS clients?

Const is really a compile-time check (other than also controlling what gets sent in the serialized message in the rosmsg case)...

Wouldn't a couple byte constants and a variable be faster?

The code may have been a quick fix for something trying to assign to the string during initialization or something.

Feel free to experiment with string constancy.

On Mon, Aug 6, 2018, 1:01 PM Yaseen Alkhafaji notifications@github.com wrote:

I was reading through some of the code in GenerationGuts and noticed the following block of code (lines 1322-1325):

if (!type.Equals("string", StringComparison.InvariantCultureIgnoreCase)) { prefix = "const "; }

According to blame, the code came from this commit https://github.com/uml-robotics/ROS.NET/commit/b1f0c8d1277f319113b6c00d7304c9e9832bdd67 . I understand that most of the YAMLParser code is over 4 years old, and is overdue for a rewrite, but I'm just wondering what the reasoning was for this explicit case. I personally like using string constants in a static context, so I commented out the string check. Are there any weird edge cases that could arise from this decision?

Thank you!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/uml-robotics/ROS.NET/issues/51, or mute the thread https://github.com/notifications/unsubscribe-auth/ABLrW-8R1hJIeQEKhS5A5nV2ksrl0vCfks5uOHZZgaJpZM4VwvxE .

YaseenAlk commented 6 years ago

Thank you for the very quick reply!

I'm new to ROS, so I'm not sure if string constants are actually used in practice. But I do remember that I needed to adjust some lines in GenerationGuts in the past. They're in one of the commits in the PR I made a few days ago.

Removing the string check, while including the above commit, runs successfully on common_msgs (although I've been doing all of my testing on .NET Core 😝).