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

Fails to compile with Xcode 14.2 #1149

Closed
mikedld opened this issue Mar 5, 2023 · 10 comments · Fixed by #1150 or #1152
Closed

Fails to compile with Xcode 14.2 #1149

mikedld opened this issue Mar 5, 2023 · 10 comments · Fixed by #1150 or #1152

Comments

@mikedld
Copy link

mikedld commented Mar 5, 2023

Using:

  • sqlite_orm 1.8.1 (via vcpkg 2023.02.24)
  • cmake with CMAKE_CXX_STANDARD set to 20
  • GitHub Actions macos-latest (a.k.a. macos-12) image

Getting the following error:

In file included from /Users/runner/work/<snip>:
/Users/runner/work/<snip>/_build/vcpkg_installed/x64-osx/include/sqlite_orm/sqlite_orm.h:12687:23: error: no matching constructor for initialization of 'std::string_view' (aka 'basic_string_view<char>')
                os << std::string_view{it, next};
                      ^               ~~~~~~~~~~
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/include/c++/v1/string_view:267:5: note: candidate constructor not viable: no known conversion from 'std::string::const_iterator' (aka '__wrap_iter<const char *>') to 'const char *' for 1st argument
    basic_string_view(const _CharT* __s, size_type __len) _NOEXCEPT
    ^
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/include/c++/v1/string_view:261:5: note: candidate constructor not viable: requires 1 argument, but 2 were provided
    basic_string_view(const basic_string_view&) _NOEXCEPT = default;
    ^
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/include/c++/v1/string_view:276:5: note: candidate constructor not viable: requires single argument '__s', but 2 arguments were provided
    basic_string_view(const _CharT* __s)
    ^
/Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk/usr/include/c++/v1/string_view:258:5: note: candidate constructor not viable: requires 0 arguments, but 2 were provided
    basic_string_view() _NOEXCEPT : __data (nullptr), __size(0) {}
    ^
1 error generated.

Same code with CMAKE_CXX_STANDARD set to 17 builds fine.

@trueqbit
Copy link
Collaborator

trueqbit commented Mar 5, 2023

Looks like your C++ STL implementation is lacking the iterator range constructor.
What's the version of Clang and the libc++ library?

@mikedld
Copy link
Author

mikedld commented Mar 5, 2023

As indicated and visible in the build log, GHA build agent is using Xcode 14.2, which according to Wikipedia1 is bundled with LLVM/Clang 14.

Footnotes

  1. https://en.wikipedia.org/wiki/Xcode

@trueqbit
Copy link
Collaborator

trueqbit commented Mar 6, 2023

OK, so it comes with libc++ 14 as well. Is it possible for you to check why it doesn't pick up that version of the STL but apparently an earlier one? Someone's asking about that problem on the Apple developer forum.

@mikedld
Copy link
Author

mikedld commented Mar 6, 2023

I've added an assertion:

/Users/runner/work/.../main.cpp:38:1: error: static_assert failed due to requirement '13000 == 14000'
static_assert(_LIBCPP_VERSION == 14000);
^             ~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

That's as much investigation as I have the time for right now. It fails on GitHub Actions, so I hope you're able to reproduce it yourself. I've downgraded to sqlite_orm 1.7.1 which builds fine.

@trueqbit
Copy link
Collaborator

trueqbit commented Mar 7, 2023

@mikedld sqlite_orm is currently using appveyor for build testing, so your information and help is appreciated!

I took a different approach for testing the availability of string_view's iterator constructor, which you may want to try out.

trueqbit added a commit that referenced this issue Mar 8, 2023
Detect the absence of string_view's iterator range ctor differently.
@mikedld
Copy link
Author

mikedld commented Mar 11, 2023

TBH the two code paths and concepts requirement seem like an overcomplication, one C++17 version would be sufficient IMHO, e.g.

void stream_sql_escaped(std::ostream& os, std::string_view str, char char2Escape) {
    while (!str.empty()) {
        const size_t next = str.find(char2Escape);
        os << str.substr(0, next);
        if (next == std::string_view::npos) [[likely]] {
            break;
        }
        const char escapedChar[] = {char2Escape, char2Escape};
        os << std::string_view(escapedChar, std::size(escapedChar));
        str.remove_prefix(next + 1);
    }
}

@mikedld
Copy link
Author

mikedld commented Mar 11, 2023

Oh, I see the project supports building with standards before C++17. In any case I think one implementation is possible, and even if you don't want it using C++20 features in question is far from necessary and doesn't really bring a lot of benefits here.

I didn't test the new version yet, will wait for 1.8.2 or later. Feel free to close this issue if you think it's fixed.

@trueqbit
Copy link
Collaborator

Your version is a very nice approach, thanks for thinking along!

In my defence, I have to say that while the entire thing may be overly complicated at this stage, it can be simplified again once we are past C++17 😃. The goal was to have the most efficient implementation with the latest C++.

However, your version is still great even in the light of iterator ranges.

It is possible to change it even further so that we don't even need a string_view, so one implementation would be enough.

@mikedld @fnc12 What do you think? It even uses the much simpler, non-formatting aware method basic_ostream<>::write():

inline void stream_sql_escaped(std::ostream& os, const std::string& str, char char2Escape) {
    for(size_t offset = 0, next; true; offset = next + 1) {
        next = str.find(char2Escape, offset);

        if(next == str.npos) {
            os.write(str.c_str() + offset, str.size() - offset);
            break;
        }

        os.write(str.c_str() + offset, next - offset + 1);
        os.write(&char2Escape, 1);
    }
}

@fnc12
Copy link
Owner

fnc12 commented Mar 12, 2023

@trueqbit if there is no need to pass std::string_view to this function then it is ok for me

firedaemon-cto pushed a commit to FireDaemon/sqlite_orm that referenced this issue Mar 12, 2023
@trueqbit trueqbit linked a pull request Mar 12, 2023 that will close this issue
trueqbit added a commit that referenced this issue Mar 12, 2023
Use a single implementation of `stream_sql_escaped()`
@trueqbit
Copy link
Collaborator

Thx @mikedld

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants