Remove unsafety in ResourceId creation

[DasLixou]

Make atomic ID_COUNTER increment match xilem style and clear confusion with safety comment

[DasLixou]

Oh wait we increment after.. the comment just irritated me.

[PoignardAzur]

I'd actually be okay to merge this after removing the val + 1 part. I think your version is clearer.

[DasLixou]

https://github.com/linebender/xilem/blob/072358e2933da4a814b9b45e74de2e4c23c873ec/src/id.rs#L27
https://github.com/linebender/xilem/blob/072358e2933da4a814b9b45e74de2e4c23c873ec/crates/xilem_core/src/id.rs#L20

that would also match xilem

[xStrom]

I would add a safety comment to the dangerous initialization spot. If we later add code inbetween the variable initialization and the unsafe block, then it's easier to miss that it is crucial that it is initialized as 1.

[DJMcNab]

I'd rather just match Xilem exactly, i.e. use .try_into().unwrap(). This is our only usage of unsafe, which it would be nice to remove.

• We're nowhere near CPU bottlenecked;
• this will be called very few times a frame (~15?); and,
• it is only a single comparison which should be branch predicted well

Also, that title is probably too long. If you apply this suggestion, I'd change it to:

Remove unsafety in ResourceId allocation

[xStrom]

Also, that title is probably too long.

Yeah best to keep PR titles at 64 chars or below, otherwise it will be elided in the commit list on the GitHub web interface.

[DasLixou]

I'm not 100% for removing unsafe "just because it's the only one for now" and it's still safe and safes performance and binary size in theory

[DJMcNab]

Thanks! In this case, the unsafe code was safe for all reasonable scenarios, but it's still good to not get in the habit of using it (and it was technically unsound).

[xStrom]

@DasLixou I sent you a GitHub invite for getting write access to this repo.

We have a policy of PR author pressing the merge button. Once you accept the invite you can press it here.

The reason behind this policy is that sometimes the PR author may want to wait for multiple reviews, or for a review from a specific person. Or they may want to do some final polish.

Sometimes when the PR is very trivial, e.g. just a single typo fix, then it's fine for a reviewer to just merge it.

[DasLixou]

Accepted the invitation, still for some PRs like #516 I just like to have as much reviews and opinions as possible. Also, when I get inactive or something you can just kick me. But is this an indirect call to merge this?

[Philipp-M]

But is this an indirect call to merge this?

It got an approval, so yes?

[xStrom]

Yes you can merge this. 🙂