Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zig cc: add support for -L linker arguments #4893

Merged
merged 1 commit into from
Apr 1, 2020

Conversation

redj
Copy link
Contributor

@redj redj commented Apr 1, 2020

This is my first zig change. Do I need to run any tests? or add any test cases?
edit: This is towards the completion of zig cc (#4784)

@squeek502
Copy link
Collaborator

-L, --library-directory, and --library-directory= are all equivalent

So you should be able to add:

    .{
        .name = "library-directory",
        .ident = "linker_input_l",
    },
    .{
        .name = "library-directory=",
        .ident = "linker_input_l",
    },

to update_clang_options.zig also.

@andrewrk
Copy link
Member

andrewrk commented Apr 1, 2020

Note that the script will try appending a = automatically, so only the first one is necessary. You do need to include the shorter and longer forms though.

src/main.cpp Outdated
Comment on lines 733 to 734
linker_args.append(buf_create_from_str("-L"));
linker_args.append(buf_create_from_str(it.only_arg));
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this you can simply do:

lib_dirs.append(it.only_arg);

This matches the -L parameter from zig command line which you can see earlier in the file.

@@ -223,7 +227,7 @@ pub fn main() anyerror!void {
try std.fmt.allocPrint(allocator, "-I={}/clang/include/clang/Driver", .{llvm_src_root}),
};

const child_result = try std.ChildProcess.exec(.{
const child_result = try std.ChildProcess.exec2(.{
Copy link
Member

Choose a reason for hiding this comment

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

if you update to zig master branch then exec will be correct

@andrewrk
Copy link
Member

andrewrk commented Apr 1, 2020

This is my first zig change. Do I need to run any tests? or add any test cases?

Thanks for the contribution! zig cc has no test harness yet, so for now we rely on careful code review.

src/main.cpp Outdated
@@ -790,6 +794,9 @@ static int main0(int argc, char **argv) {
buf_eql_str(arg, "-export-dynamic"))
{
rdynamic = true;
} else if (buf_eql_str(arg, "-L")) {
i += 1;
lib_dirs.append(buf_ptr(linker_args.at(i)));
Copy link
Contributor Author

@redj redj Apr 1, 2020

Choose a reason for hiding this comment

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

Should I remove this change since I'm now adding to lib_dirs directly instead of adding to linker_args?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it.

@redj redj force-pushed the zig-cc-add-linker-flag-l branch from 7162bc0 to 7be005a Compare April 1, 2020 18:18
@redj redj force-pushed the zig-cc-add-linker-flag-l branch from 7be005a to dfe33e6 Compare April 1, 2020 18:24
@andrewrk andrewrk merged commit 2b6dfdd into ziglang:master Apr 1, 2020
@andrewrk
Copy link
Member

andrewrk commented Apr 1, 2020

Nice work!

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.

None yet

3 participants