MDEV-35461 Remove redundant checks for standard library functions #4017
MDEV-35461 Remove redundant checks for standard library functions #4017longjinvan wants to merge 1 commit intoMariaDB:mainfrom
Conversation
|
|
svoj
added
the
External Contribution
|
@vuvova in MDEV-35461 you suggested that these checks should be removed. Which means they have to be removed along with compatibility code spread across other files. Could you confirm it is still your preference? |
|
Yes, sure, why would we want garbage like |
svoj
left a comment
svoj
left a comment
There was a problem hiding this comment.
Please cleanup compatibility code as well.
|
@longjinvan Hi! It's been over 2 months since changes were requested. Are you still interested in completing this task? |
|
Hi @svoj, I'm still working on this task, I will submit a new PR as soon as possible. Thanks! |
|
@longjinvan alright. It'd be better to update this pull request though, rather than creating new one. I will convert this one to draft so that it is off of our radar. Reset draft status once it is updated. |
longjinvan
force-pushed
the
MDEV-35461
branch
from
7780239 to
3d05318
Compare
|
(windows) |
longjinvan
force-pushed
the
MDEV-35461
branch
from
3d05318 to
1fa790c
Compare
longjinvan
force-pushed
the
MDEV-35461
branch
from
4ecdbe2 to
0c8a032
Compare
svoj
left a comment
svoj
left a comment
There was a problem hiding this comment.
git grep HAVE_ALLOCA and git grep HAVE_MEMCPY still return quite a few items that should be cleaned up, right?
cmake/os/LinuxCache.cmake
Outdated
| SET(HAVE_STRTOUL 1 CACHE INTERNAL "") | ||
| SET(HAVE_STRTOULL 1 CACHE INTERNAL "") | ||
| SET(HAVE_VSNPRINTF 1 CACHE INTERNAL "") | ||
| ENDIF() No newline at end of file |
There was a problem hiding this comment.
Given that you removed all occurrences of these defines, why do you have to keep this file? There must be no point in having them in WindowsCache.cmake either.
| #else | ||
| vsprintf (msg_buf, format, args); | ||
| msg_buf[sizeof(msg_buf) - 1] = '\0'; /* overflow? */ | ||
| #endif |
There was a problem hiding this comment.
readline is a third party library. It would be good to keep it intact and add -DHAVE_VSNPRINTF to extra/readline/CMakeLists.txt instead. Along with all other required definitions.
The very same point applies to zlib, its CMakeLists.txt should be updated.
longjinvan
force-pushed
the
MDEV-35461
branch
2 times, most recently
from
bd04ca2 to
1119725
Compare
gkodinov
changed the title
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your submission. I'm terribly sorry that it is taking that long for the review to conclude. I hope that you're still willing to work on this.
The diff LGTM.
Please stand by for the final review.
Remove function checks that are guaranteed by C99 standards to reduce the excessive time spent during CMake configuration step. Also, clean up related compatibility code. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
6 participants
Description
This commit removes function checks for standard library functions that are guaranteed to be available when using C99 standards[1]. These include:
For
memcpyandvsnprintf, the global checks are removed but the defines are still passed locally to third-party libraries (zlibandreadline) via their respectiveCMakeLists.txt, since those libraries still reference them internally.The corresponding cached values in
cmake/os/WindowsCache.cmakeare also removed where the checks no longer exist.All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
[1] https://www.dii.uchile.cl/~daespino/files/Iso_C_1999_definition.pdf
How can this PR be tested?
✅ Build succeed.
✅ All MTR tests pass.
Basing the PR against the correct MariaDB version
✅ This is a code change related to build performance improvement applicable to multiple versions, and the PR is based against the latest MariaDB development branch.
PR quality check
✅ I have checked the
CODING_STANDARDS.mdfile and my PR conforms to this where appropriate.✅ For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.
Copyright
All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.