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.

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

Don't expose bit flag operations quite so much to the user. #161

Merged
merged 1 commit into from
Nov 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions ykpack/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ license = "Apache-2.0 OR MIT"

[dependencies]
bincode = "1.3.1"
bitflags = "1.2"
fallible-iterator = "0.2.0"
serde = { version = "1.0.115", features = ["derive"] }
6 changes: 3 additions & 3 deletions ykpack/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ pub const SIR_SECTION_PREFIX: &str = ".yksir_";

#[cfg(test)]
mod tests {
use super::{BasicBlock, Body, Decoder, Encoder, Pack, Statement, Terminator};
use super::{BasicBlock, Body, BodyFlags, Decoder, Encoder, Pack, Statement, Terminator};
use fallible_iterator::{self, FallibleIterator};
use std::io::{Cursor, Seek, SeekFrom};

Expand Down Expand Up @@ -66,7 +66,7 @@ mod tests {
let sir1 = Pack::Body(Body {
symbol_name: String::from("symbol1"),
blocks: blocks1,
flags: 0,
flags: BodyFlags::empty(),
local_decls: Vec::new(),
num_args: 0,
});
Expand All @@ -82,7 +82,7 @@ mod tests {
let sir2 = Pack::Body(Body {
symbol_name: String::from("symbol2"),
blocks: blocks2,
flags: 0,
flags: BodyFlags::empty(),
local_decls: Vec::new(),
num_args: 0,
});
Expand Down
24 changes: 14 additions & 10 deletions ykpack/src/types.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Types for the Yorick intermediate language.

use bitflags::bitflags;
use serde::{Deserialize, Serialize};
use std::{
convert::TryFrom,
Expand Down Expand Up @@ -343,14 +344,17 @@ impl Display for Local {
}
}

/// Bits in the `flags` bitfield in `Body`.
pub mod bodyflags {
/// This function is annotated #[do_not_trace].
pub const DO_NOT_TRACE: u8 = 1;
/// This function is annotated #[interp_step].
pub const INTERP_STEP: u8 = 1 << 1;
/// This function is yktrace::trace_debug.
pub const TRACE_DEBUG: u8 = 1 << 2;
bitflags! {
/// Bits in the `flags` bitfield in `Body`.
#[derive(Serialize, Deserialize)]
pub struct BodyFlags: u8 {
/// This function is annotated #[do_not_trace].
const DO_NOT_TRACE = 0b00000001;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use the 1<<X idiom? It's easier to read and less error prone.

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 used to use 1<<X but I've made too many mistakes with it to be fond of it any more. To be honest, I didn't think deeply about this: I think all of the examples you'll find using bitflags use the 0b notation, which suggests that's what people have gravitated towards.

/// This function is annotated #[interp_step].
const INTERP_STEP = 0b00000010;
/// This function is yktrace::trace_debug.
const TRACE_DEBUG = 0b00000100;
}
}

/// The definition of a local variable, including its type.
Expand All @@ -374,15 +378,15 @@ impl Display for LocalDecl {
pub struct Body {
pub symbol_name: String,
pub blocks: Vec<BasicBlock>,
pub flags: u8,
pub flags: BodyFlags,
pub local_decls: Vec<LocalDecl>,
pub num_args: usize,
}

impl Display for Body {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
writeln!(f, "symbol: {}", self.symbol_name)?;
writeln!(f, " flags: {}", self.flags)?;
writeln!(f, " flags: {:?}", self.flags)?;
writeln!(f, " num_args: {}", self.num_args)?;

writeln!(f, " local_decls:")?;
Expand Down
4 changes: 2 additions & 2 deletions yktrace/src/sir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use std::{
iter::Iterator,
path::Path
};
use ykpack::{self, bodyflags, Body, CguHash, Decoder, Pack, Ty};
use ykpack::{self, Body, BodyFlags, CguHash, Decoder, Pack, Ty};

/// The serialised IR loaded in from disk. One of these structures is generated in the above
/// `lazy_static` and is shared immutably for all threads.
Expand Down Expand Up @@ -144,7 +144,7 @@ pub fn sir_trace_str(sir: &Sir, trace: &dyn SirTrace, trimmed: bool, show_blocks

let body = sir.bodies.get(&loc.symbol_name);
if let Some(body) = body {
if body.flags & bodyflags::INTERP_STEP != 0 {
if body.flags.contains(BodyFlags::INTERP_STEP) {
write!(res_r, "INTERP_STEP ").unwrap();
}
}
Expand Down
12 changes: 6 additions & 6 deletions yktrace/src/tir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ use std::{
fmt::{self, Display, Write}
};
pub use ykpack::{
BinOp, CallOperand, Constant, ConstantInt, IPlace, Local, LocalDecl, LocalIndex, Ptr,
SignedInt, Statement, Terminator, UnsignedInt
BinOp, BodyFlags, CallOperand, Constant, ConstantInt, IPlace, Local, LocalDecl, LocalIndex,
Ptr, SignedInt, Statement, Terminator, UnsignedInt
};

const RETURN_LOCAL: Local = Local(0);
Expand Down Expand Up @@ -101,7 +101,7 @@ impl<'a> TirTrace<'a> {
// Ignore yktrace::trace_debug.
// We don't use the 'ignore' machinery below, as that would require the TraceDebugCall
// terminator to contain the symbol name, which would be wasteful.
if body.flags & ykpack::bodyflags::TRACE_DEBUG != 0 {
if body.flags.contains(BodyFlags::TRACE_DEBUG) {
continue;
}

Expand Down Expand Up @@ -211,7 +211,7 @@ impl<'a> TirTrace<'a> {
{
if let Some(callee_sym) = op.symbol() {
if let Some(callee_body) = sir.bodies.get(callee_sym) {
if callee_body.flags & ykpack::bodyflags::INTERP_STEP != 0 {
if callee_body.flags.contains(BodyFlags::INTERP_STEP) {
if in_interp_step {
panic!("recursion into interp_step detected");
}
Expand Down Expand Up @@ -255,7 +255,7 @@ impl<'a> TirTrace<'a> {

// If the function has been annotated with do_not_trace, turn it into a
// call.
if callbody.flags & ykpack::bodyflags::DO_NOT_TRACE != 0 {
if callbody.flags.contains(BodyFlags::DO_NOT_TRACE) {
ignore = Some(callee_sym.to_string());
term_stmts.push(Statement::Call(op.clone(), newargs, Some(ret_val)))
} else {
Expand Down Expand Up @@ -288,7 +288,7 @@ impl<'a> TirTrace<'a> {
}
}
Terminator::Return => {
if body.flags & ykpack::bodyflags::INTERP_STEP != 0 {
if body.flags.contains(BodyFlags::INTERP_STEP) {
debug_assert!(in_interp_step);
in_interp_step = false;
continue;
Expand Down