FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Handle operators 2 by rpavlik · Pull Request #448 · solvespace/solvespace · GitHub

Handle operators 2 - #448

Merged
whitequark merged 3 commits into
solvespace:masterfrom
rpavlik:handle-operators-2
Jul 10, 2019
Merged

whitequark merged 3 commits into
solvespace:masterfrom
rpavlik:handle-operators-2

Conversation

rpavlik commented Jul 9, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Alternate to #447 - doesn't have the operator bool stuff, but you at least get ==, !=, and <. Let's see what CI thinks.

whitequark commented Jul 9, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

@jwesthues Any strong opinions on this change? It adds a bit of type safety in that it's an error to compare unrelated handles, and it makes maps with handles as keys a bit nicer. And although I'm very much not fond of the way C++ manages to provide basic functionality, this implementation is not invasive and shouldn't cause us any problems like doing this with CRTP.

Overall, a lot of people cite SolveSpace's idiosyncratic C++ as a reason the codebase is hard to contribute to; and although some of those complaints are not that well founded, some others are. I personally think there's no especially good reason to not define equality on aggregate classes where it makes sense for all members.

whitequark mentioned this pull request Jul 9, 2019

Copy link
Copy Markdown
Member

Seems reasonable to me.

whitequark merged commit b2af9ce into solvespace:master Jul 10, 2019

Copy link
Copy Markdown
Contributor

Thanks @rpavlik!

Copy link
Copy Markdown
Member

Yes, thanks @rpavlik. I like the readability improvements this brings.

rpavlik deleted the handle-operators-2 branch July 11, 2019 19:18
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL