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

Refactor language flags to go through the language API. #686

Merged
merged 1 commit into from
Apr 13, 2017

Conversation

tvandijck
Copy link
Contributor

So you can use 'language "C++11"' instead of 'flags { "C++11" }'

@@ -366,7 +366,10 @@
end

function m.completion(cfg)
_p(3, '<Completion EnableCpp11="%s" EnableCpp14="%s">', iif(cfg.flags["C++11"], "yes", "no"), iif(cfg.flags["C++14"], "yes", "no"))
local cpp11 = (cfg.language == 'gnu++11') or (cfg.language == 'C++11') or cfg.flags["C++11"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be a function, to allow others to add other language extensions like GNU++11. For example, VS2015 Update 3 (probably VS2017 too) allows you to specify "C++Latest", we should have support for this to translate to C++17. When C++20 starts happening, it will need to map to that, and changing it everywhere is going to be time consuming.

settings['GCC_C_LANGUAGE_STANDARD'] = 'gnu99'
if (p.languages.isc(cfg.language)) then
if cfg.language ~= "C" then
settings['GCC_C_LANGUAGE_STANDARD'] = cfg.language:lower()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on previous comment, this will need to handle things like "C++Latest" (but a C equivalent), so it's probably best this entire block for adding GCC_C_LANGUAGE_STANDARD be a function instead, same for the C++ block.

api.deprecateValue("flags", "C++14", 'Use `language "C++14"` instead', function(value) end, function(value) end)
api.deprecateValue("flags", "C90", 'Use `language "C90"` instead', function(value) end, function(value) end)
api.deprecateValue("flags", "C99", 'Use `language "C99"` instead', function(value) end, function(value) end)
api.deprecateValue("flags", "C11", 'Use `language "C11"` instead', function(value) end, function(value) end)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these all set the language?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@starkos suggested we don't do that in his last comment on this thread: #659

We could though, I don't think with the changes I made it would break anything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, shouldn't break anything. Just thought it would be easier to handle both the language and the flags at the point of application, which you did.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the suggestion was ultimately the correct one:

	api.deprecateValue("flags", "C++11", 'Use `language "C++11"` instead', 
	function(value) 
		language "C++11"
	end, 
	function(value) 
		<what to do here> ???
	end)

I've tried a few things,

removelanguage "C++11"
or
language "C++"

but they all break...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a Default value to language? I'm picturing a situation like this:

project "NiceProject"
  language "C++14"
  files { "**.h", "**.c" }

  filter { "files:RealCFiles/**.c" }
    language "Default"

Would the language value flow down regardless of extension? Assuming it would, the above might be desired functionality?

Copy link
Member

@starkos starkos Feb 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that's right, forgot about that part. We need the ability to set the language (or standard, or whatever) for each "dialect".

clanguage "C99"
cpplanguage "C++14"

We can still have the generic language, which is used if a more specific value hasn't been set. And of course this needs to extend to any supported language:

dlanguage "SafeD"

So yeah…there's that. Not sure if there is a more elegant/transparent way to express this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could just do:

language "C++14"
filter { "files:*.c" }
    language "C99"

but that said, if we went the "dialect" route, I would certainly prefer to actually use that name.
So either:

dialect {
    cpp = "C++14"
    c = "C90",
    d = "D3.4",  -- if such a thing was a thing.
}

or as suggested:

cdialect 'C90'
cppdialect 'C++14'
ddialect 'D3.4"

I'm somewhat indifferent to the approach, I think both have merit..

You guys want me to prototype that instead??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could just do:

language "C++14"
filter { "files:*.c" }
    language "C99"

You're quite right, I was overthinking. It is really only an issue in multi-language (or dialect) projects, and this suggestion is the right way to go.

@tvandijck
Copy link
Contributor Author

Can we merge this? or is there more work left?

@samsinsane
Copy link
Member

@tvandijck There's a few places where you remove checks for iscpp, these should probably stay. In some cases, there's more code in the function and by removing the check you're causing module developers to copy code from core. vs2010.generateProject is a good example of this, the D module will end up generating a .vcxproj and .visualdproj if you select any VS action for a D project.

@tvandijck
Copy link
Contributor Author

by removing the check you're causing module developers to copy code from core.

not sure I understand this...

the D module will end up generating a .vcxproj and .visualdproj if you select any VS action for a D project.

I see that one, but the check was wrong... vcxproj is for both C and C++ projects.
I guess I can add a check to check for either.. I'm just wondering where this is needed exactly.

In that same boat, I then also think the "project.isnative" method is confusing, because there really isn't such a thing...

@samsinsane
Copy link
Member

Well, ideally, we'd change the functions so that generic and specific code don't exist in the same function. Maybe for now, the best move would be to check for C and C++ and then, eventually, we can move the generic code out. Thoughts?

@neico
Copy link
Contributor

neico commented Mar 6, 2017

While you're at it, add the new switches for Visual Studio as well?
See: https://blogs.msdn.microsoft.com/vcblog/2016/06/07/standards-version-switches-in-the-compiler/

["C++"] = "/std:c++latest",
["C++14"] = "/std:c++14"

imo it's a horrible thing that MS did there, especially since "some" of the standards aren't locked behind the switches, I really hope they'll sort this out in the future and maybe add switches for the older standards as well...

/permissive- will also need a place somewhere, but I'm not sure where that'd be (see https://blogs.msdn.microsoft.com/vcblog/2016/11/16/permissive-switch/)

@tvandijck
Copy link
Contributor Author

@samsinsane what changes do we still need here? other then what @neico suggests to add?

Copy link
Member

@samsinsane samsinsane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have this feeling that this will break at least one of my modules, but I'm not sure which one or where, and I'm just not that bothered to find out exactly how. Something else has probably already broken them, so let's move forward and I'll submit a PR if something needs to be fixed.

So you can use 'language "C++11"' instead of 'flags { "C++11" }'
@tvandijck
Copy link
Contributor Author

I rebased this, and made some more modifications to solve some of the comments @samsinsane made about me removing some of the required "iscpp" calls... I think I got them all correct now, but please review.

@tvandijck
Copy link
Contributor Author

@neico this commit just modifies the flag/api.... I've not actually implemented usage of it inside of the Visual Studio action at all... Feel free to submit further PR's on top of this change for that once this is merged (or even against this PR).

end
end
else
settings['GCC_C_LANGUAGE_STANDARD'] = 'gnu99'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this map to one of the C11 standards when the language is C++17? I believe C++17 is based on C11, rather than C99, or is it the plan for the next C++ standard to be on C11?

@@ -51,16 +51,14 @@
if #user > 0 then
p.generate(prj, ".csproj.user", function() p.outln(user) end)
end

elseif premake.project.iscpp(prj) then
else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, this should probably have the check for C/C++, but I'm happy to ignore this since it's for VS2005 and VS2008, and VS2008 will be EOL in a little under a year.

@starkos
Copy link
Member

starkos commented Apr 12, 2017

Are we good to merge? I think it looks pretty good, major props to @tvandijck for putting it together!

@samsinsane samsinsane merged commit 9572876 into premake:master Apr 13, 2017
@ghost
Copy link

ghost commented Apr 13, 2017

Looks like the line endings in src/actions/vstudio/vs2010_vcxproj.lua have changed, so I'm getting a huge merge conflict with #734. I feel like we should revert this and fix the line endings here.

@samsinsane
Copy link
Member

samsinsane commented Apr 13, 2017

I've noticed that line ending issue, I'm fairly certain it was already in master from somewhere else. My apologies, I was talking about the line ending issue that existed prior to these changes. Technically, this PR is actually fixing the line endings, they were actually broken in #732. For some weird reason git won't show that anything changed, even though I'm looking at a diff between 4670158 and f3dae94 and it couldn't be more obvious that the line endings are different.

@ghost
Copy link

ghost commented Apr 14, 2017

I fixed this in my branch by running git rebase master -s recursive -X ignore-space-change and then manually fixing up line endings in every commit that touches src/actions/vstudio/vs2010_vcxproj.lua.

I think git provides facilities to automatically normalize line endings on push with a .gitattributes file, so we might want to look into that at some point.

@neico
Copy link
Contributor

neico commented Apr 14, 2017

@aleksijuvani .gitattributes and .editorconfig together can avoid any of those.

if you throw in .clang-format you get even actual style guidelines enforced.

So yeah, imo any git project should at least use the first 2 mentioned files.

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

Successfully merging this pull request may close these issues.

4 participants