Conversation
✅ Deploy Preview for dpp-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
github-actions
bot
added
documentation
|
CONFLICTS? I just pulled the branch bruv Also remind me to turn modules off by default in CMake before marking as ready |
braindigitalis
left a comment
braindigitalis
left a comment
There was a problem hiding this comment.
I know it's not part of this pr but I also think we should add a new CI entry for static link on Linux
| option(DPP_USE_PCH "Use precompiled headers to speed up compilation" OFF) | ||
| option(AVX_TYPE "Force AVX type for speeding up audio mixing" OFF) | ||
| option(DPP_TEST_VCPKG "Force VCPKG build without VCPKG installed (for development use only!)" OFF) | ||
| option(DPP_MODULES "Support for C++20 modules" ON) |
There was a problem hiding this comment.
should this default to off? it is experimental
There was a problem hiding this comment.
For sure yeah, that's just for my convenience while developing, I'll leave this unresolved so I don't forget to turn it off
| @@ -0,0 +1,27 @@ | |||
| cmake_minimum_required(VERSION 3.30) | |||
There was a problem hiding this comment.
the latest Ubuntu seems to only have 3.22, is this an acceptable version on msvc?
There was a problem hiding this comment.
I believe msvc ships with 3.30 yeah, although I can take it down to 3.28, 3.30 is import std; experimental support, I'll change it tomorrow
As for package managers, not my problem :kekw: but yeah that does mean OFF by default
| * You can retrieve them with std::get(). | ||
| */ | ||
| typedef std::variant<std::monostate, std::string, int64_t, bool, snowflake, double> command_value; | ||
| DPP_EXPORT typedef std::variant<std::monostate, std::string, int64_t, bool, snowflake, double> command_value; |
There was a problem hiding this comment.
do we need to export declarations that have no bodies in a cpp file?
There was a problem hiding this comment.
The export in modules work a bit differently than Window's __declspec(dllexport) in that it's about names rather than definitions, a little bit like public and private in classes, so yes we need to export the names we introduce to be used as part of the API
| * @param choice command_option_choice to be serialized | ||
| */ | ||
| void to_json(nlohmann::json& j, const command_option_choice& choice); | ||
| DPP_EXPORT void to_json(nlohmann::json& j, const command_option_choice& choice); |
There was a problem hiding this comment.
I'm not sure these are supposed to be exported to the library user
There was a problem hiding this comment.
Hmm I'll double check but if they're used by templates or if we want conversions from json etc they need to be visible which means exported
|
First of all. Thank you for your contributions. All stuff u do is just crazy 👍. I have a question. U specified that it works on clang and almost on MVSC. But what about g++? Is there issues porting it there. Or does g++ |
|
the fact this PR is "1400" too is satisfying |
|
@braindigitalis for sure I can add that @ruslan-ilesik I believe g++14 has them experimental and g++15 (unreleased) has a lot of work in them, I'll test it as well, but I'm ready to ship it just for clang and msvc if it doesn't work on g++ |
|
PR on hold, will reopen later |
4 participants
There are still some things to iron out, but it works on clang, and almost works on MSVC...
Code change checklist