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

Implement pass_defines #109

Conversation

oriceemple
Copy link
Contributor

No description provided.

@eeide
Copy link
Member

eeide commented Jul 25, 2016

Interesting!

A preliminary comment: I'm not keen on system("clang ..."), because this is different than how other passes find their external programs.

@oriceemple
Copy link
Contributor Author

@eeide - How would you recommend writing it instead?

@eeide
Copy link
Member

eeide commented Jul 25, 2016

The pattern is to have a configure-time test for finding the program, "burn" the configure-time value into a constant in the file creduce_config.pm (which is created at configure-time by the Automake- or CMake-based build systems, from creduce_config.pm.in), and then use the value of that constant in the code that invokes the external program.

A good example of this is the handling of CLANG_FORMAT.

Let me know if the above explanation isn't enough to get you pointed in the right direction. The detection and handling of external programs can be a bit complicated!

@oriceemple
Copy link
Contributor Author

So is it ok for me to add the existence of the command-line clang as a new requirement to creduce?

@eeide
Copy link
Member

eeide commented Jul 25, 2016

So is it ok for me to add the existence of the command-line clang as a new requirement to creduce?

That is what your patch requires, isn't it? From my quick initial reading of the patch (which I have not returned to), I understand that your new pass does not work at run time unless clang is available. (So your new pass needs to check for this at run time, if it doesn't already.)

Whether or not clang is an acceptable run-time dependency—I haven't really thought about this yet. I suspect that it's fine, given that we already have clang-format as a required run-time dependency.

Likely, it would make sense for the configure-time default to be the clang that is part of whatever LLVM tree that C-Reduce is being built against (the same as clang-format).

@oriceemple
Copy link
Contributor Author

@eeide - I've applied your requests, see #110

@oriceemple oriceemple closed this Jul 25, 2016
@eeide
Copy link
Member

eeide commented Jul 25, 2016

OK, thanks.

By the way, you don't have to make a new GitHub pull request every time you update the patch. In some ways it is better if you do not, because making new pull requests tends to scatter the discussion of the patch.

@oriceemple
Copy link
Contributor Author

I know that many people prefer to have the pull request containing a few big commits (for simple things, only one). If I modify the same branch in my working tree, I need to use --force on my github repo. This is ok by me, but is that ok with you?

@eeide
Copy link
Member

eeide commented Jul 25, 2016

This is what I would suggest: While the pull request is being discussed, just make new commits that address the issues raised during the discussion. If all goes well, and the pull request is to be merged, there might be a request for a squash at the end. (@regehr likes squashed pull requests as you describe. I like them, too, but I think I'm less picky about it, maybe.)

Does that make sense?

@oriceemple
Copy link
Contributor Author

Yes, thanks for explaining

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.

2 participants