NTSTATUS trick for NTTIME?

tridge at samba.org tridge at samba.org
Wed Mar 9 20:38:11 MST 2011


Hi Volker,

There may be a simpler way to achieve what you want. If we add the
-Wconversion warning to the developer flags then we will get a warning
on mixing of time_t and NTTIME, where NTTIME is typedef to a uint64_t.

I wrote a little test here:

  http://git.samba.org/?p=tridge/junkcode.git;a=tree;f=nttime;

and I get these warnings from the test:

 nttime_test.c:25: warning: conversion to ‘time_t’ from ‘NTTIME’ may change the sign of the result
 nttime_test.c:27: warning: conversion to ‘NTTIME’ from ‘time_t’ may change the sign of the result
 nttime_test.c:33: warning: conversion to ‘NTTIME’ from ‘time_t’ may change the sign of the result
 nttime_test.c:35: warning: conversion to ‘time_t’ from ‘NTTIME’ may change the sign of the result

I also think the structure approach may not win us as much as it does
for NTSTATUS. The NTSTATUS type is mostly used as a return value, but
NTTIME is often used as a pointer argument. When used as a pointer
argument, you only get a warning, not a compile error, which means we
aren't actually better off than we are with -Wconversion. For
functions that return a time, we would get a compile error rather than
a warning by using the structure, but I'm not sure the difference is
worth the overhead of all the accessor functions.

What would you think about trying the low-impact approach of adding
-Wconversion ? The disadvantage would be that we get a whole lot of
warnings from existing code. I've tried this on a S4 build, and it
does add a lot of warnings, but it looks like many of the warnings are
actually things we should fix. 

We could also potentially teach waf to consider gcc -Wconverion
warnings as fatal if the warning contains the words 'NTTIME' and
'time_t'. We could do that with the autoconf build too, with a wrapper
script. 

Alternatively, we could add rules to autobuild to consider a set of
warnings (defined via regular expressions) as fatal, and to refuse the
push to master if there is a NTTIME/time_t warning. That may be the
simplest solution, and potentially we could have -Wconversion only on
in autobuild, to reduce the noise of /usr/include warnings for most
developers (depending on how many there are).

The autobuild approach also would work for a lot of other warning
types, and would be a step towards what we discussed earlier of
autobuild requiring that all commits not increase the warning count.

btw, this relies on the fact that time_t is signed on Linux, whereas
NTTIME is unsigned. I think that we don't have to worry about
portability of the warning though, as enough of us (all of us?)
develop on Linux, so we'll easily catch the errors.

Cheers, Tridge


More information about the samba-technical mailing list