premake / premake-core

Premake
https://premake.github.io/
BSD 3-Clause "New" or "Revised" License
3.22k stars 620 forks source link

Un-setting a previously set API #1050

Open tdesveauxPKFX opened 6 years ago

tdesveauxPKFX commented 6 years ago

As was discussed in #1043, it can be useful to be able to "unset" an API.

Currently, some API allow "Default" as input to unset them but not all API implement this.

To quote myself from the PR,

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.

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.

So, use "Default" and/or "Off", use "nil" ? And more importantly, for consistency, this behavior should be handled by premake for all API and not individually.

I'm up working on either solutions, just waiting for your input on this.

samsinsane commented 6 years ago

I prefer the idea of Default over Off or nil. Off is already used as part of the boolean APIs, and nil would be strange to use.

starkos commented 6 years ago

I, too, prefer an explicit value like "Default" over nil, though I'm open to other names for that value. I agree that "Off" isn't a good choice for this value.

This does limit the use of boolean fields. I'm not sure if that is a problem or not, or if we should provide an "unset" value for those?

tdesveauxPKFX commented 6 years ago

Well, this is the main issue I have with using "Default". If we go with it, we either:

Adding to that the list: type that would take "Default" and clear it's values.

I think an unsetting action should "stand out" and setting an API to a determined value isn't distinct enough IMO.

starkos commented 6 years ago

I think I would argue that a field that can be "on", "off", or "unset" isn't really a boolean, but a tri-state, and should be represented as a list of values instead. I think the boolean field type should really only be used for things can only be true or false.

TurkeyMan commented 6 years ago

@starkos +1... things with default state shouldn't be boolean. I'd love to see Default propagate to all api's. Lists are a bit awkward, because there's no strong definition of what 'default' means in that context, since it's not one value, it's a while bunch already. We haven't had this sort of complaint about lists, so let's just leave that for now ;)

samsinsane commented 6 years ago

Lists probably don't have the complaint because they have remove variants.

I completely disagree with the Boolean view. Compare these outputs:

There's a benefit to emitting nothing; Nothing won't cause an error or warning because certain versions of a compiler or tool doesn't support the API at all, or only supports the On value. Sometimes Premake has bugs and emits bad values, it's easier to set an API to default than to hack out the value from being emit at all until Premake is fixed.

While Premake being bugged isn't exactly a great reason, not everyone follows master and are "stuck" with whatever the latest alpha does. I've decided to do this at home and at work to avoid having to deal with modules breaking for whatever reason, previously it has consumed too much time fixing them.

tdesveauxPKFX commented 6 years ago

That was my main argument to have a Default added to boolean but now I tend to agree with @starkos and @TurkeyMan. I see the value in keeping the boolean kind to, well, a boolean and moving APIs that use both On and Off to string.

While on the boolean kind, I'd argue to have it accept only true and false only and not "On" and "Off" as well. It would help to create a real difference for APIs like in @samsinsane example.

In any case, I'm still in favor to have the "Default" value implemented for all APIs except list:* and boolean.