duckdb_base_std:: plus compile time test on discontinued functions
#17866
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
For the reviewers, main question are:
|
This comment was marked as outdated.
This comment was marked as outdated.
f680b79 to
c3758f8
Compare
Mytherin
reviewed
Jun 11, 2025
e029ace to
5ab81c8
Compare
duckdb_base_std:: plus compile time test on discontinued functions
|
Thanks for the review comments, adapted, I think this is ready for another go |
Mytherin
reviewed
Jun 12, 2025
github-actions bot
pushed a commit
to duckdb/duckdb-r
that referenced
this pull request
Jun 12, 2025
`duckdb_base_std::` plus compile time test on discontinued functions (duckdb/duckdb#17866)
github-actions bot
added a commit
to duckdb/duckdb-r
that referenced
this pull request
Jun 12, 2025
`duckdb_base_std::` plus compile time test on discontinued functions (duckdb/duckdb#17866) Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
Mytherin
added a commit
that referenced
this pull request
Jun 12, 2025
Improved version of #17807, using infrastructure from #17866 to check `std::stringstream` are not used anymore at compile time. Original problem I had bumped in duckdb-wasm, where there would be some yet to be properly understood factor that would change the global static default locale to an unsupported one that would, via a SQL call to `duckdb_tables` end up calling a locale-influenced stringstream that would then throw. Testing can also be done on native applying the following diff, yet to be added as testing mode ```diff diff --git a/test/unittest.cpp b/test/unittest.cpp index 1c04f98..a70847a2a6 100644 --- a/test/unittest.cpp +++ b/test/unittest.cpp @@ -30,6 +30,8 @@ int main(int argc, char *argv[]) { duckdb::unique_ptr<FileSystem> fs = FileSystem::CreateLocal(); string test_directory = DUCKDB_ROOT_DIRECTORY; + std::locale::global(std::locale("ja_JP.SJIS")); + int new_argc = 0; auto new_argv = duckdb::unique_ptr<char *[]>(new char *[argc]); for (int i = 0; i < argc; i++) { ``` Also cleans up comment from previous PR review at #17866 (comment) Note that other streams might (and will) have the same problems, so it would be handy to expand this further, here taking it one step further.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Core constraints are as follow:
std::namespace functions (e.g.:unique_ptrorisspace)stdcodeDesign:
::stdnamespace is wrapped in::duckdb_base_std. This means all original functions are still available, if needed during migration, if needed to implement wrapped classes, or for boundaries with other libraries. This is currently done only for certain classes or functions, but more can be exposed, it's a matter of adding to the headers insrc/include/duckdb/original/std/*.hppor adding a new headerduckdb_base_std::unique_ptrinstead ofstd::unique_ptrsrc/include/duckdb/common/shadow_forbidden_functions.hppfunctions or classes to be forbidden are re-declared instdnamespaceNow when explicitly including
shadow_forbidden_functions.hpp(that can be injected at compile time, see changes inCMakeLists.txt) the compiled codebase will be checked for absence of those functions or classes due to errors being throws connected to multiple declarations (but only on usage).How to add more functions:
src/include/duckdb/original, addingusing ::std::some_funcwithin the namespaceduckdb_base_stdstd::some_functoduckdb_base_std::some_funcsome_functoshadow_forbidden_functions.hppSHADOW_FORBIDDEN_FUNCTIONS=1 GEN=ninja make, now any other usage will be flagged as compiler error. If so, iterateHow to expand to more extensions
Currently this is checked only for a very limited set of source files (
src/,third_party/, and some in-tree extensions).This can be expanded for relevant extensions like:
Future work
SHADOW_FORBIDDEN_FUNCTIONSon a set of known to be working extensions*streamto the group of to be avoided functions