Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[libc++] Install modules. #75741
[libc++] Install modules. #75741
Changes from all commits
e020e08
26f262d
9620519
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't consistent with
is-standard-library
above. Which is it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we please use valid python identifiers for keys? That way we can load the json into typed models.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or has that ship sailed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, P1689 uses kebab-case as well. Note that 18 is in rc3; I don't know how much "Python APIs are easier" is to make a release blocker versus "the file is not consistent with itself". Something for release managers to decide.
Note that there's nothing stopping a typed model from acting on it as-is because a function can transform these into identifier names (IME, the explicitness from things like
#[serde(rename = "…")]
are better than forcing the interchange format to bend to a specific language's ergonomics).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I agree that this probably isn't important enough to do anything about. And standardizing behind a specific format is more important.
But I also don't think it's unreasonable to prefer interacting with the data in a typed/schema-enforced way, and that treating keys as identifiers rather than strings is preferable to achieve that.
However, if they're no tooling that yet depends on the format, I think it's worth changing. Because consistency isn't everything, but it's nice to have, and we might lose the ability to have it later.
That said, this can land after the release, it's just a little uglier of a scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've no strong opinion about the naming. However as mentioned in the commit message the naming is based on a discussion in SG15 and as @mathstuf mentioned is used in P1689. (clang-scan-deps and CMake (and probably others too) have implemented this paper.) So IMO changing the naming should be discussed in SG15.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this. I should have used copy-paste programming ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem. Was very confused when "no such key" errors came up. Changed it to the one in the line it complained about…still errored. Finally saw the difference. Just glad I got around to testing this before the final release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw does that mean you're working on implementing this is CMake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep :) .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your commit message, please explain what the patch does. In particular, please explain the location where we install the
.cppm
files and the manifest file, and mention that the manifest file is supposed to live alongside thedylib
, and that there's a contract with Clang. Also mention the command-line that can be used to find the manifest file fromclang
.You referenced the SG15 discussion, but I would like us to capture the important points of that discussion in the commit message.