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

X3: Simplify lambda_visitor. #114

Closed
wants to merge 1 commit into from
Closed

X3: Simplify lambda_visitor. #114

wants to merge 1 commit into from

Conversation

mlang
Copy link
Contributor

@mlang mlang commented Jun 2, 2015

If I am not missing something obvious, there is actually no need for
lambda_visitor to be as complicated as it is.

@vtnerd
Copy link
Contributor

vtnerd commented Jun 2, 2015

Are there tests for the multiple lambda case? The original version leverages the using keyword to bring in the overloads from the parent, so the outtermost instance had all overloads in the same visibility. I think this verison is ambigious with multiple lambdas, but I haven't tested this exact scenario (operators). It should be the same. I don't think its possible to expand the using declaration either here, I tried a couple of ways with clang 3.5. I don't think the C++ language syntax supports commas in this situation.

@djowel
Copy link
Collaborator

djowel commented Jun 3, 2015

There's a plan to expand this to an overloads set. So consider this a work in progress. OTOH, perhaps it belongs somewhere else. How is it ambiguous? A test case would be good.

@vtnerd
Copy link
Contributor

vtnerd commented Jun 3, 2015

I think the patch here would create an ambiguous function call if multiple lambdas were provided, whereas the code on the develop branch won't. The lambda_visitor class would have an operator() defined in multiple of its children so anyone invoking operator() on lambda_visitor would have a compilation error. The stackoverflow article linked above describes the same situation with named functions. The current version on the develop branch should not have this issue due to the recursive using statements.

Also, I said I think because clang 3.5 doesn't complain with operator(), but does complain if the function is given a name. gcc 4.8 complains in both situations. I wouldn't expect a special case for operators, but I haven't looked up the specification (stackoverflow provides a decent overview). Since @K-ballo filed this lovely bug, I'm assuming its still an outstanding issue with Clang.

@djowel
Copy link
Collaborator

djowel commented Jun 3, 2015

Ah I see. I should read more carefully. Happens when there's not enough caffeine :)

@K-ballo
Copy link
Member

K-ballo commented Jun 3, 2015

IIUC overloads are declarations with the same name in the same scope, while all the aggregated operator() in the proposed change obviously come from a different scope each. I believe the intended usage of the code to be ill-formed, but compilers disagree (in interesting ways). I'll consult a higher power and get back.

@mlang
Copy link
Contributor Author

mlang commented Jun 3, 2015 via email

@djowel djowel closed this Jun 4, 2015
@tomilov
Copy link

tomilov commented Jun 4, 2015

I am sure here is the answer.

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.

5 participants