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

Fixes #3092 - Implement loading @file c arguments #3178

Merged
merged 1 commit into from
Aug 18, 2020

Conversation

sudobash1
Copy link
Contributor

This is a fix for the bug #3092 I filed on expanding @file c arguments.

Just as background, a @file argument works like this: If gcc or clang encounters a flag which starts with @ it will treat the remainder of that flag as a file path, and it will use the contents of that file as flags. (It is somewhat like an "include file" for command line arguments).

Initially I implemented this as simply passing the @file arguments directly to the compiler, but then I realized that this could cause problems since the arguments contained with the file would not get parsed and filtered. If you are curious this implementation is here.

This PR instead has vim read the contents of the @file and will add those to the argument list to be filtered.

I have added a Vader test for this.

Thanks for you work!

@stale
Copy link

stale bot commented Aug 13, 2020

This pull request has been automatically marked as stale because it has not been updated recently. Make sure to write tests and document your changes. See :help ale-dev for information on writing tests.
If your pull request is good to merge, bother w0rp or another maintainer again, and get them to merge it.

@stale stale bot added the stale PRs a bot will close automatically label Aug 13, 2020
@stale stale bot closed this Aug 15, 2020
@w0rp w0rp reopened this Aug 18, 2020
@w0rp w0rp self-assigned this Aug 18, 2020
@w0rp w0rp added this to In Review in On the Radar via automation Aug 18, 2020
@stale stale bot removed stale PRs a bot will close automatically labels Aug 18, 2020
Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

I say we just give this a shot and see if anyone notices any issues. I can't think of a better way to handle this.

@w0rp w0rp merged commit e27d437 into dense-analysis:master Aug 18, 2020
On the Radar automation moved this from In Review to Done Aug 18, 2020
@w0rp
Copy link
Member

w0rp commented Aug 18, 2020

Cheers! 🍻

@w0rp
Copy link
Member

w0rp commented Aug 18, 2020

If we end up with someone having issues with readfile() calls being too slow, we have the option later of storing the result of readfile() for @file arguments in a Dictionary, so we don't read the file from disk all the time.

@sudobash1
Copy link
Contributor Author

I think this is my first open-source PR to get merged. Thanks for your work on this project @w0rp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants