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

WIP: For loops #480

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

WIP: For loops #480

wants to merge 1 commit into from

Conversation

rpavlik
Copy link
Contributor

@rpavlik rpavlik commented Sep 10, 2019

Pulled out of #460 because it crashed on Windows and consumed all available space on Linux using GCC + sanitizers.

Some found by clang-tidy. Some of these reduce unnecessary copies
or ref-count adjustments.
@whitequark
Copy link
Contributor

Do you think you could revisit this PR? The changes all look nice and reasonable to me, modulo some style nits, and it'd be great to merge this.

@rpavlik
Copy link
Contributor Author

rpavlik commented May 10, 2020

Ok, I'll take a peek at this when I get a chance. The description of it worries me, but maybe I'll see my problem easier with fresh eyes. (I didn't remember that I had any outstanding unmerged for loop code, I thought only the Eigen proof of concept was left.)

Any hint on what the style nits are? Do you want all loop bodies, etc. bracketed? (I normally do that, so seeing a few not like that was the only style thing that jumped out at me rereading this on my phone, at least.). Should be easy enough for me to bisect which change causes problems since most of those diff hunks look independent.

@ruevs
Copy link
Member

ruevs commented May 11, 2020

If it turns out the "nitpicks" are purely formatting you could use clang-format:
https://github.com/solvespace/solvespace/blob/master/CONTRIBUTING.md#coding-style

@whitequark
Copy link
Contributor

Any hint on what the style nits are?

Oh sorry, I forgot to actually mention. It's just one thing, in a few places you use a reference when iterating then immediately convert it to a pointer and access through that (extraneous) pointer.

@rpavlik
Copy link
Contributor Author

rpavlik commented May 11, 2020

Ah, that was to perform minimum changes. No problem, I can fix that.

@phkahler
Copy link
Member

@rpavlik Any word on this PR or #475 or #460 ? It's fine if you still want to make changes, or have any of them merged. But if they're obsolete I'd like to close them. Thanks!

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