Add documentation to has_deref#114505

ouz-a · 2023-08-05T09:05:35Z

Documentation of has_deref needed some polish to be more clear about where it should be used and what's it's purpose.

RalfJung · 2023-08-05T10:41:49Z

compiler/rustc_middle/src/mir/mod.rs

Both of which conditions? And it only does that check in debug-mode, so really this is not checked most of the time! The caller can not rely on this check.

Also there is no phase called "Derefered"? At least look at https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/syntax/enum.MirPhase.html I cannot find one. The AnalysisPhase::PostCleanup says that * must be the first projection, so maybe this should say "when in a Runtime MirPhase"?

My bad, it should been MirPass not MirPhase, https://doc.rust-lang.org/nightly/nightly-rustc/src/rustc_mir_transform/lib.rs.html#454-463

Passes get reordered though. MirPhase is the right concept to refer to here. MirPhase make certain guarantees. For example, the PostCleanup one makes the guarantee we care about here. So that's what it should reference.

compiler/rustc_middle/src/mir/mod.rs

rustbot · 2023-08-06T06:55:22Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

RalfJung · 2023-08-06T11:41:45Z

compiler/rustc_middle/src/mir/mod.rs

MirPass doesn't implement PartialOrd so this doesn't make a lot of sense.

MirPhase does implement PartialOrd, on the other hand.

RalfJung · 2023-08-06T11:42:08Z

compiler/rustc_middle/src/mir/mod.rs

Suggested change

// To make sure this is not accidentally used in wrong mir phase

this comment referred to the debug assertion that is no longer here

RalfJung · 2023-08-06T11:43:14Z

compiler/rustc_middle/src/mir/mod.rs

The comment is wrong or confusing. It says that the function checks that "if there is a deref it is in the first place", but that's not what the function checks.

Simplified the wording I think it's clearer now.

RalfJung · 2023-08-06T12:08:19Z

compiler/rustc_middle/src/mir/mod.rs

Suggested change

/// Returns 'true' if this 'Place's first projection equals to 'Deref' projection else

/// returns false.

///

/// Must only be called, after `MirPass::Derefer`.

/// Returns 'true' if this `Place`'s first projection is `Deref`.

///

/// This is useful because for MIR phases `AnalysisPhase::PostCleanup` and later,

/// `Deref` projections can only occur as the first projection. In that case this method

/// is equivalent to `is_indirect`, but faster.

Well that's certainly way better than mine, thanks for the review

RalfJung

Missed that in my previous suggestions.

With that fix, r=me. Thanks for taking care of this!

RalfJung · 2023-08-06T13:14:22Z

compiler/rustc_middle/src/mir/mod.rs

Suggested change

/// Returns 'true' if this `Place`'s first projection is `Deref`.

/// Returns `true` if this `Place`'s first projection is `Deref`.

RalfJung · 2023-08-06T13:14:34Z

compiler/rustc_middle/src/mir/mod.rs

Suggested change

/// Returns 'true' if this `Place`'s first projection is `Deref`.

/// Returns `true` if this `Place`'s first projection is `Deref`.

ouz-a · 2023-08-06T14:29:54Z

@bors r=RalfJung

edit: that doesn't seem like worked ? 🤔

bors · 2023-08-06T14:29:56Z

@ouz-a: 🔑 Insufficient privileges: Not in reviewers

RalfJung · 2023-08-06T14:35:19Z

@bors r+ rollup

bors · 2023-08-06T14:35:21Z

📌 Commit 6df5462 has been approved by RalfJung

It is now in the queue for this repository.

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#114466 (Add Allocation to SMIR) - rust-lang#114505 (Add documentation to has_deref) - rust-lang#114519 (use offset_of! to calculate dirent64 field offsets) - rust-lang#114537 (Migrate GUI colors test to original CSS color format) - rust-lang#114539 (linkchecker: Remove unneeded FIXME about intra-doc links) Failed merges: - rust-lang#114485 (Add trait decls to SMIR) r? `@ghost` `@rustbot` modify labels: rollup

Add documentation to has_deref Documentation of `has_deref` needed some polish to be more clear about where it should be used and what's it's purpose. cc rust-lang#114401 r? `@RalfJung`

rustbot assigned RalfJung Aug 5, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 5, 2023

ouz-a mentioned this pull request Aug 5, 2023

Simplify some code that depend on Deref #97077

Merged

RalfJung reviewed Aug 5, 2023

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 5, 2023

View reviewed changes

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 6, 2023

View reviewed changes

ouz-a force-pushed the cleanup_mir branch from 8d6a8a8 to 11f817c Compare August 6, 2023 12:13

RalfJung reviewed Aug 6, 2023

View reviewed changes

cleanup misinformation regarding has_deref

6df5462

ouz-a force-pushed the cleanup_mir branch from c66b036 to 6df5462 Compare August 6, 2023 14:29

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2023

matthiaskrgr mentioned this pull request Aug 6, 2023

Rollup of 5 pull requests #114553

Merged

bors merged commit 13de583 into rust-lang:master Aug 6, 2023

rustbot added this to the 1.73.0 milestone Aug 6, 2023

ouz-a deleted the cleanup_mir branch August 10, 2023 10:03

ouz-a mentioned this pull request Aug 15, 2023

Cleanup: Place::is_indirect vs Place::has_deref #114401

Closed

-    /// Returns 'true' if this 'Place's first projection equals to 'Deref' projection else
-    /// returns false.
-    ///
-    /// Must only be called, after `MirPass::Derefer`.
+    /// Returns 'true' if this `Place`'s first projection is `Deref`.
+    ///
+    /// This is useful because for MIR phases `AnalysisPhase::PostCleanup` and later,
+    /// `Deref` projections can only occur as the first projection. In that case this method
+    /// is equivalent to `is_indirect`, but faster.

	/// Returns 'true' if this `Place`'s first projection is `Deref`.
	/// Returns `true` if this `Place`'s first projection is `Deref`.

Uh oh!

Conversation

ouz-a commented Aug 5, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Aug 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rustbot commented Aug 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ouz-a commented Aug 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Aug 6, 2023

Uh oh!

RalfJung commented Aug 6, 2023

Uh oh!

bors commented Aug 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RalfJung Aug 5, 2023 •

edited

Loading

ouz-a commented Aug 6, 2023 •

edited

Loading