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

ADL issue in input_adapter #2248

Closed
mika-fischer opened this issue Jul 7, 2020 · 6 comments
Closed

ADL issue in input_adapter #2248

mika-fischer opened this issue Jul 7, 2020 · 6 comments
Labels
kind: bug state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Comments

@mika-fischer
Copy link

json::parse does not work with containers which only provide begin() and end() member functions. This worked in 3.7.0.

Example https://godbolt.org/z/NtXn4M:

#if 1
#include "https://raw.githubusercontent.com/nlohmann/json/v3.8.0/single_include/nlohmann/json.hpp"
#else
#include "https://raw.githubusercontent.com/nlohmann/json/v3.7.0/single_include/nlohmann/json.hpp"
#endif

struct container {
  std::string data;              
  auto begin() const { return data.begin(); }
  auto end()   const { return data.end();   }
};

auto foo(container& c) { return nlohmann::json::parse(c); }

gives a (long) compile error.

This is probably because of the unqualified begin and end in the trailing return type here (https://github.com/nlohmann/json/blob/v3.8.0/single_include/nlohmann/json.hpp#L4796-L4805) without using std::begin; using std::end;:

template<typename ContainerType>
 auto input_adapter(const ContainerType& container) -> decltype(input_adapter(begin(container), end(container)))
 {
     // Enable ADL
     using std::begin;
     using std::end;
  
     return input_adapter(begin(container), end(container));
 }

The old code had std::begin and std::end there, which is also wrong, for different reasons. Unfortunately, I don't know how to fix it...

@gregmarr
Copy link
Contributor

gregmarr commented Jul 7, 2020

You could fix this by adding these functions in the same namespace as container

auto begin(container &c) {
    return c.begin();
}
auto end(container &c) {
    return c.end();
}

but I think it would be better to not require this.

@Francois-air does this function need the trailing return type, or can it be removed? The function only has a single return statement, so the auto and return should be sufficient.

@mika-fischer
Copy link
Author

You could fix this by adding these functions in the same namespace as container

auto begin(container &c) {
    return c.begin();
}
auto end(container &c) {
    return c.end();
}

Sure, I'm aware of that. The bug is that this should not be required of the user, as you say.

@Francois-air does this function need the trailing return type, or can it be removed? The function only has a single return statement, so the auto and return should be sufficient.

I would assume that it's because of C++11 compatibility...

By "I don't know how to fix this" I meant: I don't know how to correctly specify the return type with C++11, so that it works in all cases...

@mika-fischer
Copy link
Author

Probably something like this is the way to go: https://stackoverflow.com/a/35036343

@gregmarr
Copy link
Contributor

gregmarr commented Jul 7, 2020

Ugh, I was thinking that C++11 supported automatic return type deduction from a single return and C++14 expanded it to multiple returns, but it was actually 14 that added the automatic return type deduction and 17 that expanded it to multiple returns. :(

@stale
Copy link

stale bot commented Aug 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Aug 8, 2020
@stale stale bot closed this as completed Aug 16, 2020
@nlohmann nlohmann reopened this Aug 19, 2020
@stale stale bot removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Aug 19, 2020
@stale
Copy link

stale bot commented Sep 20, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Sep 20, 2020
@stale stale bot closed this as completed Oct 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated
Projects
None yet
Development

No branches or pull requests

3 participants