Add omitframepointer API #1043
Add omitframepointer API #1043
Conversation
| scope = "config", | ||
| kind = "string", | ||
| allowed = { | ||
| "Default", |
There was a problem hiding this comment.
This is an "unset". I was unsure that it is a value we want and kinda forgot.
So I see some APIs that have the option to set Default and others not. What is your take on this?
There was a problem hiding this comment.
As I mentioned in the other comment chain, I've encountered the need for an "unset" value. So I'm 100% in favour of not having to hack around in my Premake scripts just to "unset" something.
src/_premake_init.lua
Outdated
| api.deprecateValue("flags", "NoFramePointer", 'Use `framepointer "Off"` instead.', | ||
| function(value) | ||
| omitframepointer("On") | ||
| end) |
There was a problem hiding this comment.
I believe there's two functions that can be defined, one for flags { "NoFramePointer" } and one for removeflags { "NoFramePointer" }. See above deprecation for StaticRuntime.
There was a problem hiding this comment.
Well, I commented too soon.
I suppose this is the reason for a Default value?
There was a problem hiding this comment.
There's this, but sometimes you just want to unset the value. Either it's causing you compile issues (compiler bugs, etc), or you're using a compiler that doesn't fully support the options. I've encountered the latter when people have assumed that Clang accepts all GCC flags, but it actually doesn't.
There was a problem hiding this comment.
I see.
The problem that I see is that an API like -fvisibility has a Default value that does something (ie -fvisibility=default) so I feel that premake should take care of this at a higher level than per-API. Maybe something like the removeflags API? Or allowing user to set nil?
There was a problem hiding this comment.
You raise a good point, @starkos @tvandijck @TurkeyMan thoughts on extending the API system to "unset" an API? I like the idea of using the remove prefix, it's consistent with the list-based APIs.
There was a problem hiding this comment.
I prefer a "Default" or "Off" to "remove", since it makes the behavior of that particular switch more explicit. Do you actually need to emit -fvisibility=default? Isn't that the same as not including the flag at all?
There was a problem hiding this comment.
Well I can't find anything about a difference between -fvisibility=default and not including the flag so I guess I could no output anything for visibility "Default".
I still stand against using "Off" as an equivalent to remove. In the UnsignedChar API I added, unsignedchar "Off" output -fno-unsigned-char and it is necessary as some platforms have unsigned char by default.
As for "Default", it does not sit well with me but I can't find a case that would cause an issue.
There was a problem hiding this comment.
Also if "Off" is not used as a "Default", it means that boolean APIs must either be changed to string or that the internal workings of boolean APIs accept "Default" as input.
Honestly, I'd rather that all APIs accept "nil" that would simplify things a lot.
Should I open an issue to discuss this? It concerns premake as a whole and not just this PR.
src/tools/gcc.lua
Outdated
| @@ -96,6 +95,10 @@ | |||
| }, | |||
| symbols = { | |||
| On = "-g" | |||
| }, | |||
| omitframepointer = { | |||
There was a problem hiding this comment.
Does Clang automatically pull this in? I feel like it doesn't, but it definitely should since the flag was pulled in.
|
I added a test for "Default" and will open an issue to discuss what to do for un-setting values. |
…er Default in vstudio
|
I replaced the references to NoFramePointer flag with the new API. I think that's all to do here. I will wait for this to be merged before resolving the conflicts on my other PRs as it would result with more of the same conflicts. |
|
The linux fail doesn't seem to be from this PR and I can't reproduce it on my linux. Any idea? |
No description provided.