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

Handle operators #447

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
2 participants
@rpavlik
Copy link
Contributor

commented Jul 9, 2019

Handles seem like the kind of thing you should be able to compare, and the .v scattered throughout the codebase was an impediment to me learning it. Finally did a little modification I meant to do for a while: provide ==, !=, < and explicit operator bool (aka "use in if statements") operators for all the handle types. Easiest, least messy way was with a curiously-recurring-template-pattern base class so there is only one implementation of the operators, yet the handles are still type-safe (can't compare across handle types without using .v to get the uint32_t).

If it seems useless to you, no hard feelings if you just reject this (or drop parts of it, like the operator bool perhaps, since that seems less obviously-useful than the comparison operators). FWIW, I also have a version of this based on #428 (actually I devved it there first then rebased it onto master), at https://github.com/rpavlik/solvespace/tree/handle-operators-after-simplify

Could revise to not use CRTP, but would then need a "tag type" for each handle type to keep them distinct, so this seemed simplest.

@whitequark

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

I think this doesn't work, technically speaking. The reason is that according to C++11, only classes without base classes count as POD, and handles really ought to stay POD. In particular, the last time I changed some existing classes to be non-POD, I immediately reaped a bunch of problems with default-initialization and zero-initialization not working as expected--IIRC it was related to default constructor not zero-initializing POD members or something like that but my memory is hazy.

I am neither opposed nor particularly in favor of this, stylistically speaking, since the benefit is not very large and there is quite some churn. I'd have to think more (and probably discuss more) to see if this is a worthwhile change in that respect.

@whitequark

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Oh yeah and it breaks horribly on MSVC for the same reasons. But that's probably the least of the worries here, VS2013 is six years old and we're getting pretty close to the point where upgrading it makes good sense. Not only is VS2013 very buggy, but MS recently shut down the only resource (MS Connect) that had a collection of workarounds for these bugs, so even though I'll get some hate from some last Windows XP holdouts out there, it's really hard to justify still supporting it.

@whitequark

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

I just tried to bump the MSVC requirement to 2015 and of course the testsuite crashes with Command exited with code -1073741571. Not sure why and I don't have that MSVC version locally... help with this is much appreciated.

@rpavlik

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

ah, I figured I must have changed something fundamental for pod initialization to stop working here with GCC - worked around by adding the two constructors. (Pretty sure newer compilers also have xp support in a separate toolchain?)

Can do this in a different way (either code duplication/macro usage or traits) without touching the POD classes themselves, if it's interesting.

edit: oh it's used in a union... eek... yeah quickly getting out of "quickie cleanup hack" territory :)

@whitequark

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Oh, how'd traits work here? That sounds nicer than CRTP.

@rpavlik

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

err, like this... https://github.com/rpavlik/solvespace/tree/handle-operators-2 (oops, I patched it again)

Don't know how to inject the "explicit operator bool" this way, but it at least gives you equality/inequality.

@rpavlik rpavlik referenced this pull request Jul 9, 2019

Merged

Handle operators 2 #448

@whitequark

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

I think this approach is not really viable, but #448 might be good.

@whitequark whitequark closed this Jul 9, 2019

@rpavlik rpavlik deleted the rpavlik:handle-operators branch Jul 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.