Skip to content

PR#6794: pass -package flags when building C files#150

Closed
ghost wants to merge 2 commits intotrunkfrom
unknown repository
Closed

PR#6794: pass -package flags when building C files#150
ghost wants to merge 2 commits intotrunkfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Mar 5, 2015

Patch for 6794.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are you exposing an optional argument (the optional argument itself looks rather fishy to me) if it is not used in the patch. Do you have a need for it in an external plugin?

(If so, I think that rather than an obscure API, I would prefer a locations : package list -> string list function to be exposed that allows people to implement whatever variant of include_flags without hassle.)

Note that I think naming the function "include_flags" rather than "compile_flags" is a good change.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Why are you exposing an optional argument (the optional argument itself looks rather fishy to me) if it is not used in the patch. Do you have a need for it in an external plugin?

I can useful if you need the list of "-L" for instance. I don't mind removing it.

(If so, I think that rather than an obscure API, I would prefer a locations : package list -> string list function to be exposed that allows people to implement whatever variant of include_flags without hassle.)

Yeah I though about that. Given that the function is quite small already it didn't seem worth it to split it even more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd prefer the patch without the hard-to-document optional argument, but I have nothing against exposing a separate function for -L if that is useful.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK, I'll remove it.

@gasche
Copy link
Copy Markdown
Member

gasche commented Mar 8, 2015

Merged in 4.02 and master, thanks.

@gasche gasche closed this Mar 8, 2015
@damiendoligez
Copy link
Copy Markdown
Member

@gasche Are you sure you merged in 4.02? I can't find the commit.

@gasche
Copy link
Copy Markdown
Member

gasche commented Mar 9, 2015

Indeed I had forgotten it. I just committed in 4.02@15891. Thanks for double-checking!

lthls added a commit to lthls/ocaml that referenced this pull request Jun 4, 2020
lthls added a commit to lthls/ocaml that referenced this pull request Sep 23, 2020
lthls added a commit to lthls/ocaml that referenced this pull request Sep 23, 2020
lthls added a commit to lthls/ocaml that referenced this pull request Sep 24, 2020
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Aug 10, 2021
stedolan pushed a commit to stedolan/ocaml that referenced this pull request Oct 5, 2021
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
Co-authored-by: Thibaut <thibaut.mattio@gmail.com>
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