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

[BUG] string interpolation of certain types warns about "non-portable usage of variadic functions" #1136

Closed
DyXel opened this issue Jun 22, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@DyXel
Copy link
Contributor

DyXel commented Jun 22, 2024

Describe the bug
Title. This one has different effects on different compilers.

To Reproduce
Steps to reproduce the behavior:

  1. Sample code - distilled down to minimal essentials please
#include <filesystem>

my_enum: @enum type = {
    foo;
    bar;
}

aaa: () -> std::string = {
    a := my_enum::foo;
    return "(a)$";
}

bbb: () -> std::string = {
    p: std::filesystem::path = ();
    return "(p)$";
}

main: () = std::cout << aaa() << bbb();
  1. Command lines including which C++ compiler you are using
    Clang 17: -std=c++20 -Wall -Wextra -pedantic -Werror
    MSVC: /std:c++latest /MD /EHsc /experimental:module /W4 /WX
  2. Expected result - what you expected to happen
    I would expect these to compile on all compilers, they seem to work in GCC 14. But non-GCC compilers seem unhappy about it.
  3. Actual result/error
    On clang I get the following:
error: cannot pass object of non-trivial type 'typename std::remove_reference<my_enum &>::type' (aka 'my_enum') through variadic function; call will abort at runtime [-Wnon-pod-varargs]
   63 |     return { cpp2::to_string(cpp2::move(a)) }; 
      |                              ^
1 error generated.

On MSVC I get roughly:

error C2220: the following warning is treated as an error
warning C4840: non-portable use of class 'std::filesystem::path' as an argument to a variadic function
note: 'std::filesystem::path::path' is non-trivial
note: see declaration of 'std::filesystem::path::path'
note: the constructor and destructor will not be called; a bitwise copy of the class will be passed as the argument
note: see declaration of 'std::filesystem::path'

So the @enum case seems to fail in clang (presumably because it defines a to_string member function?) and the std::filesystem::path case seems to fail on MSVC. This latter one I have no idea whats wrong.
Additional context
For the @enum case, calling to_string directly seems to workaround the issue, although that mostly makes the string interpolation redundant.

Initially I thought about opening an issue for each case, but they seem very related. Also I think these have to do with the definition at cpp2util.h:1192.

@DyXel DyXel added the bug Something isn't working label Jun 22, 2024
@DyXel
Copy link
Contributor Author

DyXel commented Jun 22, 2024

Addendum: Clang always fails to compile this even if you don't turn up all the warnings. If you instead use -Wno-non-pod-varargs, it compiles, but once you execute the program it says: Illegal instruction.

@hsutter
Copy link
Owner

hsutter commented Jun 23, 2024

Thanks! Fixed... now the first case works as expected, and the second case gives the correct "not customized" message for filesystem::path.

That said, I could see adding support for "anything you can stream with << by pushing it through a stringstream" and I've added a code comment for that, but the naive overload creates ambiguities with other overloads so I think the right thing is to change those core to_string overloads into a single function with if constexpr alternatives (as @filipsajdak did with is, thanks again Filip!) and then it should work fine... I just didn't have time to do that today.

@DyXel
Copy link
Contributor Author

DyXel commented Jun 23, 2024

Thanks. I think the crashes were related to the fact that auto to_string(...) uses the old vararg stuff from C. So the compiler was right about warning for that.

For the std::filesystem::path one, I was getting very frustrated because I thought that ought to work, and in fact, the documentation says that it has operator string_type(), what is not obvious is the fact that string_type is probably not the regular std::string but std::wstring on Windows, so yeah...

MarekKnapek pushed a commit to MarekKnapek/cppfront that referenced this issue Jul 4, 2024
…auto const&`

This avoids errors about passing non-PODs to variadics (even though the parameter is never used), and ensures the catchall is used only for a single parameter (not inadvertently for a format string)

Closes hsutter#1136
@hsutter
Copy link
Owner

hsutter commented Jul 12, 2024

Catching up: Last week's commit cced142 also added the support you asked for above to allow interpolating std::filesystem::path and anything else that supports the << streaming operator. Fixed! Thanks again.

@DyXel
Copy link
Contributor Author

DyXel commented Jul 12, 2024

Sweet. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants