-
-
Notifications
You must be signed in to change notification settings - Fork 605
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
Pass D's global version conditionals to the preprocessor #16777
Conversation
Thanks for your pull request and interest in making D better, @ryuukk! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#16777" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This certainly need a changelog entry.
This probably also needs to be controllable, injecting a bunch of macros into C source code could have arbitrary effects, could collide with already defined macros (which will result in errors).
This might cause less problems with a prefix so that we pass -D _D_VERSION_Foo
for version = Foo;
. But this definitely requires some discussion.
Tests would also be good.
I thought of the prefix, but forgot to include it, it is now added I'll add a test if nothing else needs to change |
I don't like how i had to concatenate the strings, if anyone have a better idea, please let me know |
I don't know how to windows apparently, what's the right switch for defines? |
I think you have to concat the name to the switch instead of pushing "/D" separately. |
nothing works, i am out of idea, and i don't have a windows machine to test if you know windows, please submit a commit to this branch |
The code you're editing is in a dead |
In addition to Rikki's comment you I think you have en extraneous space at the end -argv.push(("/D_D_VERSION_"~id.toString()~" \0").ptr);
+argv.push(("/D_D_VERSION_"~id.toString()~"\0").ptr); |
Oh good catch lol, thanks |
If this ever gets merged, before that, i'd like to improve this code: foreach(id; global.versionids)
{
argv.push("-D");
argv.push(("_D_VERSION_"~id.toString()~" \0").ptr);
} There probably is a OutBuffer i could reuse, but i run out of time, i'll try tomorow |
@WalterBright What do you think about this PR? |
Thank you for submitting this. There is currently a switch to add preprocessor defines (and any other commands to it):
Currently, by default, C compilers typically predefined 400 macros, most of which are undocumented. Many have leading underscores. C code arbitrarily depends on these. And who knows what other macros the C code relies on.
It is an interesting idea, but defer it until we get considerably more experience with ImportC to see if it is really necessary. The lack of it is not going to be an impediment because of the |
I did a search on GH and couldn't find |
Hence the prefix, this is a solved issue already The usecase was the following: Have a D library that's easily configurable with "To consume my library you have to do: Nobody wants to do that |
Fair enough. But it's not very attractive.
anyway. |
I agree
That's not attractive User shouldn't care if library mixes both D/C, DMD is a compiler that supports both languages, there shouldn't be any difference how it handles version conditionals How do you tell D code what compiler version was used? and how do you tell it to C sources? do you duplicate your code? Duplication and repetition is the root of evil, seems like a common issue with D, repetition and noise At the very least, there should be a mechanisms that say: make version apply to both D/C, would this be a good compromise? a opt-in flag?.. when i think about it, i would prefer not, because it makes build scripts more complicated than it should be.. The solution lies in simplicity |
Adding another feature, when the desired result is easily obtained with an existing feature, makes things more complicated. |
You are right, however, i do not like having to pass 2 flags for the same thing, it tells me that i am doing something wrong, wich i am not |
You wouldn't be doing anything wrong. The preprocessor switch is pretty clear - it's for the preprocessor. |
cmon.. if you expect people to type this, i will ignore what you have to say on that matter from now on, it's like the .enum, you can't understand because you only use D, and you stuck in pre-C era Anyone else have an opinion on this PR? |
Perhaps Anyways, can someone share its opinion on the PR? |
With the attitude you expressed in #16777 (comment), I'm not particularly motivated to share my opinion lest I also get treated that way. Walter already expressed a rejection (in the short-term at least), and there's no one higher to appeal to in this case, so that settles it in my book. If you want library users to include your library with
/// Your D library
version(X)
import cfile_with_X;
else version(Y)
import cfile_with_Y; /// cfile_with_x
#define ENABLE_X
#include "cfile.c" /// cfile_with_y
#define ENABLE_Y
#include "cfile.c"
// config.d
version (X)
enum v = 'X';
else version(Y)
enum v = 'Y'; // c file
__import config;
void main(void)
{
if (v == 'X')
X();
else if (v == 'Y')
Y();
} See: https://dlang.org/spec/importc.html#__import Yes, I know you probably hate this. You don't have to tell me how ugly it looks compared to your preferred solution. But given that this PR is rejected and you care about a short |
My goal is not to get anyone sympathy, i just want to improve things, i don't even want to send PRs, i'd prefer work on my project, but who else will do it? I use a language as a tool, at some point if the tool is no longer useful to me, i'll pick an other one and move on
This PR was not rejected, Walter expressed his opposition, big difference, walter didn't try other languages to see how they have solved that kind of issues Let's consult the rest of the community instead of acting like a dictator's servant In Zig one can define things right when importing a C file, it can even be manipulated with comptime const c = @cImport({
if (X) {
@cDefine("ENABLE_X");
}
@cInclude("lib.h");
}); Can we at least have something similar so i don't have to duplicate my build scripts or create bunch of extra files? version(X) @cDefine("ENABLE_X")
import lib; |
It's time for me to quit |
This allows better integration of C files into a project
ping @WalterBright