[–] 3 points 9 months ago

So try_from(&**p) is not a code smell/poor form in Rust?

No. It's how you (explicitly) go from ref to deref.

Here:

  • p is &PathBuf
  • *p is PathBuf
  • **p is Path (Deref)
  • And &**p is &Path.

Since what you started with is a reference to a non-Copy value, you can't do anything that would use/move *p or **p. Furthermore, Path is an unsized type (just like str and [T]), so you need to reference it (or Box it) in any case.

Another way to do this is:

let p: &Path = p.as_ref();

Some APIs use AsRef in signatures to allow passing references of different types directly (e.g. File::open()), but that doesn't apply here.

  • source
  • parent
  • context
  • [–] [S] 1 point 9 months ago

    Yes, thank you. Mainly, I've noticed this opportunity after I cleaned up the code from all the previous back and forth. I decided to leave it as it was to get an opinion on that. Also, I clearly had wrong understanding about how real Crustaceans use ?

  • source
  • parent
  • context
  • [–] 3 points 9 months ago (1 child)

    Not very familiar with the libraries, and BB_C has already given a lot of more detailed feedback, but here are some small things:

    • Even without rewriting get_files_in_dir as a single chain of method calls, you can still replace
      if dir.is_err() {  
          return None  
      };
      // use dir.unwrap()
      
      with
      let Ok(dir) = dir else {
          return None;
      };
      // use dir
      
      In general, if you can use if let or let else to pattern match, prefer that over unwrapping -- Clippy has a lint relating to this, actually.
    • Although the formatting doesn't seem horrible, it doesn't seem like Rustfmt was used on it. You might want to make use of it for slightly more consistent formatting. I mainly noticed the lack of trailing commas, and the lack of semicolons after return None in get_files_in_dir.
  • source
  • [–] -1 points 9 months ago (2 children)

    I don't know what was going on behind the scenes but as a user of their mail app it felt barebones. So to see them working on a full rewrite for years instead of moving forward is kinda surprising. Like bro its a mail app features are more important than speed or memory safety.

  • source
  • [–] 1 point 9 months ago

    I've been happy to see proton begin trending in what I see as the right direction (for me as a consumer). Between this uplift and finally talking drive for Linux it's been a happy couple of weeks.

  • source
  • [–] 6 points 9 months ago (2 children)

    Let's do this incrementally, shall we?

    First, let's make get_files_in_dir() idiomatic. We will get back to errors later.

    fn get_files_in_dir(dir: &str) -> Option<Vec<PathBuf>> {
        fs::read_dir(dir)
            .ok()?
            .map(|res| res.map(|e| e.path()))
            .collect::<Result<Vec<_>, _>>()
            .ok()
    }
    

    Now, in read_parquet_dir(), if the unwraps stem from confidence that we will never get errors, then we can confidently ignore them (we will get back to the errors later).

    fn read_parquet_dir(entries: &Vec<String>) ->  impl Iterator<Item = record::Row> {
        // ignore all errors
        entries.iter()
            .cloned()
            .filter_map(|p| SerializedFileReader::try_from(p).ok())
            .flat_map(|r| r.into_iter())
            .filter_map(|r| r.ok())
    }
    

    Now, let's go back to get_files_in_dir(), and not ignore errors.

    fn get_files_in_dir(dir: &str) -> Result<Vec<PathBuf>, io::Error>
    {
        fs::read_dir(dir)?
            .map(|res| res.map(|e| e.path()))
            .collect::<Result<Vec<_>, _>>()
    }
    
     
     fn main() -> Result<(), io::Error> {
         let args = Args::parse();
    -    let entries = match get_files_in_dir(&args.dir)
    -    {
    -        Some(entries) => entries,
    -        None => return Ok(())
    -    };
    -
    +    let entries = get_files_in_dir(&args.dir)?;
     
         let mut wtr = WriterBuilder::new().from_writer(io::stdout());
         for (idx, row) in read_parquet_dir(&entries.iter().map(|p| p.display().to_string()).collect()).enumerate() {
    
    

    Now, SerializedFileReader::try_from() is implemented for &Path, and PathBuf derefs to &Path. So your dance of converting to display then to string (which is lossy btw) is not needed.

    While we're at it, let's use a slice instead of &Vec<_> in the signature (clippy would tell you about this if you have it set up with rust-analyzer).

    
    fn read_parquet_dir(entries: &[PathBuf]) ->  impl Iterator<Item = record::Row> {
        // ignore all errors
        entries.iter()
            .filter_map(|p| SerializedFileReader::try_from(&**p).ok())
            .flat_map(|r| r.into_iter())
            .filter_map(|r| r.ok())
    }
    
         let entries = get_files_in_dir(&args.dir)?;
     
         let mut wtr = WriterBuilder::new().from_writer(io::stdout());
    -    for (idx, row) in read_parquet_dir(&entries.iter().map(|p| p.display().to_string()).collect()).enumerate() {
    +    for (idx, row) in read_parquet_dir(&entries).enumerate() {
             let values: Vec<String> = row.get_column_iter().map(|(_column, value)| value.to_string()).collect();
             if idx == 0 {
                 wtr.serialize(row.get_column_iter().map(|(column, _value)| column.to_string()).collect::<Vec<String>>())?;
    
    
    

    Now let's see what we can do about not ignoring errors in read_parquet_dir().


    Approach 1: Save intermediate reader results

    This consumes all readers before getting further. So, it's a behavioral change. The signature may also scare some people 😉

    fn read_parquet_dir(entries: &Vec<PathBuf>) ->  Result<impl Iterator<Item = Result<record::Row, ParquetError>>, ParquetError> {
        Ok(entries
            .iter()
            .map(|p| SerializedFileReader::try_from(&**p))
            .collect::<Result<Vec<_>, _>>()?
            .into_iter()
            .flat_map(|r| r.into_iter()))
    }
    

    Approach 2: Wrapper iterator type

    How can we combine errors from readers with flat record results?

    This is how.

    enum ErrorOrRows {
        Error(Option<ParquetError>),
        Rows(record::reader::RowIter<'static>)
    }
    
    impl Iterator for ErrorOrRows {
        type Item = Result<record::Row, ParquetError>;
    
        fn next(&mut self) -> Option<Self::Item> {
            match self {
                Self::Error(e_opt) => e_opt.take().map(Err),
                Self::Rows(row_iter) => row_iter.next(),
            }
        }
    }
    
    fn read_parquet_dir(entries: &[PathBuf]) ->  impl Iterator<Item = Result<record::Row, ParquetError>>
    {
        entries
            .iter()
            .flat_map(|p| match  SerializedFileReader::try_from(&**p) {
                Err(e) => ErrorOrRows::Error(Some(e)),
                Ok(sr) => ErrorOrRows::Rows(sr.into_iter()),
            })
    }
    
     
         let mut wtr = WriterBuilder::new().from_writer(io::stdout());
         for (idx, row) in read_parquet_dir(&entries).enumerate() {
    +        let row = row?;
             let values: Vec<String> = row.get_column_iter().map(|(_column, value)| value.to_string()).collect();
             if idx == 0 {
                 wtr.serialize(row.get_column_iter().map(|(column, _value)| column.to_string()).collect::<Vec<String>>())?;
    

    Approach 3 (bonus): Using unstable #![feature(gen_blocks)]

    fn read_parquet_dir(entries: &[PathBuf]) ->  impl Iterator<Item = Result<record::Row, ParquetError>> {
        gen move {
            for p in entries {
                match SerializedFileReader::try_from(&**p) {
                    Err(e) => yield Err(e),
                    Ok(sr) => for row_res in sr { yield row_res; }
                }
            }
        }
    }
    
  • source
  • [–] 4 points 9 months ago (1 child)

    Your usage and implementation of get_files_in_dir is quite awkward. It is returning None on errors and then you're returning Ok(()) when it returns None, ignoring a bunch of errors. Why not just have get_files_in_dir return a Result<Vec<PathBuf>, io::Error>? You could drastically simplify the function by just ?ing the results.

  • source
  • [–] 13 points 9 months ago

    Rust doesn't have inheritance like in traditional OOP languages because it doesn't have virtual methods. You have to manually implement methods to delegate to base classes.

    Also what is this trash meme?

  • source
  • [–] 3 points 9 months ago*

    Deref works on top of encapsulation though. Inheritance syntactically hides the encapsulation, with Deref it's in the clear.

    It's true that it feels like inheritance, but I'm grateful for it. Otherwise it would be a pain to use the windows API.

  • source
  • [–] 4 points 9 months ago

    It was just a quick solution showing how to do it with one macro and zero dependencies, utilizing the power of meta variables. It probably can be made nicer.

    Side Note: It's not directly relevant/needed here, but this also shows that maybe a theoretical $reverse{} meta variable can be useful with repeated patterns, to in this case generate the variants in the expected order.

  • source
  • parent
  • context
  • [–] 2 points 9 months ago* (last edited 9 months ago) (2 children)

    (slightly edited from initial version)

    #![feature(macro_metavar_expr_concat)]
    #![feature(macro_metavar_expr)]
    
    macro_rules! gen_enums {
        ([$($name:ident)+]) => {
            gen_enums!(${count($name)}: [$($name)+]);
        };
        ($n:literal: [$($name:ident)+]) => {
            enum ${concat(Position, $n)} { $($name,)+ }
        };
        ($name:ident, $($tail:ident,)*) => {
            gen_enums!([$name $($tail)*]);
            gen_enums!($($tail,)*);
        };
        () => {};
    }
    
  • source
  • [–] 3 points 9 months ago

    As with all ads, especially M$ ones..
    No Code, Don't Care

    At least if the code was available, I would find out what they mean by "spoofed Mime" and how that attack vector works (Is the actual file "magic" header spoofed, but the file still manages to get parsed with its non-"spoofed" actual format none the less?!, How?).

    Also, I would have figured out if this is a new use of "at scale" applied to purely client code, or if a service is actually involved.

  • source
  • [–] 11 points 9 months ago

    Sort of cool from an engineering standpoint, but realistically the best layer of security for WhatsApp is to not use it. Meta is a security vulnerability.

  • source
  • view more: ‹ prev next ›