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

[basic.align] Move the Alignment clause adjacent to the Object model #7323

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AlisdairM
Copy link
Contributor

@AlisdairM AlisdairM commented Oct 19, 2024

[Edit]
Simplified to move just the Alignment subclause.

[Original, for reference]
Reorder the subclauses of [basic.memobj], Memory and objects, into a logical progression where each clause augments the one before.

  • Memory model specifies the notion of memory for C++
  • Object model specifies objects, and how they occupy memory
  • Alignment restricts where objects can reside in memory
  • Storage duration describes how objects acquire well-aligned memory
  • Intermediate and erroneous values specifies the initial contents of acquired memory
  • Lifetime describes the validity of objects within their acquired storage
  • Temporary objects have a lifetime of their own.

@jensmaurer
Copy link
Member

I don't think micro-optimizing various small-ish subclauses in the standard is a good way to spend our time.

That said, I disagree with the suggested ordering. In particular, storage duration is more about when the destructor is automatically called in various circumstances (or not at all), and not so much about the actual duration of the storage. I think we first should know about lifetime in general before we dig into that.

Further, indeterminate / erroneous values is mostly discussing what happens within the lifetime of an object when no initialization has happened, and not about storage without in-lifetime objects.

In short, I don't find the current ordering "obviously wrong".

@AlisdairM
Copy link
Contributor Author

How would you feel about moving just the alignment clause to follow Object model?

@jensmaurer
Copy link
Member

How would you feel about moving just the alignment clause to follow Object model?

I'm ok with that. Alignment is an aspect of the object model, I think.

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.

Please rebase; there's a stray commit here.

@AlisdairM
Copy link
Contributor Author

I believe the stay commit is a consequence of rebasing?

@jensmaurer
Copy link
Member

I hope that isn't true. I just merged the change that is your second commit to "main", so I'm a bit surprised to see it pop up here (again).

@AlisdairM
Copy link
Contributor Author

I just rebased, and the commit ID of that intermediate commit has changed. I rebased again and that should have fixed it. I have no idea what the commit ID would change, but I guess that is what I get for jumping on a change that just landed?

@tkoeppe
Copy link
Contributor

tkoeppe commented Oct 19, 2024

Please simplify the commit description, too (or at least during squashing).

@AlisdairM
Copy link
Contributor Author

The commit description has been greatly simplified. I retained the original description above only in the GitHub conversation, so that Jens's comments retained their context.

@AlisdairM
Copy link
Contributor Author

Oh, unless you mean the one-line description, I can fix that too.

Alignment places additional restrictions on memory in the object model
for any storage that is acquired, so belongs here.
@AlisdairM AlisdairM changed the title [basic.memobj] Provide a logic progression of subclauses [basic.align] Move the Alignment clause adjacent to the Object model Oct 19, 2024
@AlisdairM
Copy link
Contributor Author

I hope that the PR is now in a simple and clean state, ready for landing.

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