-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add const generics to the HIR #58503
Conversation
Co-Authored-By: Gabriel Smith <yodaldevoid@users.noreply.github.com>
Co-Authored-By: Gabriel Smith <yodaldevoid@users.noreply.github.com>
Co-Authored-By: Gabriel Smith <yodaldevoid@users.noreply.github.com>
Co-Authored-By: Gabriel Smith <yodaldevoid@users.noreply.github.com>
Co-Authored-By: Gabriel Smith <yodaldevoid@users.noreply.github.com>
Co-Authored-By: Gabriel Smith <yodaldevoid@users.noreply.github.com>
Co-Authored-By: Gabriel Smith <yodaldevoid@users.noreply.github.com>
Co-Authored-By: Gabriel Smith <yodaldevoid@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
14dfb4d
to
0b4cd07
Compare
0b4cd07
to
18ce997
Compare
#[derive(Clone, RustcEncodable, RustcDecodable, Debug)] | ||
pub struct ConstArg { | ||
pub value: AnonConst, | ||
pub span: Span, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This span is identical to the span in body of AnonConst
, is it viable to use it instead?
Other AnonConst
s don't create extra spans during lowering as far as can I see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The whole ConstArg
could be refactored away in that case.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, I see GenericArg::span
wants to work in isolation without any contexts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we went through the same thought process when this was implemented. If you can see a way to refactor this, I'd be all ears.
LGTM, r=me modulo removing abort / adding lint test (#58503 (comment)). I'll keep @eddyb assigned in case he wants to look. |
@bors r=petrochenkov p=1 (Increasing priority because this unblocks other work.) |
📌 Commit 727e204 has been approved by |
⌛ Testing commit 727e204 with merge 3cdf81e0294cfe7414af2cae07bd3e82d1975058... |
@bors retry |
@@ -1684,6 +1707,11 @@ impl<'a, 'b, 'tcx> IndexBuilder<'a, 'b, 'tcx> { | |||
let encode_info = IsolatedEncoder::encode_info_for_ty_param; | |||
self.record(def_id, encode_info, (def_id, has_default)); | |||
} | |||
hir::GenericParamKind::Const { .. } => { | |||
let def_id = self.tcx.hir().local_def_id(param.id); | |||
let encode_info = IsolatedEncoder::encode_info_for_const_param; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe open an issue about unifying the param encoding in metadata, the same way GenericParamDef
has common fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #58581.
Add const generics to the HIR Split out from #53645. cc @yodaldevoid r? @eddyb
☀️ Test successful - checks-travis, status-appveyor |
…tor, r=eddyb Refactor generic parameter encoder functions Addresses rust-lang#58503 (comment). r? @eddyb
…tor, r=eddyb Refactor generic parameter encoder functions Addresses rust-lang#58503 (comment). r? @eddyb
Split out from #53645. This work is a collaborative effort with @yodaldevoid.
r? @eddyb