Simplify Pkg_rules.source_files a bit#14301
Conversation
Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com> Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>
| let acc = | ||
| Path.Local.Set.of_list_map files ~f:(Path.Local.relative path) | ||
| |> Path.Local.Set.union acc | ||
| | S_DIR -> if skip_dir name then Skip else Right relative |
There was a problem hiding this comment.
I think this if needs to be pulled out above the let releative. We always consider skipping, but we don't always need to construct relatative.
There was a problem hiding this comment.
You mean something like this:
List.rev... ->
match kind with
| S_DIR -> if skip_dir name then Skip else Right (Path.Local.relative path name)
| _ -> if skip_file name then Skip else Left (Path.Local.relative path name))It'd go against the idea of not repeating the relative call, for what I perceive to be minimal benefit
Did you miss that skip_dir and skip_file are different functions?
Alizter
left a comment
There was a problem hiding this comment.
I've asked copilot to review in case it notices anything important.
There was a problem hiding this comment.
Pull request overview
This PR refactors Pkg_rules.Pkg.source_files to compute Path.Local.relative once during directory partitioning, keeping behavior the same while simplifying downstream mapping and making future debugging inside the partition function easier.
Changes:
- Compute
relativepaths once in therev_filter_partition_mapcallback and carryPath.Local.tvalues forward. - Simplify accumulation by removing
of_list_mapand eliminating an extrarelativemapping step for subdirectories.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Just a trivial simplification in
Pkg_rules.source_files, nothing changing dune behaviour.Instead of
mapping therelativepart later, do it once in the partition.This isn't particularly useful on its own, but in case of changes inside the partition function, it makes debugging a lot easier
Extracted from #13792