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

language 'C++' overrides language 'C++11' #770

Closed
tvandijck opened this issue May 1, 2017 · 20 comments
Closed

language 'C++' overrides language 'C++11' #770

tvandijck opened this issue May 1, 2017 · 20 comments

Comments

@tvandijck
Copy link
Contributor

previously I could do this:

workspace 'X'
    flags { 'C++11' }

    project 'Y'
        language 'C++'

and all my code would be compiled with C++11 standards enabled..

workspace 'X'
    language 'C++11'

    project 'Y'
        language 'C++'

doesn't work that way... project Y now uses whatever is the default, which is OK, since that is what is specified... but for SC2/Heroes, I now have to basically fix up 98 project lua scripts. I know I did the work, and we had long discussions about this particular refactor, but it's a rather inconvenient API the way we made this work..

@samsinsane
Copy link
Member

I can appreciate the pain you're going through, I'll be going through this soon enough. I don't think ignoring C++ will work though, it's a good way to not have anything emit, this is good if you need to emit a standard that Premake doesn't have. An end user shouldn't need to know how to add a new standard into Premake for all of the actions, just how to use filter and buildoptions.

I do think we need a better solution though, because the above is kind of horrifying.

@tvandijck
Copy link
Contributor Author

it's a good way to not have anything emit,

Just for what it is worth, premake sets the default language at startup to C++.
I tried removing that line, and then any project that doesn't set it explicitly errors during validation. Which is kind of interesting, as it lead some of the projects to suddenly use CompileAs set to C++, whereas previously they didn't, and hence they started failing...

@pdoane
Copy link

pdoane commented May 2, 2017

I would like to find a solution to the problem that keeps something like the current syntax. I maintain several projects that support multiple C++ language revisions and this change simplifies usage a lot. Instead of this:

    if _OPTIONS["language"] == "C++11" then
        flags { "C++11" }
    elseif _OPTIONS["language"] == "C++14" then
        flags { "C++14" }
    elseif _OPTIONS["language"] == "C++17" then
        flags { "C++17" }   -- future proofing
    end

I can now do:

    language ( _OPTIONS["language"] )

Also, if I have a mixed solution where some projects need different dialects, the script changes from:

        removeflags { "C++11" }
        removeflags { "C++17" }
        flags { "C++14" } 

to

        language "C++14"

So I think there needs to be some way to control the '-std=xxx' option with a single API call which this is handling great... the problem is just in the backwards compatibility.

Ideally for me language 'C++' would recognize that language 'C++11' is more specific and ignore it.

@samsinsane
Copy link
Member

Ideally for me language 'C++' would recognize that language 'C++11' is more specific and ignore it.

As I mentioned above, this would result in Premake only supporting language standards that it is aware of. We have a project (there might be more) that uses c++0x while our other projects use C++11, so this kind of change absolutely does not work for us. In my opinion, there always needs to be a way to tell Premake to not emit anything at all.

@starkos
Copy link
Member

starkos commented May 3, 2017

Just for what it is worth, premake sets the default language at startup to C++. I tried removing that line, and then any project that doesn't set it explicitly errors during validation.

What if you remove the line that sets C++ to be the default at project creation, but then set to C++ if you find an uninitialized project during validation? Even if only as a temporary solution until a better fix is sorted?

@tvandijck
Copy link
Contributor Author

What if you remove the line that sets C++ to be the default at project creation, but then set to C++ if you find an uninitialized project during validation? Even if only as a temporary solution until a better fix is sorted?

There would practically be no difference... since you can override it with a new string anyway.

@tvandijck
Copy link
Contributor Author

We have a project (there might be more) that uses c++0x while our other projects use C++11, so this kind of change absolutely does not work for us

I'm not sure what wouldn't work here?

workspace 'foobar'
    language 'C++11'

   project 'A'
       language 'C++0x'

  project 'B'
      language 'C++'

would work fine with the PR I submitted? if you specify a version, it overrides C++, if you don't specify a version, it leaves the version as it was... so in this example, project A will be compiled with C++0x, and project B would be C++11... interestingly in the old 'flags' situation, you actually had to do the following:

workspace 'foobar'
    language 'C++'
    flags { 'C++11' }

   project 'A'
       language 'C++'
       removeflags { 'C++11' }
       compileoptions { '-std=c++0x' }

  project 'B'
      language 'C++'

so in all honestly, I think with the merge of the language and flags for this particular API, as well as the PR that adds the override behavior to the language API, makes this a fairly clean and workable alternative... but I absolutely like to understand all use-cases before we proceed with merging anything.

@samsinsane
Copy link
Member

The problem is that C++0x is not a supported standard in Premake. Even if it was, my argument is that I should be able to tell Premake that this is just a C++ project, don't emit any standards, I need something that you don't know about (yet). Hacking or updating Premake are not always valid options when needing things like this.

@tvandijck
Copy link
Contributor Author

my argument is that I should be able to tell Premake that this is just a C++ project, don't emit any standards

Which is currently not possible. If I specify "flags {"C++14"}, in the workspace, and you don't actively remove those flags from your project.. then you get C++14 as a standard, and that is exactly the behavior I'd like to maintain, but again I'm open to alternative suggestions.

