Closed moiseev-signal closed 1 year ago
For 4.x I would suggest adding #[cfg_attr(curve25519_dalek_warn_unused), must_use)] and adopt as default in 5.x
TBH, this would be overkill. std itself adds must_use
annotations all the time; it's not considered breaking.
Ya - though never remember seeing in a patch release though - would think minor bump makes sense most here perhaps
must_use
isn't breaking. It generates a warning. Warnings are specifically designed so new ones can be added without breaking builds.
While -D warnings
changes that, it's opt-in for that very reason.
That makes sense yes, I was being pedantic for no reason :sweat_smile:
Sorry, I might have been hasty on this. I didn't realize @tarcieri hadn't approved. Should I roll back?
I forgot to hit approve, sorry. I agreed in spirit in my comment
If I would think SemVer pedantically - I would think this is a breaking API behaviour change -
Since it's be damned if you don't and be damned if you do situation since SemVer isn't always practical :)
And since it's still a mandatory compiler warning at the build stage and not an optional clippy dev-only lint.
Some builds might have deny all warnings and if we add a warning those will break - however broken or not.
Ofcourse from practical PoV it makes sense to add this as a default - just wanted to mention.
If the preference is to maintain the SemVer and agree re: it's breaking - then I would suggest;
For 4.x I would suggest adding
#[cfg_attr(curve25519_dalek_warn_unused), must_use)]
and adopt as default in 5.xThat said -
Should probably check and add them to all similarly used fn's for the sake of consistency at the same release regardless.