Improve sanitize_filename #321
Conversation
|
Thank you for your work. I haven't inspected the code thoroughly enough (Including speed tests). but from skimming it, it definitely seems like a significant improvement over the function introduced in #317. Regarding your second commit, I would much prefer if Catch2 was updated rather than modified, if you want to open a PR for that feel free to, otherwise I'll do it after the weekend. Thanks again for the work you've done! |
neumannt
force-pushed
the
sanitize_filename
branch
from
c3092df to
394d930
Compare
|
I have dropped to catch2 patch, the issue is fixed by updating catch2 to v2.13.8. |
neumannt
force-pushed
the
sanitize_filename
branch
from
394d930 to
4af1551
Compare
The old implementation allocated a new string for every invocation, and repeatedly scanned the string for occurences of the various Windows device names. This commits resizes the original string instead if needed, and detects all devices with a single pass.
neumannt
force-pushed
the
sanitize_filename
branch
from
4af1551 to
74e5fa8
Compare
There was a problem hiding this comment.
The speed test I ran showed that for the test string this function was at least 140 times faster than the previous implementation in #317, taking a maximum of 7µs.
Aside from the suggestion below, everything seems to be in order.
The old sanitize_filename implementation copied the string unconditionally, and then did multiple passes over the string to detect Windows special device files. This implementation now keeps the string in place and finds all special devices in a single pass.
The separate commit for catch.hpp is required to build Crown on Ubuntu 21.10, MINSIGSTKSZ is not a constexpr but calls sysconf. This mimics what upstream catch2 does, an alternative would be to update the catch2 header to the latest upstream release.