Missing percent_encode<true> symbol (on MacOS/clang)

[feltech]

Version

2.7.4

Platform

Darwin FWAMM008.local 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun 8 22:22:19 PDT 2023; root:xnu-8796.121.3~7/RELEASE_ARM64_T8103 arm64

What steps will reproduce the bug?

Linking libada.a into a MacOS binary that uses ada::unicode::percent_encode<true>

How often does it reproduce? Is there a required condition?

Windows and Linux (Ubuntu) seem fine. MacOS x86 and ARM both fail though. So presumably it's a clang thing.

What is the expected behavior?

Linking succeeds.

What do you see instead?

undef: __ZN3ada7unicode14percent_encodeILb1EEEbNSt3__117basic_string_viewIcNS2_11char_traitsIcEEEEPKhRNS2_12basic_stringIcS5_NS2_9allocatorIcEEEE
Undefined symbols for architecture arm64:
  "bool ada::unicode::percent_encode<true>(std::__1::basic_string_view<char, std::__1::char_traits<char>>, unsigned char const*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>&)", referenced from:
      openassetio::v1::utils::FileUrlPathConverterImpl::pathToUrl(std::__1::basic_string_view<char, std::__1::char_traits<char>>, openassetio::v1::utils::PathType) in lto.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Additional information

$ nm --demangle libada.a | grep percent_encode
0000000000004608 T ada::unicode::percent_encode(std::__1::basic_string_view<char, std::__1::char_traits<char> >, unsigned char const*)
0000000000004ab0 T ada::unicode::percent_encode(std::__1::basic_string_view<char, std::__1::char_traits<char> >, unsigned char const*, unsigned long)
0000000000024b00 T bool ada::unicode::percent_encode<false>(std::__1::basic_string_view<char, std::__1::char_traits<char> >, unsigned char const*, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&)

Note the lack of percent_encode<true> (only false)

I note via a GitHub search that ada::unicode::percent_encode<false> is used within Ada's own codebase, so is instantiated, whereas the true specialisation is never used. So I suspect it's just never generated in the library binary.

[anonrig]

@lemire Any idea how to prevent this?

[lemire]

We don't want users to rely on percent_encode<true>. It is not part of our public API and it could change at any time. It is unfortunate that it appears in our header file, but it does not make it part of public API. It is very much undocumented as far as I can tell. Unsurprisingly, we therefore have no test where we call it as a API function. If you wanted to to make it part of our public API, you might use explicit instantiation declaration which, as far as I know, we do not do. In fact, not only may this change without warning, I would say that it is likely to change in the future.

The following might be sufficient (untested):

--- a/include/ada/unicode.h
+++ b/include/ada/unicode.h
@@ -191,6 +191,9 @@ std::string percent_encode(std::string_view input,
 template <bool append>
 bool percent_encode(std::string_view input, const uint8_t character_set[],
                     std::string& out);
+template bool percent_encode<true>(std::string_view input,
+                                   const uint8_t character_set[],
+                                   std::string& out);
 /**
  * Returns the index at which percent encoding should start, or (equivalently),
  * the length of the prefix that does not require percent encoding.

But you probably want to move this line into a source file and use extern in the header file.

But this assumes that we want users of the ada library to call ada::unicode::percent_encode<true>.

I would say that if we want people to use ada for percent encoding, we should design a proper interface. These functions were developed for our own use cases. They may have all sorts of assumptions builtin.

[feltech]

For context, I came across this whilst writing file://<->path conversion functions, so unless/until #309 is implemented, having Ada do the percent encoding/decoding for us is really handy 😉

In particular, it is required to not percent-encode the beginning of some URLs (file:///C:/). So the template specialisation in question, which conditionally appends if percent-encoding is required, fit the bill.

It's not the end of the world if this particular function remains private, but means an additional string allocation even if no percent-encoding is required (which is what I've done for now).

[lemire]

I agree that solving issue 309 is desirable but the step forward is to actually go ahead and implement it in ada. Right?

Exposing dirty bits seems like a step backward.