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

[GSoC] Make LLVM back-end support multiple/space-separated ccflags (Fixes #5510) #5511

Merged
merged 3 commits into from
Mar 13, 2017

Conversation

coodie
Copy link
Contributor

@coodie coodie commented Mar 4, 2017

PR contributed by @coodie - thanks!

This fixes #5510. Not a big deal, simply split passed arguments by spaces.

Reviewed by @mppf.

Tested:

  • hellos pass with CHPL_LLVM=none
  • test/release/examples/primers and test/extern/ferguson with CHPL_LLVM=llvm
  • test/release/examples/primers and test/extern/ferguson with CHPL_LLVM=llvm and --llvm

@bradcray bradcray changed the title Fix for #5510 Make LLVM back-end support multiple/space-separated ccflags (Fixes #5510) Mar 6, 2017
@bradcray
Copy link
Member

bradcray commented Mar 6, 2017

@mppf: You are the most obvious reviewer for this PR, if you have time to look at it.

@coodie: One other thing that this PR should include before being merged is a test that will lock in the working behavior to make sure we don't backslide in the future. Let us know if you need pointers into the Chapel testing system for help on how to do this.

@coodie
Copy link
Contributor Author

coodie commented Mar 6, 2017

I like how you put effort to testing even little changes like this :), that's really proffesional.

@bradcray I have added locking tests. Please take a look.

@bradcray
Copy link
Member

bradcray commented Mar 6, 2017

Hi @coodie: If you were to check in this new test separately/first (say at the same time you filed the issue), you would need to include the .future and .bad files as you've prepared them here. Since in this case, they're most likely to be merged at the same time as the fix itself, you can (and should) drop those files from the PR because otherwise our test system will just tell us that the future is passing (an indication that its .future and .bad files should be removed). Put another way, since you'll be filing the fix, it ceases to be a test that will work in the "future" and should work now instead.

@mppf
Copy link
Member

mppf commented Mar 7, 2017

@bradcray, @coodie - this looks fine to me. We just need somebody to shepherd it through testing.

@bradcray
Copy link
Member

bradcray commented Mar 7, 2017

Do you think it requires a full test run, or just for those tests that use ccflags and the llvm back-end?

@mppf
Copy link
Member

mppf commented Mar 9, 2017

I think testing test/release/examples/primers and test/extern/ferguson with CHPL_LLVM=llvm with and without --llvm would be sufficient.

  • hellos pass with CHPL_LLVM=none
  • test/release/examples/primers and test/extern/ferguson with CHPL_LLVM=llvm
  • test/release/examples/primers and test/extern/ferguson with CHPL_LLVM=llvm and --llvm

@mppf
Copy link
Member

mppf commented Mar 10, 2017

I plan to merge this PR on Monday.

@mppf mppf merged commit 939e9dd into chapel-lang:master Mar 13, 2017
@coodie coodie changed the title Make LLVM back-end support multiple/space-separated ccflags (Fixes #5510) [GSoC] Make LLVM back-end support multiple/space-separated ccflags (Fixes #5510) Aug 28, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clang arguments passed in wrong way when using --ccflags
4 participants