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

[too many to list] Index macros directly from definitions #7525

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

AlisdairM
Copy link
Contributor

This commit uses the macro to index macro definitions directly from the header synopses where they are defined, thus removing the need for a separate indexing macro that can get out of sync.

This commit does NOT address function-like macros, or macros that use placemarker syntax in their naming.

White this PR might have been more easily reviewed as one commit for each file, it seems better to apply the changes consistently. all at once, so that any future edits have a consistent pattern to follow.

@jensmaurer
Copy link
Member

jensmaurer commented Dec 30, 2024

White this PR might have been more easily reviewed as one commit for each file, it seems better to apply the changes consistently. all at once, ...

That is a non-sequitur. One commit for each file does not mean one pull request for each file, i.e. github provides for an environment where multiple commits can be naturally applied in an all-or-nothing fashion by grouping them into a single pull request.

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.

I support the general approach, but please split at least the large changes into separate commits (NOT separate pull requests), with a proper section label in each commit description.

source/future.tex Outdated Show resolved Hide resolved
source/diagnostics.tex Outdated Show resolved Hide resolved
@jensmaurer jensmaurer added the changes requested Changes to the wording or approach have been requested and not yet applied. label Dec 30, 2024
@AlisdairM AlisdairM force-pushed the index_macros_from_definitions branch from 1dab1c5 to db99309 Compare December 31, 2024 11:12
@AlisdairM
Copy link
Contributor Author

That is a non-sequitur. One commit for each file does not mean one pull request for each file, i.e. github provides for an environment where multiple commits can be naturally applied in an all-or-nothing fashion by grouping them into a single pull request.

I have broken up the PR into many more specific commits.

@AlisdairM
Copy link
Contributor Author

All the requested changes should now be applied. Ready for re-review.

@jensmaurer jensmaurer removed the changes requested Changes to the wording or approach have been requested and not yet applied. label Dec 31, 2024
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 typo-fix this commit description:

[support] Better index the many macros in the [support] clauses

"Not that there are additional edits in other clauses to ensure that
macrods are consistently indexed using only through \libmacro to
support future evolution of that LaTeX macro."

Not -> Note

macrods -> macros

"using only through" feels non-English to me.

source/compatibility.tex Outdated Show resolved Hide resolved
source/macros.tex Outdated Show resolved Hide resolved
source/macros.tex Outdated Show resolved Hide resolved
@AlisdairM AlisdairM force-pushed the index_macros_from_definitions branch from db99309 to 00be37c Compare January 2, 2025 02:53
@AlisdairM
Copy link
Contributor Author

Again, latest push should have addressed all the open change requests.

source/support.tex Outdated Show resolved Hide resolved
@wg21bot wg21bot added the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Jan 2, 2025
@jensmaurer
Copy link
Member

I've talked to @tkoeppe, and we agree that we'd prefer to see one commit per header. This means we can start the title of the respective commit with "[cerrno.syn]" or similar, which gives a nice scope to each commit.

I understand that sometimes, macros are referenced from various places (e.g. va_start), and you want to deal with all macros from a given header uniformly. I'd suggest you factor the "obvious" candidates such as [cerrno.syn] (that have no references to those macros elsewhere) as a first iteration step, and we can have a deeper look at the rest later.

@AlisdairM
Copy link
Contributor Author

Could you confirm whether I hit all uses of a macro in multiple headers in a single commit, when resolving a header synopsis, or revisit with a separate commit or set of commits? You mention <cerrno> as having no references, but it is actually the most referenced single header due to cross-references to errno. Of course, there is every chance that I am adding too many errno cross-references too.

Could I land a smaller patch that adds the library macros, and just a few headers to get started? I expect to revisit this PR on Sunday otherwise.

@jensmaurer
Copy link
Member

We'd hope there would be some headers with macros that are not mentioned elsewhere in the standard. If <cerrno> is not such a header, fine, let's leave for later.

So, the ask is to start with those headers whose macros are not mentioned elsewhere, and have commits with label [cblah.syn] and changes for that header only, as a starter. We can then have a look at the rest; maybe it's best to summarize those under [support] or so.

The immediate idea is to support using the new macros directly
in header synopses when defining each library macro.  This will
ensure that no macros are accidentally not indexed.

A follow-up plan is that this separation of library macros will
make it easier to create a separate index of macros, or apply
other macro-specific renderings, in the future.  To this end,
all indexed uses of a macro, not just those in header files,
should be replaced by use of these new macros.  Similarly,
these LaTeX macros can be used in-place in regular text to
index cross-references where standard library macros are used
throughout the standard.
@AlisdairM AlisdairM force-pushed the index_macros_from_definitions branch from 00be37c to 06850ff Compare January 8, 2025 21:39
@AlisdairM
Copy link
Contributor Author

I have reapplied the changes one header at a time, with the exception of the Annex D updates that I applied as one commit at the end; I have not applied any changes to [support] yet as that has many headers to update; I have removed all additional indexing of macros from headers that are not the immediate subject of a commit to the same .tex file.

I have also rebased onto main so this PR should apply cleanly.

@jensmaurer jensmaurer removed the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Jan 11, 2025
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.

Ok, this looks like a very nice first step to me.

Could you please update the commit titles from "Better index the macros in this header" to "Improve indexing for macros" ? This betters the English, I think. :-)

Since this is a no-op as far as the actual text contents of the standard is concerned, I'd apply this soon afterwards. @tkoeppe , FYI.

@AlisdairM AlisdairM force-pushed the index_macros_from_definitions branch from 06850ff to da9d6dd Compare January 14, 2025 13:45
@AlisdairM
Copy link
Contributor Author

Commit messages should be updated now.

@jensmaurer
Copy link
Member

@tkoeppe , this should be ready now.

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.

4 participants