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

cppm extension support #13257

Merged
merged 1 commit into from
Jun 23, 2024
Merged

cppm extension support #13257

merged 1 commit into from
Jun 23, 2024

Conversation

TheHillBright
Copy link
Contributor

The cppm extension is used to signify a c++ source files meant to be treated as c++ modules. Both msvc and clang support this extension. gcc seems to have no support of any dedicated extension for c++ modules.

Currently, without this patch, meson fails saying no compiler can handle cppm files.

@TheHillBright
Copy link
Contributor Author

Unsure if changes in other places needed and testing needed as I'm unfamiliar with meson codebase. In any case, this PR can be the starter of supporting this new file extension as part of full c++20 module support. Related #5024.

@eli-schwartz
Copy link
Member

/cc @jpakkane

@rtgiskard
Copy link

https://clang.llvm.org/docs/StandardCPlusPlusModules.html#file-name-requirements

Please add .ccm too, at least it's the shortest extension correspond to .cc

@bruchar1 bruchar1 added the language:c++ Issues specific to c++ label Jun 12, 2024
@TheHillBright
Copy link
Contributor Author

@rtgiskard I did look at this page. But I decided to only add cppm since this is the most widely accepted extension for C++ modules. Other extensions listed in that page seem to have no other compiler supports (correct if wrong and I will then add accordingly). So, it might happen that later llvm devs decide to drop all of them except .cppm. This is also the reason why I don't add .ixx which seems only msvc supports. If any meson maintainer agrees to add other extensions into this PR though, I'll then do it.

Ping @eli-schwartz and @jpakkane due to inactivity for three weeks now.

Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

Seems like adding lang_suffixes should suffice here.

mesonbuild/compilers/compilers.py Outdated Show resolved Hide resolved
@eli-schwartz
Copy link
Member

So if I understand correctly you're just allowing C++ files to be named cppm (whether or not they are modules) for GCC. Does clang actually do something different based on file extension?

@TheHillBright
Copy link
Contributor Author

@eli-schwartz this PR is now fixed.
Your understanding is correct. When clang sees cppm files, it will treat them as c++20 modules. Since meson has not supported c++20 module dependency resolution for clang, this PR should be useful for cases like c++20 modules that implements only global module fragment and have not imported other modules.

@eli-schwartz
Copy link
Member

When backing out changes in a PR, please edit the original commit (using git commit --amend && git push --force). Do not make a change in one commit and then unmake that change in the next commit.

recognize the file extension `cppm` as c++ source file
@TheHillBright
Copy link
Contributor Author

@eli-schwartz Right and now done as per requested.

@jpakkane jpakkane merged commit c0ca35c into mesonbuild:master Jun 23, 2024
33 checks passed
@eli-schwartz
Copy link
Member

I would like to say that I strenuously object to this being merged with the commit message it currently has.

@TheHillBright TheHillBright deleted the patch-1 branch June 25, 2024 11:51
@TheHillBright
Copy link
Contributor Author

Since this is now merged into master, no further change can be done for me as an external casual contributor. For future PRs, @eli-schwartz would you like to point out the issues with the current commit message and/or how to improve further?

@eli-schwartz
Copy link
Member

It uses some "Angular JS commit style".

  • This is very inconsistent with the way the rest of the repository works as indicated by git log --oneline
  • Its purpose is to automatically generate a release changelog. This is semantically broken because meson has an existing mechanism for generating a release changelog which did not get used -- hence the commit message states a falsehood.
  • A good commit message is distinct from a good changelog entry. Producing changelogs from git commit logs is an anti-pattern; what matters to end users is not what matters to developers, and the optimal wording will necessarily differ. Using the same text for both, necessarily means that you are compromising the usefulness of at least one or the other, and potentially -- if you tried to find a middle ground -- compromising the usefulness of both.
    -For PRs with multiple commits, it's impossible to batch multiple commits into the same changelog entry. This discourages atomic commits and encourages the "JavaScript-style pull request" where new features, fixes for existing bugs, and fixup commits for previous commits in the same PR are batched together in PRs of a few hundred commits each, then squash merged.
  • For a single commit that changes one thing and "kills two birds with one stone", it is impossible to create two changelog entries.
  • This type of commit message is hard to read for people that don't regularly use it themselves. The standard commit message syntax can be easily read by anyone.

This PR originally used the standard commit message syntax, and changed to the Angular JS one in the last push.

@TheHillBright
Copy link
Contributor Author

Thanks @eli-schwartz for your detailed and good explanation. They're sensible and I'll stick to the commit message style I initially used for this PR for future PRs to meson then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language:c++ Issues specific to c++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants