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

Support for zig-cc and zig-c++ #22211

Closed
wants to merge 1 commit into from
Closed

Conversation

lars18th
Copy link

Add support to call the binary with "zig-cc" and "zig-c++" names. This will trigger the same as calling with "zig cc" and "zig-c++".

Add support to call the binary with "zig-cc" and "zig-c++" names. This will trigger the same as calling with "zig cc" and "zig-c++".
@lars18th lars18th marked this pull request as ready for review December 12, 2024 19:25
@jedisct1
Copy link
Contributor

See #8973

@mlugg
Copy link
Member

mlugg commented Dec 12, 2024

This PR implements the rejected proposal #8716. It's also immediately obvious that it would fail CI, but that's a moot point: this functionality is explicitly not something we want.

@mlugg mlugg closed this Dec 12, 2024
@lars18th
Copy link
Author

Hi @jedisct1 ,

See #8973

I've readed this several time ago. And this patch is not refusing anything discussed on this thread.

Hi @mlugg ,

This PR implements the rejected proposal #8716. It's also immediately obvious that it would fail CI, but that's a moot point: this functionality is explicitly not something we want.

Why you say "It's also immediately obvious that it would fail CI"? You've reviewed the patch? This patch doesn't change anything! The current behaviour continues exactly as it's now. All CI checks will continue running without troubles.

This patch only handles one special case: if the basename of the binary is zig-cc, in this case, and only in this case (the same for zig-c++) the command line parameters is increased with one more item. The new item will be in the position 1 and all others are rotated one position to the right. And the item is cc. Therefore if you call zig-cc param1 param2 ... the code parses zig-cc cc param1 param2 .... Where is the problem?

@mlugg
Copy link
Member

mlugg commented Dec 12, 2024

Why you say "It's also immediately obvious that it would fail CI"?

CI would immediately fail due to this code not conforming to zig fmt.

If you had included any kind of test coverage for this change, there would also have been a failure due to the unconditional calls to std.log.info in the added case.

This patch doesn't change anything!

That's very clearly not true. This patch changes how the zig binary behaves if argv[0] is anything other than zig.

Where is the problem?

The problem is that this proposal is rejected; the Zig project does not want to include this functionality.


Also, note that your code, while not buggy, contains several minor errors:

  • As mentioned above, std.log.info should never be called on non-debug success paths.
  • Using std.mem.rotate introduces an O(N) operation for absolutely no reason; just assign things correctly in your for loop.
  • The arena.free call is redundant; that's the whole point of an arena.

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.

3 participants