Thank you for the clarification. I will do it.
On Sun, Apr 12, 2020 at 1:33 PM Bruno Haible <br...@clisp.org> wrote: > > Hi Bastien, > > > [PATCH 1/6] Use memset_s if possible for explicit_bzero > > [PATCH 2/6] Use SecureZeroMemory on windows for explicit_bzero > > [PATCH 3/6] Support clang for explicit_bzero > > [PATCH 4/6] Implement fallback for explicit_bzero using jump to > > [PATCH 5/6] Improve styling in explicit_bzero > > [PATCH 6/6] Add test for explicit_bzero > > I would reorder the patch series so that adding the unit test comes first. > This allows to check against regressions in the other patches. > > Then, patch 1, 2, 3 look good but would have to be tested (on FreeBSD 12, > Solaris 11.4, native Windows, clang). On which systems can you test? I > do have test capacities, but I don't want to waste time testing the same > platforms that you have already tested. > > I would refrain from applying patch 4, because it is more complex than > what seems needed. You cite the C standard regarding 'volatile'. It is > 'volatile' which prevents the compiler from "looking inside the function > pointer"; that does not require 'static' nor a runtime computation. > I may be wrong on this, but if I'm wrong, the unit test will tell us. > So, I would propose to > 1. simplify patch 4 to a simple use of a volatile function pointer, > 2. test it on the relevant platforms (a non-GCC, non-clang compiler: > that is MSVC, Solaris cc, HP cc, AIX xlc). > 3. use more complex code only of one of these tests indicates that > it is required. > > Patch 5 is evidently OK. > > Bruno >
