Skip to content

refactor(ctypes): inline some functions#6375

Merged
rgrinberg merged 1 commit intomainfrom
ps/rr/refactor_ctypes___inline_some_functions
Nov 2, 2022
Merged

refactor(ctypes): inline some functions#6375
rgrinberg merged 1 commit intomainfrom
ps/rr/refactor_ctypes___inline_some_functions

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

@rgrinberg rgrinberg commented Nov 2, 2022

they aren't general enough to be useful

Signed-off-by: Rudi Grinberg me@rgrinberg.com

they aren't general enough to be useful

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

ps-id: 841f11b1-5d34-44e4-92c0-03cb571d8790
@rgrinberg rgrinberg force-pushed the ps/rr/refactor_ctypes___inline_some_functions branch from 92b26aa to f96e519 Compare November 2, 2022 14:39
@rgrinberg rgrinberg requested a review from mbacarella November 2, 2022 14:39
@mbacarella
Copy link
Copy Markdown
Collaborator

I found the various stages of ctypes rules generation confusing enough that I thought people coming to it fresh and trying to understand what's going on would be next-level confused. Putting them into separate small top-level named functions was my attempt to discretize and document the steps a bit better. If you think it's just contributing noise I'm happy with inlining them.

(I also did some things in an unnecessarily clunky way out of being unfamiliar with better utility functions; improvements there are obviously great)

@rgrinberg
Copy link
Copy Markdown
Member Author

The problem with wrapping already api's is that it forces the after mentioned potential contributors to learn 2 api's instead of one. At some point, the team decided that all the wrappers that save line of code here and there don't contribute enough and make it more difficult to refactor and learn dune's api.

Copy link
Copy Markdown
Collaborator

@mbacarella mbacarella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough

@rgrinberg rgrinberg merged commit dfa9116 into main Nov 2, 2022
jchavarri added a commit to jchavarri/dune that referenced this pull request Nov 3, 2022
* main:
  refactor(coq): flatten coqc and coqc_dir into record
  chore: remove melange from CHANGES (ocaml#6378)
  refactor(ctypes): inline some functions (ocaml#6375)
  chore: fix typo in CHANGES (ocaml#6376)
  fix(ctypes): remove unnecessary case changes (ocaml#6374)
  refactor: replace bash with sh (ocaml#6373)
  fix: [Path.Build.to_string_maybe_quoted] (ocaml#6371)
@Alizter Alizter deleted the ps/rr/refactor_ctypes___inline_some_functions branch November 12, 2022 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants