From b0cf797dda9c9372161749132c5667a208fa1efb Mon Sep 17 00:00:00 2001 From: Will Lillis Date: Wed, 3 Jul 2024 11:58:22 -0400 Subject: [PATCH] fix: Revert switch to owned data in global maps (#86) --- src/lsp.rs | 28 ++++---- src/test.rs | 179 +++++++++++++++++++++++++--------------------- src/types.rs | 22 +++--- src/x86_parser.rs | 28 ++++---- 4 files changed, 138 insertions(+), 119 deletions(-) diff --git a/src/lsp.rs b/src/lsp.rs index 06309e46..f9b5606c 100644 --- a/src/lsp.rs +++ b/src/lsp.rs @@ -196,7 +196,7 @@ pub fn text_doc_change_to_ts_edit( /// Given a NameTo_SomeItem_ map, returns a `Vec` for the items /// contained within the map pub fn get_completes( - map: &HashMap<(U, String), T>, + map: &HashMap<(U, &str), T>, kind: Option, ) -> Vec { map.iter() @@ -204,7 +204,7 @@ pub fn get_completes( let value = format!("{}", item_info); CompletionItem { - label: name.clone(), + label: (*name).to_string(), kind, documentation: Some(Documentation::MarkupContent(MarkupContent { kind: MarkupKind::Markdown, @@ -219,9 +219,9 @@ pub fn get_completes( pub fn get_hover_resp( word: &str, file_word: &str, - instruction_map: &HashMap<(Arch, String), T>, - register_map: &HashMap<(Arch, String), U>, - directive_map: &HashMap<(Assembler, String), V>, + instruction_map: &HashMap<(Arch, &str), T>, + register_map: &HashMap<(Arch, &str), U>, + directive_map: &HashMap<(Assembler, &str), V>, include_dirs: &[PathBuf], ) -> Option { let instr_lookup = lookup_hover_resp_by_arch(word, instruction_map); @@ -254,7 +254,7 @@ pub fn get_hover_resp( fn lookup_hover_resp_by_arch( word: &str, - map: &HashMap<(Arch, String), T>, + map: &HashMap<(Arch, &str), T>, ) -> Option { // switch over to vec? let (x86_res, x86_64_res, z80_res) = search_for_hoverable_by_arch(word, map); @@ -291,7 +291,7 @@ fn lookup_hover_resp_by_arch( fn lookup_hover_resp_by_assembler( word: &str, - map: &HashMap<(Assembler, String), T>, + map: &HashMap<(Assembler, &str), T>, ) -> Option { let (gas_res, go_res) = search_for_hoverable_by_assembler(word, map); @@ -901,21 +901,21 @@ pub fn get_ref_resp( // For now, using 'a for both isn't strictly necessary, but fits our use case fn search_for_hoverable_by_arch<'a, T: Hoverable>( word: &'a str, - map: &'a HashMap<(Arch, String), T>, + map: &'a HashMap<(Arch, &str), T>, ) -> (Option<&'a T>, Option<&'a T>, Option<&'a T>) { - let x86_res = map.get(&(Arch::X86, word.to_string())); - let x86_64_res = map.get(&(Arch::X86_64, word.to_string())); - let z80_res = map.get(&(Arch::Z80, word.to_string())); + let x86_res = map.get(&(Arch::X86, word)); + let x86_64_res = map.get(&(Arch::X86_64, word)); + let z80_res = map.get(&(Arch::Z80, word)); (x86_res, x86_64_res, z80_res) } fn search_for_hoverable_by_assembler<'a, T: Hoverable>( word: &'a str, - map: &'a HashMap<(Assembler, String), T>, + map: &'a HashMap<(Assembler, &str), T>, ) -> (Option<&'a T>, Option<&'a T>) { - let gas_res = map.get(&(Assembler::Gas, word.to_string())); - let go_res = map.get(&(Assembler::Go, word.to_string())); + let gas_res = map.get(&(Assembler::Gas, word)); + let go_res = map.get(&(Assembler::Go, word)); (gas_res, go_res) } diff --git a/src/test.rs b/src/test.rs index 0c3aefa9..5b39ff5d 100644 --- a/src/test.rs +++ b/src/test.rs @@ -1,7 +1,5 @@ #[cfg(test)] mod tests { - use std::sync::{Mutex, Once, OnceLock}; - use anyhow::Result; use lsp_textdocument::FullTextDocument; use lsp_types::{ @@ -15,22 +13,47 @@ mod tests { get_comp_resp, get_completes, get_hover_resp, get_word_from_pos_params, instr_filter_targets, populate_directives, populate_name_to_directive_map, populate_name_to_instruction_map, populate_name_to_register_map, populate_registers, - x86_parser::get_cache_dir, Arch, Assembler, Assemblers, Instruction, InstructionSets, - NameToDirectiveMap, NameToInstructionMap, NameToRegisterMap, TargetConfig, + x86_parser::get_cache_dir, Arch, Assembler, Assemblers, Directive, Instruction, + InstructionSets, NameToDirectiveMap, NameToInstructionMap, NameToRegisterMap, Register, + TargetConfig, }; - // TODO: Add include_dirs #[derive(Debug)] - struct GlobalVars { - names_to_instructions: NameToInstructionMap, - names_to_registers: NameToRegisterMap, - names_to_directives: NameToDirectiveMap, + struct GlobalInfo { + x86_instructions: Vec, + x86_64_instructions: Vec, + x86_registers: Vec, + x86_64_registers: Vec, + z80_instructions: Vec, + z80_registers: Vec, + gas_directives: Vec, + } + + #[derive(Debug)] + struct GlobalVars<'a> { + names_to_instructions: NameToInstructionMap<'a>, + names_to_registers: NameToRegisterMap<'a>, + names_to_directives: NameToDirectiveMap<'a>, instr_completion_items: Vec, reg_completion_items: Vec, directive_completion_items: Vec, } - impl GlobalVars { + impl GlobalInfo { + fn new() -> Self { + Self { + x86_instructions: Vec::new(), + x86_64_instructions: Vec::new(), + x86_registers: Vec::new(), + x86_64_registers: Vec::new(), + z80_instructions: Vec::new(), + z80_registers: Vec::new(), + gas_directives: Vec::new(), + } + } + } + + impl GlobalVars<'_> { fn new() -> Self { Self { names_to_instructions: NameToInstructionMap::new(), @@ -45,12 +68,10 @@ mod tests { // TODO: Look into a way to cleanly handle multiple GLOBALS, each representing the server // setup from a different configuration - static SETUP: Once = Once::new(); - static GLOBALS: OnceLock> = OnceLock::new(); + fn init_global_info(config: Option) -> Result { + let mut info = GlobalInfo::new(); - fn init_test_store() -> Result { - let mut store = GlobalVars::new(); - let target_config = TargetConfig { + let target_config = config.unwrap_or(TargetConfig { version: "0.1".to_string(), assemblers: Assemblers { gas: true, @@ -62,15 +83,9 @@ mod tests { x86_64: true, z80: true, }, - }; - - let mut x86_cache_path = get_cache_dir().unwrap(); - x86_cache_path.push("x86_instr_docs.html"); - if x86_cache_path.is_file() { - std::fs::remove_file(&x86_cache_path).unwrap(); - } + }); - let x86_instructions: Vec = { + info.x86_instructions = { let xml_conts_x86 = include_str!("../opcodes/x86.xml"); crate::populate_instructions(xml_conts_x86)? .into_iter() @@ -79,13 +94,7 @@ mod tests { .collect() }; - populate_name_to_instruction_map( - Arch::X86, - &x86_instructions, - &mut store.names_to_instructions, - ); - - let x86_64_instructions: Vec = { + info.x86_64_instructions = { let xml_conts_x86_64 = include_str!("../opcodes/x86_64.xml"); crate::populate_instructions(xml_conts_x86_64)? .into_iter() @@ -94,13 +103,7 @@ mod tests { .collect() }; - populate_name_to_instruction_map( - Arch::X86_64, - &x86_64_instructions, - &mut store.names_to_instructions, - ); - - let z80_instructions: Vec = { + info.z80_instructions = { let xml_conts_z80 = include_str!("../opcodes/z80.xml"); crate::populate_instructions(xml_conts_z80)? .into_iter() @@ -109,53 +112,86 @@ mod tests { .collect() }; - populate_name_to_instruction_map( - Arch::Z80, - &z80_instructions, - &mut store.names_to_instructions, - ); - - let x86_registers = { + info.x86_registers = { let xml_conts_regs_x86 = include_str!("../registers/x86.xml"); populate_registers(xml_conts_regs_x86)? .into_iter() .collect() }; - populate_name_to_register_map(Arch::X86, &x86_registers, &mut store.names_to_registers); - - let x86_64_registers = { + info.x86_64_registers = { let xml_conts_regs_x86_64 = include_str!("../registers/x86_64.xml"); populate_registers(xml_conts_regs_x86_64)? .into_iter() .collect() }; - populate_name_to_register_map( - Arch::X86_64, - &x86_64_registers, - &mut store.names_to_registers, - ); - - let z80_registers = { + info.z80_registers = { let xml_conts_regs_z80 = include_str!("../registers/z80.xml"); populate_registers(xml_conts_regs_z80)? .into_iter() .collect() }; - populate_name_to_register_map(Arch::Z80, &z80_registers, &mut store.names_to_registers); - - let gas_directives = { + info.gas_directives = { let xml_conts_gas = include_str!("../directives/gas_directives.xml"); populate_directives(xml_conts_gas)?.into_iter().collect() }; + return Ok(info); + } + + fn init_test_store<'a>(info: &'a GlobalInfo) -> Result> { + let mut store = GlobalVars::new(); + + let mut x86_cache_path = get_cache_dir().unwrap(); + x86_cache_path.push("x86_instr_docs.html"); + if x86_cache_path.is_file() { + std::fs::remove_file(&x86_cache_path).unwrap(); + } + + populate_name_to_instruction_map( + Arch::X86, + &info.x86_instructions, + &mut store.names_to_instructions, + ); + + populate_name_to_instruction_map( + Arch::X86_64, + &info.x86_64_instructions, + &mut store.names_to_instructions, + ); + + populate_name_to_instruction_map( + Arch::Z80, + &info.z80_instructions, + &mut store.names_to_instructions, + ); + + populate_name_to_register_map( + Arch::X86, + &info.x86_registers, + &mut store.names_to_registers, + ); + + populate_name_to_register_map( + Arch::X86_64, + &info.x86_64_registers, + &mut store.names_to_registers, + ); + + populate_name_to_register_map( + Arch::Z80, + &info.z80_registers, + &mut store.names_to_registers, + ); + populate_name_to_directive_map( Assembler::Gas, - &gas_directives, + &info.gas_directives, &mut store.names_to_directives, ); + store.instr_completion_items = get_completes( &store.names_to_instructions, Some(CompletionItemKind::OPERATOR), @@ -172,20 +208,9 @@ mod tests { return Ok(store); } - fn prepare_globals() { - SETUP.call_once(|| { - GLOBALS.set(Mutex::new(init_test_store().unwrap())).unwrap(); - }); - } - fn test_hover(source: &str, expected: &str) { - prepare_globals(); - - let globals = GLOBALS - .get() - .expect("global store not initialized") - .lock() - .expect("global store mutex poisoned"); + let info = init_global_info(None).expect("Failed to load info"); + let globals = init_test_store(&info).unwrap(); let source_code = source.replace("", ""); let curr_doc = Some(FullTextDocument::new( @@ -251,13 +276,8 @@ mod tests { trigger_kind: CompletionTriggerKind, trigger_character: Option, ) { - prepare_globals(); - - let globals = GLOBALS - .get() - .expect("global store not initialized") - .lock() - .expect("Another test already failed"); + let info = init_global_info(None).expect("Failed to load info"); + let globals = init_test_store(&info).unwrap(); let source_code = source.replace("", ""); @@ -329,7 +349,6 @@ mod tests { trigger_kind: CompletionTriggerKind, trigger_character: Option, ) { - prepare_globals(); let expected_kind = CompletionItemKind::VARIABLE; test_autocomplete(source, expected_kind, trigger_kind, trigger_character); } @@ -339,7 +358,6 @@ mod tests { trigger_kind: CompletionTriggerKind, trigger_character: Option, ) { - prepare_globals(); let expected_kind = CompletionItemKind::OPERATOR; test_autocomplete(source, expected_kind, trigger_kind, trigger_character); } @@ -349,7 +367,6 @@ mod tests { trigger_kind: CompletionTriggerKind, trigger_character: Option, ) { - prepare_globals(); let expected_kind = CompletionItemKind::OPERATOR; test_autocomplete(source, expected_kind, trigger_kind, trigger_character); } diff --git a/src/types.rs b/src/types.rs index c456b757..c260a8a6 100644 --- a/src/types.rs +++ b/src/types.rs @@ -13,8 +13,8 @@ pub struct Instruction { pub arch: Option, } -impl Hoverable for Instruction {} -impl Completable for Instruction {} +impl Hoverable for &Instruction {} +impl Completable for &Instruction {} impl Default for Instruction { fn default() -> Self { @@ -335,8 +335,8 @@ pub struct Directive { pub assembler: Option, } -impl Hoverable for Directive {} -impl Completable for Directive {} +impl Hoverable for &Directive {} +impl Completable for &Directive {} impl Default for Directive { fn default() -> Self { @@ -465,8 +465,8 @@ pub struct Register { pub url: Option, } -impl Hoverable for Register {} -impl Completable for Register {} +impl Hoverable for &Register {} +impl Completable for &Register {} impl Default for Register { fn default() -> Self { @@ -567,14 +567,16 @@ impl<'own> Register { } // helper structs, types and functions ------------------------------------------------------------ -pub type NameToInstructionMap = HashMap<(Arch, String), Instruction>; +pub type NameToInstructionMap<'instruction> = + HashMap<(Arch, &'instruction str), &'instruction Instruction>; -pub type NameToRegisterMap = HashMap<(Arch, String), Register>; +pub type NameToRegisterMap<'register> = HashMap<(Arch, &'register str), &'register Register>; -pub type NameToDirectiveMap = HashMap<(Assembler, String), Directive>; +pub type NameToDirectiveMap<'directive> = + HashMap<(Assembler, &'directive str), &'directive Directive>; // Define a trait for types we display on Hover Requests so we can avoid some duplicate code -pub trait Hoverable: Display + Clone {} +pub trait Hoverable: Display + Clone + Copy {} // Define a trait for types we display on Completion Requests so we can avoid some duplicate code pub trait Completable: Display {} // Define a trait for the enums we use to distinguish between different Architectures and Assemblers diff --git a/src/x86_parser.rs b/src/x86_parser.rs index 9db65121..5a1b9222 100644 --- a/src/x86_parser.rs +++ b/src/x86_parser.rs @@ -421,23 +421,23 @@ mod tests { } } -pub fn populate_name_to_instruction_map( +pub fn populate_name_to_instruction_map<'instruction>( arch: Arch, - instructions: &Vec, - names_to_instructions: &mut NameToInstructionMap, + instructions: &'instruction Vec, + names_to_instructions: &mut NameToInstructionMap<'instruction>, ) { // Add the true names first for instruction in instructions { for name in &instruction.get_primary_names() { - names_to_instructions.insert((arch, name.to_string()), instruction.clone()); + names_to_instructions.insert((arch, name), instruction); } } // then add alternate form names, being careful not to overwrite existing entries for instruction in instructions { for name in &instruction.get_associated_names() { names_to_instructions - .entry((arch, name.to_string())) - .or_insert_with(|| instruction.clone()); + .entry((arch, name)) + .or_insert_with(|| instruction); } } } @@ -578,14 +578,14 @@ pub fn populate_registers(xml_contents: &str) -> anyhow::Result> { Ok(registers_map.into_values().collect()) } -pub fn populate_name_to_register_map( +pub fn populate_name_to_register_map<'register>( arch: Arch, - registers: &Vec, - names_to_registers: &mut NameToRegisterMap, + registers: &'register Vec, + names_to_registers: &mut NameToRegisterMap<'register>, ) { for register in registers { for name in ®ister.get_associated_names() { - names_to_registers.insert((arch, name.to_string()), register.clone()); + names_to_registers.insert((arch, name), register); } } } @@ -685,14 +685,14 @@ pub fn populate_directives(xml_contents: &str) -> anyhow::Result> Ok(directives_map.into_values().collect()) } -pub fn populate_name_to_directive_map( +pub fn populate_name_to_directive_map<'directive>( assem: Assembler, - directives: &Vec, - names_to_directives: &mut NameToDirectiveMap, + directives: &'directive Vec, + names_to_directives: &mut NameToDirectiveMap<'directive>, ) { for register in directives { for name in ®ister.get_associated_names() { - names_to_directives.insert((assem, name.to_string()), register.clone()); + names_to_directives.insert((assem, name), register); } } }