Basically the only way to guarantee that a project is C++ only, with no standards emitted would be to write:

project 'a'
     removeflags { "C++11", "C++14" }
     removecompileoptions { "-std=c++0x", "-std=c++11", "-std=c++0y", "-std=c++14", "-std=c++0z", etc }

@tvandijck
Copy link
Contributor Author

tvandijck commented May 3, 2017

Thinking about the dialect API a bit further, I think it would have to look like this:

language 'C++'
dialect { ['C++'] = 'C++11' }

because that way I could write:

workspace 'foobar'
    dialect { 
        ['C'] = 'C99', 
        ['C++'] = 'C++11' 
    }

    project 'x'
        language 'C++'

    project 'y'
        language 'C'

and get the behavior I expect with the workspace overloading the behavior of anything below.
a project that wanted to enforce 'set no standard', could then do

    dialect { 
        ['C++'] = nil
    }

@samsinsane
Copy link
Member

Or removedialect { 'C++' } which would be a bit more consistent than using nil? Other than that, it sounds like a pretty good solution!

@tvandijck
Copy link
Contributor Author

I'll see if I can whip up a PR for that, to see how that would look...

@starkos
Copy link
Member

starkos commented May 3, 2017

I'll try to give this proper think if/when I can, but wanted to mention that APIs like this are very restricting, and really require an entirely different system for working with them:

    dialect { 
        ['C'] = 'C99', 
        ['C++'] = 'C++11' 
    }

I've been trying to convert the few APIs that use that approach to "flat" calls like:

cdialiect('C99')

(I'm not suggesting that's a good API name).

It would be a Really Great Thing if you didn't do that.

(Addendum: the complication is really when trying to validate the value side of the key-value assignment. There can also be ordering issues, but that wouldn't be an problem for this specific use.)

@tvandijck
Copy link
Contributor Author

Yeah the "keyed" kind is certainly 'complicated'.

  1. there is no 'remove' API for it, and I just tried to implement it, and basically just end up in a refactor all the way down to rewritting premake. I then noticed this comment, so someone already clearly had that issue before:
		-- TODO This comment is not completely valid anymore
		-- This needs work; right now it is hardcoded to only work for lists.
		-- To support removing from keyed collections, I first need to figure
		-- out how to move the wildcard():lower() bit into the value
		-- processing call chain (i.e. that should happen somewhere inside of
		-- the field.remove() call). And then I will probably need to add
		-- another accessor to actually do the removing, which right now is
		-- hardcoded inside of _fetchMerged(). Oh, and some of the logic in
		-- api.remove() needs to get pushed down to here (or field).
  1. lua just in general doesn't like tables with 'nil',
  dialect { 
        ['C++'] = nil
    }

just ends up doing absolutely nothing..
you have to write:

  dialect { 
        ['C++'] = false
    }

to get any kind of meaningful effect.

I really don't want to create

cdialect('C99')
cppdialect('C++11')

although at this point, that seems to be the only alternative that would actually work.

It would be a Really Great Thing if you didn't do that.

@starkos what in particular?

@tvandijck
Copy link
Contributor Author

I'm going to give up on the dialect approach... not going to work with the 'keyed' type...
work is here in case someone wants to see where this fails:
Blizzard@3ebd0fc

I added a bunch of tests to try and figure out a bunch use-cases, two fail..

@tvandijck
Copy link
Contributor Author

best I can come up with, considering all the constraints:
#776

@samsinsane
Copy link
Member

In lieu of removedialect, would supporting dialect { ["C++"] = "" } in the interim suffice? It wouldn't be too hard to deprecate this in the future, and it I imagine it would be relatively simple to support in most places?

-- in tools
local value = toolset.dialects[cfg.dialect[cfg.language]] -- or something
if value
  -- emit option
end

-- in vs
if table.contains(p.languages.all, cfg.dialect)
  -- emit option
end

Maybe I've completely misunderstood the problem?

@tvandijck
Copy link
Contributor Author

Yeah, I just went with the cdialect & cppdialect option instead... the "keyed" kind is actually really difficult to use correctly, and to validate correctly... for one, all dialects have to be in the 'allowed' table, regardless of whether they are C or C++, so you can write dialect { ["C"] = "C++11" }, without it failing, so you need to add code in premake.validate to detect errors like that... there is no validation of keys at all, so dialect { ["foo"] = "bar" } doesn't fail either... I did add something for that, but it breaks in a few places.

all in all... too complicated, and just not very good..
new PR here: #776, I think it's overall just better and more consistent.

@pdoane
Copy link

pdoane commented May 4, 2017

I'll do some testing with the cdialect/cppdialect approach to see if that works. While the earlier language approach was a breaking change, it does match my needs really well.

Some kind of solution is needed here - e.g. doing this just doesn't scale:

project 'a'
removeflags { "C++11", "C++14" }
removecompileoptions { "-std=c++0x", "-std=c++11", "-std=c++0y", "-std=c++14", "-std=c++0z", etc }
-- now set what you actually want here

And the issue is just getting more complicated with Visual Studio jumping into the game too.

@tvandijck
Copy link
Contributor Author

Closing, since we have the cppdialect & cdialect options for now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants