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

Define a new macro \caret and use it #1050

Merged
merged 1 commit into from Nov 17, 2016
Merged

Conversation

jensmaurer
Copy link
Member

Fixes #205.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 16, 2016

Did you run diffpdf on this, and did you check whether it copy-pastes as expected?

@jensmaurer
Copy link
Member Author

I did check selected instances that they copy-pasted as the ASCII caret character (verified with "od -x" on the resulting text file).
I've just run diffpdf, and it looks "good" (i.e. one caret character replaced by a slightly-different caret character), as expected.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 16, 2016

Great, thanks! We may hold off on merging until the motions are in; this is a somewhat big change and I'd prefer not to risk having to rebase a lot.

@jensmaurer
Copy link
Member Author

I've redone the patch because a space went missing in two places for bit_xor (clause 20). \caret{} as opposed to \caret seems to have fixed that.

@zygoloid
Copy link
Member

zygoloid commented Nov 17, 2016

Can we redefine \^ as \caret, or are we still using the original for something?

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 17, 2016

@zygoloid Messing with catcodes? Hmmm. I'm sure it can be done, but should it?

\catcode`^=\active
\newcommand{^}{\char`\^}

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 17, 2016

(And of course you'd need to make sure you don't change the catcode in math mode... I would be very hesitant to pursue this direction.)

@AlisdairM
Copy link
Contributor

This may have a race with the indexing-atomics pull request, which adds additional use of carets in the index, clearly using the older formulation for now.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 17, 2016

@AlisdairM: without fishing for a pun here, a lot of things will be racing on atomics. Please be patient and rebase a lot. I'm about to push yet another unrelated change.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 17, 2016

@jensmaurer: I'd say rebase and resolve this, and we'll merge this now. All the NB comments have been resolved, and we can fix up the motion applications if necessary.

@AlisdairM
Copy link
Contributor

@tkoeppe the atomics indexing branch should be fairly idempotent, as few changes tend to peek in there. This is the one race I know about, and happy to fix up atomics if this lands first. Note that I will not be available for the following three weeks, starting in around 24 hours, so it would be nice to land before then if possible - if not, we will catch up for next time, no problem.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 17, 2016

@jensmaurer: Ping, any chance you can get this PR resolved before Alisdair leaves?

@AlisdairM
Copy link
Contributor

Don't worry - I can see the PRs that just landed, that Thomas was referring to. I'll fix the atomics branch up again tonight, and should leave it in a state that is easy for someone else to pick up and land while I am gone - don't want to further distract /this/ review!

@jensmaurer
Copy link
Member Author

I didn't see there was a conflict meanwhile. Fixed.

@tkoeppe tkoeppe merged commit 7d68bf4 into cplusplus:master Nov 17, 2016
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