A compiler bug is always possible, but seems unlikely - this is too basic for the compiler to get wrong. However, it is important to realize that there may be a race condition. This line:

const base::TimeDelta VideoDecodeStatsReporter::kRecordingInterval =
    base::TimeDelta::FromSeconds(2);

would ideally be initialized at compile time, but this is not guaranteed. It is guaranteed to be initialized before main/WinMain however and I didn't think we ran tests before then.

Using integral values should work - you can even use an enum to avoid having to allocate storage. The cleaner solution would be to declare the class member as constexpr rather than const, but I don't think VC++ 2015 supports that. I think VC++ 2017 does but the matrix of quirks is too complex for me to be certain.

TL;DR - I can think of two explanations and they both seem unlikely.

Thanks Bruce! 

Re main/WinMain - I took a look at the call stacks between a 64bit build (no test failure) vs the 32bit build (has failure). In both I see a main() call, though what precedes main() differs sightly between the apps. LMK if this amounts to anything.

64 bit stack (no failure)
        ....
        base::LaunchUnitTests [0x000000014065A8B9+137]
        main [0x0000000140077A65+293]
        invoke_main [0x00000001408014B4+52] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:65)
        __scrt_common_main_seh [0x0000000140801377+295] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253)
        __scrt_common_main [0x000000014080123E+14] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:296)
        mainCRTStartup [0x00000001408014D9+9] (f:\dd\vctools\crt\vcstartup\src\startup\exe_main.cpp:17)
        BaseThreadInitThunk [0x00007FFE9B888102+34]
        RtlUserThreadStart [0x00007FFE9BD7C5B4+52]

32 bit stack (has failure)
        ...
        base::LaunchUnitTests [0x02B6ECD2+98]
        main [0x00AC4BB5+213]
        invoke_main [0x05F80D0E+30] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:64)
        __scrt_common_main_seh [0x05F80B90+336] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:253)
        __scrt_common_main [0x05F80A2D+13] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:296)
        mainCRTStartup [0x05F80D28+8] (f:\dd\vctools\crt\vcstartup\src\startup\exe_main.cpp:17)
        BaseThreadInitThunk [0x746238F4+36]
        RtlUnicodeStringToInteger [0x774F5DE3+595]
        RtlUnicodeStringToInteger [0x774F5DAE+542]


Is there any next step for this bug? Anyone else I should assign to? If not feel free to close. 

Constexpr didn't compile in whatever MSVC version we're currently using. 

Will probably go back to using integral values as that seems to be the most c++ way of doing things. Struggling to find a strong recommendation of enum vs int in style guide. LMK if I'm just missing it. 

This email thread touched on this issue:

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/hDqJ4KBVqog

This quote, while not in the style guide, seemed to go unopposed:

Dana Jansens <danakj@chromium.org> said:
> Always use an enum in headers IMO. Just use a static variable in .cc files.

Basically, enums have extremely clear semantics and are easy to use. Compile-time constants are surprisingly tricky to use any other way. But integer constants are at least more reliable than base::TimeDelta constants.

And somewhere in the Chromium style guide says:
"Though the Google C++ Style Guide now says to use kConstantNaming for enums, Chromium was written using MACRO_STYLE naming.  In enums that are actually enumerations (i.e. have multiple values), continue to use this style for consistency.  Use kConstantNaming when using the "enum hack" to define a single constant, as you would for a const int or the like."

If I get a chance I'll look at the bug, if only to see if it repros in VC++ 2017. If it doesn't then I'll probably just close it.

base/time owner here. I suggest avoiding any runtime initialization costs/issues by declaring in the header as:

   public:
    static constexpr base::TimeDelta kRecordingInterval =
        base::TimeDelta::FromSeconds(2);

Note that C++11 requires this "wart" be added to your .cc file (C++14 and later do not):

  // static
  constexpr base::TimeDelta VideoDecodeStatsReporter::kRecordingInterval;

...which is kind of silly because the constexpr shouldn't need storage allocated in any object file...which is why that was fixed in C++14.

FWIW, Dana's comment was from way before all our toolchains even supported C++11, IIRC. With proper, working constexpr support now, it's desirable to use constexpr initialization in many cases where only enums were an option.

> which is kind of silly because the constexpr shouldn't need storage allocated in any object file

Well...

That really depends on the CPU, compiler, use of the object, etc. With an integer constant there are a lot of ways that you can use the value without needing to allocate storage for it - you can embed it in instructions, allocate arrays with it as the size, etc. With a base::TimeDelta object the compiler may well decide that it needs or wants to allocate storage for it.

Do you know if the standard guarantees that these (potentially replicated) object copies will be folded together? With const (and possibly constexpr) objects declared outside of classes this has been a persistent problem, with multiple copies of float/double/struct/array objects ending up in the binary, wasting space. So, I would advise skepticism about defining objects in header files - the compilers may not be able to do sufficient magic to make this work.


Anyway, nice trick. I just tested and VC++ 2015 supports this. I'd previously tried the variant below in order to explicitly allocate storage and it requires VC++ 2017:

class ConstTest {
public:
  static const double kNumber;
};

constexpr double ConstTest::kNumber = 2.0;

I tried to reproduce this with VS 2015 by going back to d7af1e3aba05d9839ecffa52585b6e4808212142 and then changing kRecordingInterval and to be static const members, as shown in the linked source files. However I was unable to reproduce the failures.

Given that we have now moved on to VS 2017 where it is quite possible the bug is fixed and we now have new options for avoiding the problematic pattern I don't think there is anything more for me to do so I'm going to assign this back.

I noticed the "TODO(chcunningham): convert to static constexpr when MSVC support arrives." - as far as I can tell VS 2015 and VS 2017 both support this syntax. I used it without declaring storage in the .cc file. So, maybe convert to the new syntax, or maybe just close this as WontFix?