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

[headers] Fix note about importing library headers #4935

Merged
merged 1 commit into from Oct 20, 2021

Conversation

JohelEGP
Copy link
Contributor

@JohelEGP JohelEGP commented Sep 25, 2021

Importable C++ library headers can be imported as header units.
This currently says "as module units".
[module.unit]p1 says that
"A module unit is a translation unit that contains a module-declaration."
[module.import]p5 says that the synthetized header unit from an import <H>;
is a translation unit that contains no module-declaration.

@JohelEGP
Copy link
Contributor Author

The wording is wonky. You don't import a header unit. Can this be reworded in a way that is correct while remaining light in terminology? Maybe just drop the "as whatever unit" and let the example show the way.

@jensmaurer
Copy link
Member

Yes, just saying "... can be imported" sounds plausible.

@jensmaurer jensmaurer added the changes requested Changes to the wording or approach have been requested and not yet applied. label Sep 26, 2021
Copy link
Member

@jensmaurer jensmaurer left a comment

Choose a reason for hiding this comment

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

Looks good.

The commit description has a typo, and I think something shorter would work, too:

[headers] Fix note about importing library headers

Importable library headers are not module units, because they do not have a module-declaration.

Importable library headers are not module units,
because they do not have a module-declaration.
@JohelEGP
Copy link
Contributor Author

That is much better, especially the commit header message. Thank you.

@JohelEGP JohelEGP changed the title [headers] Fix terminology in note [headers] Fix note about importing library headers Sep 26, 2021
@tkoeppe
Copy link
Contributor

tkoeppe commented Sep 26, 2021

Looks right -- but out of curiosity, how did this wording get in in the first place? By which I mean, are we really restoring the original intent here? If so, feel free to merge.

@tkoeppe tkoeppe self-requested a review September 26, 2021 14:53
@jensmaurer jensmaurer removed the changes requested Changes to the wording or approach have been requested and not yet applied. label Sep 26, 2021
@jensmaurer
Copy link
Member

@tkoeppe , it's a note, so it's fair game for editorial fixes, I believe. Further, I think this just wants to say that you can import something where the module-name looks like a standard library header name.

"git blame" would help you with the archeology; I'm pretty sure it went in with the original modules paper.

@RedBeard0531
Copy link
Contributor

You don't import a header unit.

@JohelEGP what do you mean? Isn't that exactly what https://eel.is/c++draft/module.import#5 says you do?

I don't object to not saying header unit here if there are objections to saying it, but personally I think it is both helpful and correct.

@JohelEGP
Copy link
Contributor Author

As quoted in that link, the implementation synthesizes a header unit from import <H>; or import "H";. You can't specify a header unit in a module-import-declaration, but you can specify a header-name, from which a header unit will be synthesized.

@RedBeard0531
Copy link
Contributor

RedBeard0531 commented Sep 29, 2021

This is splitting very fine linguistic hairs, but I'd still view that as "importing the header as a header unit". That doesn't mean the header is a header unit, just that the import uses the header unit mechanism.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Sep 29, 2021

I agree with you.

That kind of wording is used elsewhere in the standard: https://eel.is/c++draft/intro#compliance.general-5, https://eel.is/c++draft/cpp.import (the title).

It is true that "you don't import a header unit". Actually, a header unit is imported. It's just that you don't name it in the module-import-declaration as a header-name.

It's also right to say that "importable C++ library headers can be imported as header units".

If it's OK, I'll revert to the line change of the original patch (keeping the improved commit message).

@tkoeppe tkoeppe merged commit c2439d3 into cplusplus:main Oct 20, 2021
@JohelEGP JohelEGP deleted the headers branch October 20, 2021 23:00
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.

None yet

4 participants