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

Initial C++20 module support for Visual Studio #1570

Merged
merged 3 commits into from
May 16, 2021
Merged

Initial C++20 module support for Visual Studio #1570

merged 3 commits into from
May 16, 2021

Conversation

hannes-harnisch
Copy link
Contributor

@hannes-harnisch hannes-harnisch commented Dec 16, 2020

What does this PR do?

Visual Studio's C++ compiler now has feature-complete support for C++20 modules (see also this link). Playing around with it, it appears to be relatively stable too, disregarding some IntelliSense syntax highlighting issues. It appears this is the first IDE and compiler toolchain to have any usable support for C++20 modules. Therefore, I think it would be useful to add at least some initial support for C++20 modules to Premake. For a better understanding of how C++20 modules work and what type of translation units they introduce, I strongly recommend this blog post and its follow-up posts.

This PR allows the use of 3 new values for Premake's "compileas" parameter. This parameter is used to set the value of the "Compile as" setting under the "Advanced" tab in the C/C++ source file properties, as dictated by the currently available options in Visual Studio. Given that the Visual Studio team has called their module implementation feature-complete, I doubt that the meaning of these options will change. At most, more options might be added in the future. The following values were added:

  • "Module": Used to designate the source file as a module interface unit for compilation.
  • "ModulePartition": Used to designate the source file as a module partition implementation unit for compilation. This type of module unit is also called a "module-internal partition" by Microsoft, though the C++20 standard does not give this type of module unit a specific name.
  • "HeaderUnit": Used to designate the source file as a header unit for compilation.

The PR also adds some unit tests, which of course ran fine. I also tested it in my personal project using a files filter and it works perfectly.

How does this PR change Premake's behavior?

No breaking changes and no existing behavior changes. It just adds more options for the "compileas" parameter.

Anything else we should know?

The 3 new values were given names that I think are closest to the terms used for the different module unit types in the C++20 standard (relevant section). Of course, the other compilers and IDEs might set up their module support somewhat differently. I still think it would be useful to extend the "compileas" parameter in this way so that it can at least be used in the only IDE currently supporting modules.

And of course, thank you Open Collective for creating Premake! You have saved me from the eldritch horrors of CMake and have made my C++ projects so much easier to set up.

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
  • Follow our coding conventions
  • Minimize the number of commits

You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!

@samsinsane
Copy link
Member

I really appreciate the effort here, however, I'm not sold on the API usage. We need to make sure whatever API we go with works for XCode and Makefiles.

@hannes-harnisch
Copy link
Contributor Author

hannes-harnisch commented Jan 10, 2021

I will update the PR appropriately when Xcode releases some support for modules. (Edit: I will not, I'm not familiar enough with Xcode. Also module support in Xcode might take a while to arrive anyway.

@starkos
Copy link
Member

starkos commented Apr 15, 2021

I really appreciate the effort here, however, I'm not sold on the API usage. We need to make sure whatever API we go with works for XCode and Makefiles.

@samsinsane Do you have another suggestion for the syntax?

This seems reasonable enough to me—thanks for taking the time to put it together and keep it up to date. The only change I would suggest is removing the "C++" from the values, so that we have the possibility of supporting similar, non-C++ module systems in the future.

@starkos starkos changed the title Initial C++20 module support for Visual Studio project files (the only IDE with support for them) Initial C++20 module support for Visual Studio Apr 15, 2021
@hannes-harnisch
Copy link
Contributor Author

hannes-harnisch commented May 3, 2021

I've changed the API usage to this, because at the time I opened the PR, I didn't realize exactly what Microsoft meant by "module internal partition". I falsely thought it referred to module partition implementation units, but it actually referred to module partitions themselves (aka module partition interface units).

I therefore shortened the strings for the new options. Based on starkos' suggestion, I also removed the C++ prefix. There the new options change from C++ModuleInterface to Module, C++ModulePartitionImplementation to ModulePartition and C++HeaderUnit to HeaderUnit respectively. They refer to module interface units, module partition interface units and header units respectively (the official terms in the C++20 standard).

@hannes-harnisch
Copy link
Contributor Author

It appears from comparing the current module systems in different compilers, that only interface units of modules and module partitions actually require special processing by the compiler and/or linker. The implementation counterparts for modules and module partitions (source files that don't have the export keyword in them anywhere) can be compiled as regular C++ source files.

Therefore, I don't think that the API that this PR adds will be fixated on the way Visual Studio does it. If anything, other IDEs might extract the kind of translation unit they are dealing with by parsing for the export keyword. Or possibly, they might add specific options for interface units and implementation units. I think support for such options can easily be added to Premake in the future and this PR should add some valuable support for C++20 in its current state.

Copy link
Member

@starkos starkos left a comment

Choose a reason for hiding this comment

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

Thanks for making those improvements, this looks good to me. @samsinsane Do these changes address your concerns?

@starkos starkos requested a review from samsinsane May 10, 2021 15:57
@starkos starkos merged commit 23f32c2 into premake:master May 16, 2021
@samsinsane
Copy link
Member

Sorry for taking so long to get back to this.

Do you have another suggestion for the syntax?

I don't - I don't know much about C++20 modules, I just knew that it was up to the implementers as to how they worked, and I was concerned that the VS way would be very different to everything else. In hindsight, this was probably unrealistic, sorry about that!

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.

3 participants