Re: [PATCH v4] rust: sync: add #[must_use] to Lock::try_lock

[Boqun Feng <boqun.feng@gmail.com>]

On Wed, Dec 11, 2024 at 09:05:32AM -0800, jos wrote:
> > Can't we use the same warning message as Guard?
> 
> > #[must_use = "the lock unlocks immediately when the guard is unused"]
> 
> That is what I originally had in my first patch and how I understood
> the annotation should be phrased, but @Boqun Feng maybe you can chime
> in with your input?
> 

Hmm... I think it was a mis-communication. In the v1, you said "..but it
currently does not issue a warning if the return value is unused. This
could result in the lock being unlocked immediately.." I thought "This"
means "not having `#[must_use]`", but you probably meant "not using the
value"? My comment is more to that commit log instead of the warning
message in the `#[must_use = ...]` block.

Anyway, I think the warning message in your v1 works for me. Again, to
be clear, my concern was on the commit log then. Hope it clarifies
things.

Regards,
Boqun

> >> +    /// Note that `T` is `#[must_use]` but `Option<T>` is not.
> >
> > Should this be a comment, i.e. `//`, rather than part of the documentation?
> 
> Agreed, after reading more documentation conventions
> (https://github.com/rust-lang/rfcs/blob/master/text/1574-more-api-documentation-conventions.md),
> this should not be part of the rustdocs.
> 
> > I would also reword to explain the "what", not just "why", e.g. something like:
> >
> >    // `Option<T>` is not `#[must_use]` even if `T` is, thus the
> > attribute is needed here.
> 
> Thank you for the insight, I can see how there is missing clarity in
> the current comment, will update in the next patch.
> 
> Thanks,
> Jason
> 
> 
> On Wed, Dec 11, 2024 at 1:45 AM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > On Wed, Dec 11, 2024 at 3:05 AM Jason Devers <dev.json2@gmail.com> wrote:
> > >
> > > +    /// Note that `T` is `#[must_use]` but `Option<T>` is not.
> >
> > Should this be a comment, i.e. `//`, rather than part of the documentation?
> >
> > I would also reword to explain the "what", not just "why", e.g. something like:
> >
> >     // `Option<T>` is not `#[must_use]` even if `T` is, thus the
> > attribute is needed here.
> >
> > Cheers,
> > Miguel