YAMLParser: Why can't strings be constants?

[YaseenAlk]

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]

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 ***@***.***> 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 <b1f0c8d> . 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 <#51>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABLrW-8R1hJIeQEKhS5A5nV2ksrl0vCfks5uOHZZgaJpZM4VwvxE> .

[YaseenAlk]

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 😝).