ice icon indicating copy to clipboard operation
ice copied to clipboard

Remove Support for the `tag` Keyword

Open InsertCreativityHere opened this issue 1 year ago • 4 comments

In the "Ice 4.0" days, we replaced the optional keyword to tag, but kept optional for backwards compatibility. We kept separate tokens for them so we could emit deprecation warnings for optional. Different behavior == different tokens.

Now that "Ice 3.8" is the target, I see no point in supporting tag, so this PR deletes it. This keyword doesn't exist in 3.7, and there's no reason for anyone to switch from optional to tag.

There isn't much to review just wanted to make sure everyone was aware of & okay with this.


Note that we already removed the deprecation warning. Or maybe it never got backported in the first place. Without it there is no difference in behavior between the tokens. The code is literally copy/paste duplicates.

InsertCreativityHere avatar May 14 '24 20:05 InsertCreativityHere

I thought our idea was to use the word tag going forward to limit confusion with "optional" and that tag would be prefered. Pretty sure we're using "tag" in various Input/Output streams and in some Slice.

Is there an issue/discussion for removing this?

externl avatar May 14 '24 20:05 externl

I thought our idea was to use the word tag going forward to limit confusion

That's going to be a tall order. Updating all our code/docs to use tag. Especially because we already use the word tag to refer to the number you assign an optional field. As in: "You can mark a field as optional, and assign it a tag number with: optional(tagNum)"

Pretty sure we're using "tag" in various Input/Output streams and in some Slice.

We're only using tag in one place. A function in Router.ice.

Is there an issue/discussion for removing this?

No, I can make a proper issue if that's preferable to discussing it here on this PR. Me and Bernard were just discussing it earlier, but indeed, only in person.

InsertCreativityHere avatar May 14 '24 20:05 InsertCreativityHere

I'm fine to remove it I guess but I'm pretty sure we've been sprinkling the word tag all around the code since we started on 3.8 and added tag as an alias for optional.

https://github.com/zeroc-ice/ice/blob/main/cpp/include/Ice/StreamHelpers.h#L396

I'm sure there are more.

This commit introduced several that should be undone. https://github.com/zeroc-ice/ice/commit/885ad85d85086ed2aafea6b2df5a30c641ea96fc

externl avatar May 14 '24 20:05 externl

Yes, you're right. I'll look around the code for any references to tag. And remove those too, if everyone is okay with this going forward in the first place.

InsertCreativityHere avatar May 14 '24 20:05 InsertCreativityHere

We had a separate issue open for removing the 'tagged's that snuck in: https://github.com/zeroc-ice/ice/issues/1623 I'm posting it here to link the two, and just to quote from it: however, the standard term remains 'optional'

InsertCreativityHere avatar May 20 '24 16:05 InsertCreativityHere

I altered this PR to also fix any mentions of the tagged keyword from icerpc. I think it's pretty trivial, so I'm not remarking reviewers, but feel free to re-re-view if you want!!

InsertCreativityHere avatar May 20 '24 16:05 InsertCreativityHere