Mz/string tests #1767
Mz/string tests #1767
Conversation
mayeul-zama
force-pushed
the
mz/string_tests
branch
from
2e7f5dc to
cdc36bf
Compare
mayeul-zama
force-pushed
the
mz/string_tests
branch
from
cdc36bf to
ddecebe
Compare
|
| loop { | ||
| match rhs.len().cmp(&lhs.len()) { | ||
| Ordering::Less => rhs.append_null(self), | ||
| Ordering::Equal => break, | ||
| Ordering::Greater => lhs.append_null(self), | ||
| } | ||
| } |
There was a problem hiding this comment.
looks inefficient as we can have the lenght of both strings and extend once ?
There was a problem hiding this comment.
IMO the code is simpler this way
There was a problem hiding this comment.
let len_diff = rhs.len() as isize - lhs.len() as isze;
match len_diff.cmp(&0) {
Ordering::Less => rhs.extend_null(len_diff().abs()),
Ordering::Equal => (),
Ordering::Greater => lhs.extend_null(len_diff().abs()),
}
does not look bad IMO
tfhe/src/strings/server_key/mod.rs
Outdated
| (true, false) | (false, true) => { | ||
| // We cannot assume the result is not padded. | ||
| // We ensure that it is padded with a single null | ||
| // which is ignored by our implementations |
There was a problem hiding this comment.
This comment has only been moved in this PR
I guess it meant that the result is still correct
I think we can just remove this last line, wdyt?
There was a problem hiding this comment.
I have no idea I don't know the string code, if we do indeed ignore something there it's fine to keep the comment
mayeul-zama
force-pushed
the
mz/string_tests
branch
2 times, most recently
from
19f43f0 to
69fe184
Compare
tfhe/src/strings/mod.rs
Outdated
| fn test_all2() { | ||
| test_all( |
There was a problem hiding this comment.
test_all calls test_all_impl
mayeul-zama
force-pushed
the
mz/string_tests
branch
from
69fe184 to
4d9e291
Compare
mayeul-zama
force-pushed
the
mz/string_tests
branch
from
4d9e291 to
77a6d00
Compare
| let uint = str.to_uint(); | ||
| let mut shift_bits = self.scalar_left_shift_parallelized(shift, 3); | ||
|
|
||
| // `shift_bits` needs to have the same block len as `uint` for the tfhe-rs shift to work | ||
| self.pad_or_trim_ciphertext(&mut shift_bits, uint.blocks().len()); | ||
|
|
||
| let shifted = self.right_shift_parallelized(&uint, &shift_bits); | ||
| let len = uint.blocks().len(); | ||
|
|
||
| let shifted = if len == 0 { | ||
| uint | ||
| } else { | ||
| self.right_shift_parallelized(&uint, &shift_bits) | ||
| }; |
There was a problem hiding this comment.
This is a note for later: As we now have block rotation available, it might be better, so this should be measured at some point
There was a problem hiding this comment.
can we have a follow up issue so we don't forget ?
There was a problem hiding this comment.
| /// let sk = ServerKey::new_radix_server_key(&ck); | ||
| /// let (s1, s2) = ("hello", "hello"); | ||
| /// | ||
| /// let enc_s1 = FheString::new(&ck, s1, None); | ||
| /// let enc_s2 = GenericPattern::Enc(FheString::new(&ck, s2, None)); | ||
| /// | ||
| /// let result = sk.eq(&enc_s1, &enc_s2); | ||
| /// let result = sk.string_eq(&enc_s1, enc_s2.as_ref()); |
There was a problem hiding this comment.
that's not great from a usability standpoint :|
|
Why do we need the GenericPatternRef again ? I don't see what we are gaining there, looks like a downgrade forcing users to use as_ref() where they could have "just" used an "&" on the pattern ? Looks good to merge to move forward though, just would like an answer on the GenericPattern thing which could be changed in a later PR |
|
I believe GenericPatternRef is there so that we do not need to own the value that is in the enum removing some clones If we still want the user to be able to do pub struct FheString;
enum GenericPaternRef<'a> {
Clear(&'a str),
Enc(&'a FheString)
}
impl<'a> From<&'a String> for GenericPaternRef<'a>{
fn from(value: &'a String) -> Self {
Self::Clear(value.as_str())
}
}
impl<'a> From<&'a str> for GenericPaternRef<'a>{
fn from(value: &'a str) -> Self {
Self::Clear(value)
}
}
// This one is so that `&"litetal"` works
impl<'a, 'b> From<&'a &'b str> for GenericPaternRef<'a>{
fn from(value: &'a &'b str) -> Self {
Self::Clear(*value)
}
}
impl<'a> From<&'a FheString> for GenericPaternRef<'a>{
fn from(value: &'a FheString) -> Self {
Self::Enc(value)
}
}
fn trim<'a>(string: &FheString, pat: impl Into<GenericPaternRef<'a>>) {
match pat.into() {
GenericPaternRef::Clear(_) => {todo!()},
GenericPaternRef::Enc(_) => {todo!()},
}
}
fn main() {
let fhe_string = FheString;
let fhe_pat = FheString;
let clear_pat = "Yoshi";
let clear_pat_str = "Yoshi".to_string();
trim(&fhe_string, &fhe_pat);
trim(&fhe_string, &clear_pat);
trim(&fhe_string, &clear_pat_str);
} |
It is indeed to avoid clones. In particular in the HLAPI wrappers. I think having simple and clean APIs in |
No description provided.