diff --git a/cpp2rust/converter/converter.cpp b/cpp2rust/converter/converter.cpp index 37aea766..6ae2bd89 100644 --- a/cpp2rust/converter/converter.cpp +++ b/cpp2rust/converter/converter.cpp @@ -2757,6 +2757,8 @@ void Converter::ConvertCXXConstructExprArgs(clang::CXXConstructExpr *expr) { } bool Converter::VisitCXXConstructExpr(clang::CXXConstructExpr *expr) { + PushSuppressIteratorClone push(*this, expr); + if (auto str = GetMappedAsString(expr, expr->getArgs(), expr->getNumArgs()); !str.empty()) { StrCat(str); @@ -2767,8 +2769,10 @@ bool Converter::VisitCXXConstructExpr(clang::CXXConstructExpr *expr) { if (ctor->isCopyOrMoveConstructor() || (ctor->isConvertingConstructor(false) && ctor->getNumParams() == 1 && ctor->getParamDecl(0)->getType()->isRValueReferenceType())) { + // Take supress before recursing into the child. + bool suppress = PushSuppressIteratorClone::take(*this); Convert(expr->getArg(0)); - if (ctor->isCopyConstructor() && !IsRedundantCopyInConversion(ctx_, expr)) { + if (ctor->isCopyConstructor() && !suppress) { StrCat(".clone()"); } return false; diff --git a/cpp2rust/converter/converter.h b/cpp2rust/converter/converter.h index a98bbf40..fa81c6a2 100644 --- a/cpp2rust/converter/converter.h +++ b/cpp2rust/converter/converter.h @@ -13,6 +13,7 @@ #include #include #include +#include #include #include "converter/lex.h" @@ -529,6 +530,7 @@ class Converter : public clang::RecursiveASTVisitor { bool in_function_formals_ = false; bool in_const_initializer_ = false; std::optional autoref_mut_; + bool suppress_iterator_clone_ = false; struct PushExplicitAutoref { Converter &c; @@ -540,6 +542,41 @@ class Converter : public clang::RecursiveASTVisitor { ~PushExplicitAutoref() { c.autoref_mut_ = prev; } }; + struct PushSuppressIteratorClone { + Converter &c; + bool prev; + PushSuppressIteratorClone(Converter &c, clang::CXXConstructExpr *expr) + : c(c), prev(c.suppress_iterator_clone_) { + auto *ctor = expr->getConstructor(); + if (!ctor->isCopyOrMoveConstructor() && + ctor->isConvertingConstructor(/*AllowExplicit=*/false) && + ctor->getNumParams() == 1 && IsIteratorType(expr->getType())) { + c.suppress_iterator_clone_ = true; + } + } + ~PushSuppressIteratorClone() { c.suppress_iterator_clone_ = prev; } + PushSuppressIteratorClone(const PushSuppressIteratorClone &) = delete; + PushSuppressIteratorClone & + operator=(const PushSuppressIteratorClone &) = delete; + + static bool take(Converter &c) { + return std::exchange(c.suppress_iterator_clone_, false); + } + + private: + static bool IsIteratorType(clang::QualType qt) { + if (auto *record = qt->getAsCXXRecordDecl()) { + for (auto *d : record->decls()) { + if (auto *tnd = llvm::dyn_cast(d)) { + if (tnd->getName() == "iterator_category") + return true; + } + } + } + return false; + } + }; + struct PushConstInitializer { Converter &c; bool prev; diff --git a/cpp2rust/converter/converter_lib.cpp b/cpp2rust/converter/converter_lib.cpp index 040d7d1b..4f60cf64 100644 --- a/cpp2rust/converter/converter_lib.cpp +++ b/cpp2rust/converter/converter_lib.cpp @@ -644,16 +644,6 @@ std::string GetClassName(clang::QualType type) { return {}; } -bool IsRedundantCopyInConversion(clang::ASTContext &ctx, - const clang::CXXConstructExpr *expr) { - auto parents = ctx.getParentMapContext().getParents(*expr); - if (parents.empty()) { - return false; - } - auto *parent = parents[0].get(); - return parent && parent->getConstructor()->isConvertingConstructor(false); -} - bool IsVaListType(clang::QualType type) { for (auto t = type; !t.isNull();) { if (auto *adjusted = t->getAs()) { diff --git a/cpp2rust/converter/converter_lib.h b/cpp2rust/converter/converter_lib.h index d9c97622..aa5d0c2b 100644 --- a/cpp2rust/converter/converter_lib.h +++ b/cpp2rust/converter/converter_lib.h @@ -145,9 +145,6 @@ GetForRangeIteratorType(const clang::CXXForRangeStmt *for_range); std::string GetClassName(clang::QualType type); -bool IsRedundantCopyInConversion(clang::ASTContext &ctx, - const clang::CXXConstructExpr *expr); - bool IsVaListType(clang::QualType type); bool IsBuiltinVaStart(const clang::CallExpr *expr); diff --git a/cpp2rust/converter/models/converter_refcount.cpp b/cpp2rust/converter/models/converter_refcount.cpp index c2bd9e18..c194e796 100644 --- a/cpp2rust/converter/models/converter_refcount.cpp +++ b/cpp2rust/converter/models/converter_refcount.cpp @@ -1541,6 +1541,7 @@ std::string ConverterRefCount::ConvertStream(clang::Expr *expr) { bool ConverterRefCount::VisitCXXConstructExpr(clang::CXXConstructExpr *expr) { PushConversionKind push(*this, ConversionKind::Unboxed); + PushSuppressIteratorClone push_suppress(*this, expr); if (auto str = GetMappedAsString(expr, expr->getArgs(), expr->getNumArgs()); !str.empty()) { @@ -1564,7 +1565,9 @@ bool ConverterRefCount::VisitCXXConstructExpr(clang::CXXConstructExpr *expr) { } if (ctor->isCopyConstructor()) { - StrCat(ConvertRValue(expr->getArg(0))); + StrCat(PushSuppressIteratorClone::take(*this) + ? ConvertRValue(expr->getArg(0)) + : ConvertFreshRValue(expr->getArg(0))); return false; } diff --git a/rules/map/ir_refcount.json b/rules/map/ir_refcount.json index 799bd5ca..5730c1fb 100644 --- a/rules/map/ir_refcount.json +++ b/rules/map/ir_refcount.json @@ -462,14 +462,27 @@ "f19": { "body": [ { - "placeholder": { - "arg": 0, - "access": "read" + "method_call": { + "receiver": [ + { + "placeholder": { + "arg": 0, + "access": "read" + } + } + ], + "body": [ + { + "text": ".clone()" + } + ] } } ], "generics": { - "T1": [], + "T1": [ + "Clone" + ], "T2": [] }, "params": { diff --git a/rules/map/ir_unsafe.json b/rules/map/ir_unsafe.json index a876e246..a8f9aeaf 100644 --- a/rules/map/ir_unsafe.json +++ b/rules/map/ir_unsafe.json @@ -466,14 +466,27 @@ "f19": { "body": [ { - "placeholder": { - "arg": 0, - "access": "read" + "method_call": { + "receiver": [ + { + "placeholder": { + "arg": 0, + "access": "read" + } + } + ], + "body": [ + { + "text": ".clone()" + } + ] } } ], "generics": { - "T1": [], + "T1": [ + "Clone" + ], "T2": [] }, "params": { diff --git a/rules/map/tgt_refcount.rs b/rules/map/tgt_refcount.rs index bff90712..ad950c5c 100644 --- a/rules/map/tgt_refcount.rs +++ b/rules/map/tgt_refcount.rs @@ -120,8 +120,8 @@ fn f17( RefcountMapIter::find_key(a0, &a1) } -fn f19(a0: RefcountMapIter) -> RefcountMapIter { - a0 +fn f19(a0: RefcountMapIter) -> RefcountMapIter { + a0.clone() } fn f20(a0: RefcountMapIter) -> Value { diff --git a/rules/map/tgt_unsafe.rs b/rules/map/tgt_unsafe.rs index a26a0be6..6fd8733a 100644 --- a/rules/map/tgt_unsafe.rs +++ b/rules/map/tgt_unsafe.rs @@ -83,8 +83,8 @@ unsafe fn f17(a0: BTreeMap>, a1: T1) -> UnsafeM UnsafeMapIterator::find_key(&a0 as *const BTreeMap>, &a1) } -unsafe fn f19(a0: UnsafeMapIterator) -> UnsafeMapIterator { - a0 +unsafe fn f19(a0: UnsafeMapIterator) -> UnsafeMapIterator { + a0.clone() } unsafe fn f20(a0: UnsafeMapIterator) -> *const T1 { diff --git a/tests/unit/initializer_list.cpp b/tests/unit/initializer_list.cpp new file mode 100644 index 00000000..96bcbc92 --- /dev/null +++ b/tests/unit/initializer_list.cpp @@ -0,0 +1,16 @@ +#include +#include +#include +#include + +size_t f(std::initializer_list bytes) { + std::vector *buf = new std::vector(bytes); + size_t n = bytes.size(); + delete buf; + return n; +} + +int main() { + assert(f({1, 2, 3}) == 3); + return 0; +} diff --git a/tests/unit/out/refcount/initializer_list.rs b/tests/unit/out/refcount/initializer_list.rs new file mode 100644 index 00000000..94bf5756 --- /dev/null +++ b/tests/unit/out/refcount/initializer_list.rs @@ -0,0 +1,27 @@ +extern crate libcc2rs; +use libcc2rs::*; +use std::cell::RefCell; +use std::collections::BTreeMap; +use std::io::prelude::*; +use std::io::{Read, Seek, Write}; +use std::os::fd::AsFd; +use std::rc::{Rc, Weak}; +pub fn f_0(bytes: Vec) -> u64 { + let bytes: Value> = Rc::new(RefCell::new(bytes)); + let buf: Value>> = Rc::new(RefCell::new(Ptr::alloc((*bytes.borrow()).clone()))); + let n: Value = Rc::new(RefCell::new((*bytes.borrow()).len() as u64)); + (*buf.borrow()).delete(); + return (*n.borrow()); +} +pub fn main() { + std::process::exit(main_0()); +} +fn main_0() -> i32 { + assert!( + (({ + let _bytes: Vec = vec![1, 2, 3]; + f_0(_bytes) + }) == 3_u64) + ); + return 0; +} diff --git a/tests/unit/out/refcount/map.rs b/tests/unit/out/refcount/map.rs index 7af8fb5f..ef8db5fe 100644 --- a/tests/unit/out/refcount/map.rs +++ b/tests/unit/out/refcount/map.rs @@ -192,7 +192,8 @@ fn main_0() -> i32 { &1_i16, ))); let const_it: Value> = Rc::new(RefCell::new( - RefcountMapIter::find_key((m.as_pointer() as Ptr>>), &10_i16), + RefcountMapIter::find_key((m.as_pointer() as Ptr>>), &10_i16) + .clone(), )); let x1: Value = Rc::new(RefCell::new(if (*it.borrow()) == (*end.borrow()) { 0_u32 @@ -200,11 +201,13 @@ fn main_0() -> i32 { (*(*it.borrow()).second().borrow()) })); assert!(((*x1.borrow()) == 4_u32)); - let x2: Value = Rc::new(RefCell::new(if (*const_it.borrow()) == (*end.borrow()) { - 0_u32 - } else { - (*(*const_it.borrow()).second().borrow()) - })); + let x2: Value = Rc::new(RefCell::new( + if (*const_it.borrow()) == (*end.borrow()).clone() { + 0_u32 + } else { + (*(*const_it.borrow()).second().borrow()) + }, + )); assert!(((*x2.borrow()) == 0_u32)); let x3: Value = Rc::new(RefCell::new( if (*it.borrow()) @@ -218,7 +221,7 @@ fn main_0() -> i32 { assert!(((*x3.borrow()) == 4_u32)); let x4: Value = Rc::new(RefCell::new( if (*const_it.borrow()) - == RefcountMapIter::end((m.as_pointer() as Ptr>>)) + == RefcountMapIter::end((m.as_pointer() as Ptr>>)).clone() { 0_u32 } else { @@ -274,7 +277,7 @@ fn main_0() -> i32 { ); RefcountMapIter::erase( ((r).clone() as Ptr>>), - &(*it4.borrow()), + &(*it4.borrow()).clone(), ); assert!(((*r.upgrade().deref()).len() as u64 == 3_u64)); assert!( diff --git a/tests/unit/out/refcount/redundant_copy_in_conversion.rs b/tests/unit/out/refcount/redundant_copy_in_conversion.rs new file mode 100644 index 00000000..d1fd38e1 --- /dev/null +++ b/tests/unit/out/refcount/redundant_copy_in_conversion.rs @@ -0,0 +1,55 @@ +extern crate libcc2rs; +use libcc2rs::*; +use std::cell::RefCell; +use std::collections::BTreeMap; +use std::io::prelude::*; +use std::io::{Read, Seek, Write}; +use std::os::fd::AsFd; +use std::rc::{Rc, Weak}; +pub fn sink_0(it: RefcountMapIter) -> i32 { + let it: Value> = Rc::new(RefCell::new(it)); + let cit: Value> = Rc::new(RefCell::new((*it.borrow()).clone())); + return if (*cit.borrow()) == (*it.borrow()).clone() { + (*(*it.borrow()).second().borrow()) + } else { + 0 + }; +} +pub fn main() { + std::process::exit(main_0()); +} +fn main_0() -> i32 { + let m: Value>> = Rc::new(RefCell::new(BTreeMap::new())); + (m.as_pointer() as Ptr>>) + .with_mut(|__v: &mut BTreeMap>| { + __v.entry(0.clone()) + .or_insert_with(|| Rc::new(RefCell::new(::default()))) + .as_pointer() + }) + .write(1); + let end: Value> = Rc::new(RefCell::new(RefcountMapIter::end( + (m.as_pointer() as Ptr>>), + ))); + let it0: Value> = Rc::new(RefCell::new(RefcountMapIter::find_key( + (m.as_pointer() as Ptr>>), + &0, + ))); + let const_it: Value> = Rc::new(RefCell::new((*it0.borrow()).clone())); + let r: Value = Rc::new(RefCell::new( + if (*const_it.borrow()) == (*end.borrow()).clone() { + 0 + } else { + 1 + }, + )); + (*r.borrow_mut()) += ({ + let _it: RefcountMapIter = (*it0.borrow()).clone(); + sink_0(_it) + }); + (*r.borrow_mut()) += if (*end.borrow()) == (*end.borrow()) { + 0 + } else { + 1 + }; + return (*r.borrow()); +} diff --git a/tests/unit/out/unsafe/initializer_list.rs b/tests/unit/out/unsafe/initializer_list.rs new file mode 100644 index 00000000..dbec9d20 --- /dev/null +++ b/tests/unit/out/unsafe/initializer_list.rs @@ -0,0 +1,28 @@ +extern crate libc; +use libc::*; +extern crate libcc2rs; +use libcc2rs::*; +use std::collections::BTreeMap; +use std::io::{Read, Seek, Write}; +use std::os::fd::{AsFd, FromRawFd, IntoRawFd}; +use std::rc::Rc; +pub unsafe fn f_0(mut bytes: Vec) -> u64 { + let mut buf: *mut Vec = (Box::leak(Box::new(bytes.clone())) as *mut Vec); + let mut n: u64 = bytes.len() as u64; + ::std::mem::drop(Box::from_raw(buf)); + return n; +} +pub fn main() { + unsafe { + std::process::exit(main_0() as i32); + } +} +unsafe fn main_0() -> i32 { + assert!( + ((unsafe { + let _bytes: Vec = vec![1, 2, 3]; + f_0(_bytes) + }) == (3_u64)) + ); + return 0; +} diff --git a/tests/unit/out/unsafe/map.rs b/tests/unit/out/unsafe/map.rs index 88b42d14..a20b2ebe 100644 --- a/tests/unit/out/unsafe/map.rs +++ b/tests/unit/out/unsafe/map.rs @@ -50,10 +50,10 @@ unsafe fn main_0() -> i32 { let mut it: UnsafeMapIterator = UnsafeMapIterator::find_key(&m as *const BTreeMap>, &1_i16); let mut const_it: UnsafeMapIterator = - UnsafeMapIterator::find_key(&m as *const BTreeMap>, &10_i16); + UnsafeMapIterator::find_key(&m as *const BTreeMap>, &10_i16).clone(); let mut x1: u32 = if it == end { 0_u32 } else { *it.second() }; assert!(((x1) == (4_u32))); - let mut x2: u32 = if const_it == end { + let mut x2: u32 = if const_it == end.clone() { 0_u32 } else { *const_it.second() @@ -65,11 +65,12 @@ unsafe fn main_0() -> i32 { *it.second() }; assert!(((x3) == (4_u32))); - let mut x4: u32 = if const_it == UnsafeMapIterator::end(&m as *const BTreeMap>) { - 0_u32 - } else { - *const_it.second() - }; + let mut x4: u32 = + if const_it == UnsafeMapIterator::end(&m as *const BTreeMap>).clone() { + 0_u32 + } else { + *const_it.second() + }; assert!(((x4) == (0_u32))); (*m.entry(4_i16).or_default().as_mut()) = 5_u32; let mut it4: UnsafeMapIterator = diff --git a/tests/unit/out/unsafe/redundant_copy_in_conversion.rs b/tests/unit/out/unsafe/redundant_copy_in_conversion.rs new file mode 100644 index 00000000..d78561fc --- /dev/null +++ b/tests/unit/out/unsafe/redundant_copy_in_conversion.rs @@ -0,0 +1,33 @@ +extern crate libc; +use libc::*; +extern crate libcc2rs; +use libcc2rs::*; +use std::collections::BTreeMap; +use std::io::{Read, Seek, Write}; +use std::os::fd::{AsFd, FromRawFd, IntoRawFd}; +use std::rc::Rc; +pub unsafe fn sink_0(mut it: UnsafeMapIterator) -> i32 { + let mut cit: UnsafeMapIterator = it.clone(); + return if cit == it.clone() { *it.second() } else { 0 }; +} +pub fn main() { + unsafe { + std::process::exit(main_0() as i32); + } +} +unsafe fn main_0() -> i32 { + let mut m: BTreeMap> = BTreeMap::new(); + (*m.entry(0).or_default().as_mut()) = 1; + let mut end: UnsafeMapIterator = + UnsafeMapIterator::end(&m as *const BTreeMap>); + let mut it0: UnsafeMapIterator = + UnsafeMapIterator::find_key(&m as *const BTreeMap>, &0); + let mut const_it: UnsafeMapIterator = it0.clone(); + let mut r: i32 = if const_it == end.clone() { 0 } else { 1 }; + r += (unsafe { + let _it: UnsafeMapIterator = it0.clone(); + sink_0(_it) + }); + r += if end == end { 0 } else { 1 }; + return r; +} diff --git a/tests/unit/redundant_copy_in_conversion.cpp b/tests/unit/redundant_copy_in_conversion.cpp new file mode 100644 index 00000000..cbea23b1 --- /dev/null +++ b/tests/unit/redundant_copy_in_conversion.cpp @@ -0,0 +1,26 @@ +#include + +static int sink(std::map::iterator it) { + std::map::const_iterator cit(it); + return cit == it ? it->second : 0; +} + +int main() { + std::map m; + m[0] = 1; + std::map::iterator end = m.end(); + std::map::iterator it0 = m.find(0); + std::map::const_iterator const_it = it0; + // Comparing const_iterator with iterator forces an implicit conversion of + // `end` to const_iterator. The AST shape differs between platforms: + // + // Linux: const_it == ConvertingCtor(end) + // macOS: const_it == ConvertingCtor(CopyCtor(end)) + // + // The extra inner CopyCtor on macOS would emit a redundant .clone() in the + // generated Rust. cpp2rust suppresses it so the output matches Linux. + int r = const_it == end ? 0 : 1; + r += sink(it0); + r += end == end ? 0 : 1; + return r; +}