A new proposal has been announced: Uplift the
invalid_atomic_ordering lint from clippy to rustc. I have no clue if this will be discussed in a meeting or not but you probably can find better things to do with that time in any case.
Something seems to have happened to the MCP stream robot, so I am fulfilling its role manually. Beep boop.
I'm in favor /o/. A. It seems harmless and provides better IDE checking vs a runtime panic, and B. This bit me the other day and would have been nice to have :)
I believe lint additions should be lang MCP?
In https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Process.20for.20moving.20lints.20from.20Clippy.20to.20Rustc/near/214891748 I was told to make it in compiler-team. https://github.com/rust-lang/compiler-team/issues/346 is also precedent of this being a compiler-team thing in the past.
(This would be a good thing to have a meta discussion about, I think -- personally I don't think lang needs to approve lints about things that don't work. Maybe if there are proposals to lint on things that _do_ work it makes some sense to loop in lang because it's a weak language feature deprecation. But especially for "that's the wrong way to use a library function" I don't think lang needs to be involved, though maybe libs would want to know.)
Yeah, I think there's definitely some meta discussion -- my impression was that lang wanted to approve all lints. It seems fine for some of that to be ceded to T-compiler/libs.
We have typically said we want to approve lints, yes.
But I don't know that we need an MCP
I might be inclined to just have the PR
we should document it, regardless
:+1: for just doing an FCP in the PR for lang team approval.
For lack of an obvious place to put it, I filed it as a meeting proposal in https://github.com/rust-lang/lang-team/issues/72
I just went and did it https://github.com/rust-lang/rust/pull/79654.
One super nitpickey issue I thought of is:
_.store(_, AcqRel) the lint's help message tells you which orderings would be valid for a
store, for example:
error: atomic stores cannot have `Acquire` or `AcqRel` ordering --> $DIR/lint-invalid-atomic-ordering-bool.rs:25:20 | LL | x.store(false, Ordering::AcqRel); | ^^^^^^^^^^^^^^^^ | = help: consider using ordering modes `Release`, `SeqCst` or `Relaxed`
If the code really did want
AcqRel semantics on the store, the correct answer would be
That said, I feel like anybody who knows that they really want
AcqRel for that store is likely to know this already, and we suggest
SeqCst which is strictly stronger on the store than the
AcqRel swap would be anyway.
A similar issue also exists for a
_.load(Release) since in the unlikely event where they really want that, the right answer would be to do both
_.load(Relaxed) and a
fence(Release) (and in practice, IME
_.load(Release) is usually
_.load(<uh, was it Acquire or Release that I use for loads?>))
Anyway, I think this is fine, since the help message mentions sufficiently strong options for both those cases, and code that wants those things is pretty weird anyway and would probably only be written by people who know this already (hopefully). I figured I'd mention it in case someone strongly disagrees, though.
Doesn't this ordering literally not make sense though? Like it makes no sense to give this to LLVM as it doesn't mean anything
If the code really did want AcqRel semantics on the store,
That is, I don't think there is such a thing as AcqRel semantics on a store
@Mario Carneiro right, it would need to be an RMW.
you see this in implementation of some concurrent algorithms (petersons algorithm for example requires it when locking, i believe)
If it's a RMW or something where the ordering makes sense, I would expect the error/lint warning against it not to exist
Yeah, the lint doesn't fire on anything that wouldn't cause an unconditional panic.
Anyway, it seems what I said was just very confusing, sorry.