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

Remove anaphoric macros. #806

Closed
jdoliner opened this issue May 14, 2013 · 20 comments
Closed

Remove anaphoric macros. #806

jdoliner opened this issue May 14, 2013 · 20 comments
Assignees
Milestone

Comments

@jdoliner
Copy link
Contributor

Anaphoric macros are code that's understandable to only one person at the company, they require compiler pragmas to get them to compile and they're incredibly ugly. We should get rid of these ASAP.

@srh
Copy link
Contributor

srh commented May 14, 2013

Some of them, OPT1 at least, also modify local variables like arg, while others don't, and I'm not sure if it's necessary for some reason or not.

@ghost ghost assigned mlucy May 14, 2013
@mlucy
Copy link
Member

mlucy commented May 14, 2013

I concur. (Also, all of them should modify arg; which ones don't?)

@srh
Copy link
Contributor

srh commented May 14, 2013

Only OPT1 and OPT2 if that exist modifies the local variable arg. The others do not.

@mlucy
Copy link
Member

mlucy commented May 14, 2013

Ah; I misunderstood what you meant.

@srh
Copy link
Contributor

srh commented May 14, 2013

Is OPT1 and OPT2's behavior necessary, or could they have made a new arg variable in local scope? I've always been curious about that.

@mlucy
Copy link
Member

mlucy commented May 14, 2013

There's no reason; it's arguably a bug actually.

@AtnNn
Copy link
Member

AtnNn commented Jun 29, 2013

@mlucy have you started work on this issue? I'm going to give it a shot.

@mlucy
Copy link
Member

mlucy commented Jun 29, 2013

@AtnNn -- I would really strongly prefer that we not do this until 1.9 or 2.0 when the RQL changes have slowed down. There's nothing worse than having an interface change under you when there are a bunch of RQL terms in code review.

After that, though, it would be wonderful. This is an important enough interface that I think we should talk about the new one; could you write up a RQL proposal with the new interface you're proposing?

@AtnNn
Copy link
Member

AtnNn commented Jun 29, 2013

I don't have anything specific in mind, I just find the current macros hard to work with.

@jdoliner
Copy link
Contributor Author

I think the new interface is approximately what a C++ driver for rethinkdb would look like. Rewriting stuff would be pretty straightforward if you had that.

@AtnNn
Copy link
Member

AtnNn commented Jul 4, 2013

could you write up a RQL proposal with the new interface you're proposing?

What @jdoliner propose sounds about right: approximately what a C++ driver for rethinkdb would look like

Here is what it could look like: 63f3231455dd826b5744966adf5887fe7dcf8274

@mlucy
Copy link
Member

mlucy commented Jul 4, 2013

I don't think GCC 4.3 supports C++11 anonymous functions, does it? (Otherwise, that interface looks awesome.)

@AtnNn
Copy link
Member

AtnNn commented Jul 5, 2013

I updated the atnnn/minidriver branch to not use lambdas.

I would really strongly prefer that we not do this until 1.9 or 2.0 when the RQL changes have slowed down.

I will stop working on that branch for now.

@ghost ghost assigned AtnNn Sep 12, 2013
@AtnNn
Copy link
Member

AtnNn commented Sep 12, 2013

RQL changes have slowed down. I have rebased and updated the atnnn/minidriver branch and put it up in review 908

@coffeemug
Copy link
Contributor

My opinion (@AtnNn, @mlucy -- feel free to ignore it) is that we should still hold off on making this change. We have a lot on our plates; refactoring code now might introduce bugs that puts even more on our plates and makes stability work harder. I'd rather focus 100% on fixing issues that users face, and do this post 2.0 when things calm down a bit.

@jdoliner
Copy link
Contributor Author

I'm strongly in favor of doing this now. Code gets a lot easier to make stable when more than one person has any idea what it means. This will have the precise opposite effect from what you're describing @coffeemug.

@srh
Copy link
Contributor

srh commented Sep 13, 2013

@coffeemug: It's already in code review and it's great.

@mlucy
Copy link
Member

mlucy commented Sep 13, 2013

I'm way backed up on my reviews right now, but I plan to get to this soon.

There might be some merge conflicts with Review 907, but there should be minimal difficulty aside from that. Now is as good a time as any.

@coffeemug
Copy link
Contributor

Ok, I remove my objections.

AtnNn pushed a commit that referenced this issue Oct 17, 2013
@AtnNn
Copy link
Member

AtnNn commented Nov 1, 2013

This is in review 1005, assigned to @mlucy.

@AtnNn AtnNn closed this as completed in 73c52a9 Nov 13, 2013
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

No branches or pull requests

5 participants