Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix outdated comment of fn_can_unwind #113213

Merged
merged 1 commit into from
Jul 4, 2023
Merged

Fix outdated comment of fn_can_unwind #113213

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 5 additions & 11 deletions compiler/rustc_middle/src/ty/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1101,12 +1101,11 @@ where
///
/// This takes two primary parameters:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is now also wrong? 😀 should this mention fn_can_unwind as that was probably used to replace codegen_fn_attr_flags?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I am not sure which line you are talking about.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woops, meant fn_def_id.

/// This takes two primary parameters:

Either say "this takes one primary parameter"/remove this line entirely, or alternatively, also mention fn_def_id and mention "This is only applicable for Rust-defined functions, and generally isn't needed except for small optimizations where we try to say a function which otherwise might look like it could unwind doesn't actually unwind (such as for intrinsics and such)."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did already mentioned fn_def_id though?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have still been a bit too tired? 😅 yikes

sorry, you're absolutely right

///
/// * `codegen_fn_attr_flags` - these are flags calculated as part of the
/// codegen attrs for a defined function. For function pointers this set of
/// flags is the empty set. This is only applicable for Rust-defined
/// functions, and generally isn't needed except for small optimizations where
/// we try to say a function which otherwise might look like it could unwind
/// doesn't actually unwind (such as for intrinsics and such).
/// * `fn_def_id` - the `DefId` of the function. If this is provided then we can
/// determine more precisely if the function can unwind. If this is not provided
/// then we will only infer whether the function can unwind or not based on the
/// ABI of the function. For example, a function marked with `#[rustc_nounwind]`
/// is known to not unwind even if it's using Rust ABI.
///
/// * `abi` - this is the ABI that the function is defined with. This is the
/// primary factor for determining whether a function can unwind or not.
Expand Down Expand Up @@ -1138,11 +1137,6 @@ where
/// aborts the process.
/// * This affects whether functions have the LLVM `nounwind` attribute, which
/// affects various optimizations and codegen.
///
/// FIXME: this is actually buggy with respect to Rust functions. Rust functions
/// compiled with `-Cpanic=unwind` and referenced from another crate compiled
/// with `-Cpanic=abort` will look like they can't unwind when in fact they
/// might (from a foreign exception or similar).
#[inline]
#[tracing::instrument(level = "debug", skip(tcx))]
pub fn fn_can_unwind(tcx: TyCtxt<'_>, fn_def_id: Option<DefId>, abi: SpecAbi) -> bool {
Expand Down
Loading