remove some const arg in ty dep path boilerplate #74404

lcnr · 2020-07-16T17:41:23Z

followup to #74113, together with #74376, this closes #74360.

tesuji · 2020-07-16T17:55:55Z

src/librustc_middle/ty/mod.rs

@@ -1615,12 +1615,33 @@ pub struct WithOptConstParam<T> {

 impl<T> WithOptConstParam<T> {
    /// Creates a new `WithOptConstParam` setting `const_param_did` to `None`.
+    #[inline(always)]
    pub fn unknown(did: T) -> WithOptConstParam<T> {


I think the general guideline is not to inline for generic function.

hm... this function is only instantiated for WithOptConstParam<DefId> and WithOptConstParam<LocalDefId>.

Would be kind of sad if this is a problem :/

Wait, isn't this only for code in the standard library as inline causes that to get included in every CGU? I don't think that policy counts for compiler internals afaik 🤔

Might be misremembering something here though

src/librustc_middle/ty/mod.rs

eddyb · 2020-07-17T14:19:53Z

src/librustc_middle/ty/mod.rs

+    /// Returns `Some((did, param_did))` if `def_id` is a const argument,
+    /// `None` otherwise.
+    #[inline(always)]
+    pub fn try_fetch(did: LocalDefId, tcx: TyCtxt<'_>) -> Option<(LocalDefId, DefId)> {


I think it's clearer if you start with unknown and then use the other method. Bonus: the other case likely takes the unknown anyway, so it would only be created once.

I don't think so. The other method creates a WithOptConstParam, requiring us to write query_name((def.did, def.const_param_did.unwrap())) which seems really noisy.

We might also change the return value of try_upgrade to Option<(LocalDefId, DefId)> but that makes the other use case less clear.

The problem at its core is that we have two different kinds of queries.

split queries with query_name and query_name_of_const_arg where try_fetch is used at the start of query_name

shared queries taking ty::WithOptConstParam, these call try_upgrade at their start and call itself if that returns some.

We may try and split the remaining shared queries at which point try_upgrade won't be used anymore.

bors · 2020-07-17T20:54:15Z

☔ The latest upstream changes (presumably #72983) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb · 2020-07-21T20:39:54Z

src/librustc_middle/ty/mod.rs

+    /// In case `self` is unknown but `self.did` is a const argument, this returns
+    /// a `WithOptConstParam` with the correct `const_param_did`.
+    #[inline(always)]
+    pub fn try_upgrade(self, tcx: TyCtxt<'_>) -> Option<WithOptConstParam<LocalDefId>> {


Hmm, so if we wanted this name to be more explicit, it could be try_upgrade_to_const_arg, right?
Then the other one could be try_lookup_as_const_arg.

Alternatively, try_upgrade_with_const_param and try_lookup_const_param_for.

eddyb · 2020-07-21T21:33:33Z

@bors rollup=never (just in case there's a perf impact, even if small)

eddyb · 2020-07-21T21:33:45Z

@bors r+

bors · 2020-07-21T21:33:46Z

📌 Commit 4bffe8f has been approved by eddyb

bors · 2020-07-22T05:50:01Z

⌛ Testing commit 4bffe8f with merge a02f4b3b2949898ae25f68f01203574f21127307...

bors · 2020-07-22T05:56:09Z

💔 Test failed - checks-actions

lcnr · 2020-07-22T07:05:58Z

looks like a network failure

@bors retry

bors · 2020-07-22T07:06:54Z

⌛ Testing commit 4bffe8f with merge 300a3672c74c560ff7d962d52a2c47ec2b55dd90...

bors · 2020-07-22T07:13:11Z

💔 Test failed - checks-actions

lcnr · 2020-07-22T07:26:39Z

Again
@bors retry

bors · 2020-07-22T07:38:48Z

⌛ Testing commit 4bffe8f with merge e3cea76f3d3b1c5aba3975f5ecebb67c246b89f4...

Manishearth · 2020-07-22T07:44:02Z

@bors retry yield

bors · 2020-07-22T07:52:10Z

⌛ Testing commit 4bffe8f with merge a385482389198bd17c8883c3f37b89185d24515d...

bors · 2020-07-22T08:00:46Z

💔 Test failed - checks-azure

Manishearth · 2020-07-22T15:26:14Z

@bors retry

network problems

Mark-Simulacrum · 2020-07-22T21:50:17Z

@bors treeclosed-

bors · 2020-07-22T21:50:30Z

⌛ Testing commit 4bffe8f with merge 4a86573...

bors · 2020-07-23T00:20:57Z

☀️ Test successful - checks-actions, checks-azure
Approved by: eddyb
Pushing 4a86573 to master...

rust-log-analyzer · 2020-07-25T07:53:36Z

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

##[group]Run exit 1
exit 1
shell: /bin/bash --noprofile --norc -e -o pipefail {0}
##[endgroup]
##[error]Process completed with exit code 1.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

rust-highfive assigned eddyb Jul 16, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 16, 2020

tesuji reviewed Jul 16, 2020

View reviewed changes

eddyb reviewed Jul 17, 2020

View reviewed changes

src/librustc_middle/ty/mod.rs Outdated Show resolved Hide resolved

eddyb reviewed Jul 17, 2020

View reviewed changes

eddyb reviewed Jul 21, 2020

View reviewed changes

lcnr added 2 commits July 21, 2020 23:06

remove some const arg in ty dep path boilerplate

ebc0c15

try_update -> try_upgrade

38e9e62

lcnr force-pushed the ty-dep-path-cleanup-aaaaa branch from 12bcb56 to ddf2c70 Compare July 21, 2020 21:06

fetch -> lookup

4bffe8f

lcnr force-pushed the ty-dep-path-cleanup-aaaaa branch from ddf2c70 to 4bffe8f Compare July 21, 2020 21:12

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 Jul 21, 2020

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

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 Jul 22, 2020

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

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 Jul 22, 2020

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

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 Jul 22, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 23, 2020

bors merged commit 4a86573 into rust-lang:master Jul 23, 2020

lcnr deleted the ty-dep-path-cleanup-aaaaa branch July 23, 2020 06:07

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove some const arg in ty dep path boilerplate #74404

remove some const arg in ty dep path boilerplate #74404

lcnr commented Jul 16, 2020

tesuji Jul 16, 2020 •

edited

Loading

lcnr Jul 16, 2020

lcnr Jul 16, 2020

eddyb Jul 17, 2020

lcnr Jul 17, 2020

lcnr Jul 17, 2020

lcnr Jul 17, 2020

bors commented Jul 17, 2020

eddyb Jul 21, 2020

eddyb commented Jul 21, 2020

eddyb commented Jul 21, 2020

bors commented Jul 21, 2020

bors commented Jul 22, 2020

bors commented Jul 22, 2020

lcnr commented Jul 22, 2020

bors commented Jul 22, 2020

bors commented Jul 22, 2020

lcnr commented Jul 22, 2020

bors commented Jul 22, 2020

Manishearth commented Jul 22, 2020

bors commented Jul 22, 2020

bors commented Jul 22, 2020

Manishearth commented Jul 22, 2020

Mark-Simulacrum commented Jul 22, 2020

bors commented Jul 22, 2020

bors commented Jul 23, 2020

rust-log-analyzer commented Jul 25, 2020

remove some const arg in ty dep path boilerplate #74404

remove some const arg in ty dep path boilerplate #74404

Conversation

lcnr commented Jul 16, 2020

tesuji Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

lcnr Jul 16, 2020

Choose a reason for hiding this comment

lcnr Jul 16, 2020

Choose a reason for hiding this comment

eddyb Jul 17, 2020

Choose a reason for hiding this comment

lcnr Jul 17, 2020

Choose a reason for hiding this comment

lcnr Jul 17, 2020

Choose a reason for hiding this comment

lcnr Jul 17, 2020

Choose a reason for hiding this comment

bors commented Jul 17, 2020

eddyb Jul 21, 2020

Choose a reason for hiding this comment

eddyb commented Jul 21, 2020

eddyb commented Jul 21, 2020

bors commented Jul 21, 2020

bors commented Jul 22, 2020

bors commented Jul 22, 2020

lcnr commented Jul 22, 2020

bors commented Jul 22, 2020

bors commented Jul 22, 2020

lcnr commented Jul 22, 2020

bors commented Jul 22, 2020

Manishearth commented Jul 22, 2020

bors commented Jul 22, 2020

bors commented Jul 22, 2020

Manishearth commented Jul 22, 2020

Mark-Simulacrum commented Jul 22, 2020

bors commented Jul 22, 2020

bors commented Jul 23, 2020

rust-log-analyzer commented Jul 25, 2020

tesuji Jul 16, 2020 •

edited

Loading