`static inline` functions are not supported by header units & modules

[ZacharyHenkel]

Would it be possible to make SafeIntExceptionAssert simply inline once again?

Here's the error as seen in msvc

safeint3.hpp(430,20): error C2129: static function 'void SafeIntExceptionAssert(void) noexcept' declared but not defined

[dcleblanc]

I don't think you have the latest header, which shouldn't matter - this hasn't been changed in years.

I don't understand how
static inline void SafeIntExceptionAssert() SAFEINT_NOTHROW {}

Could possibly be seen as not defined.

[ZacharyHenkel]

static functions are local to the TU they are defined in. Header units are a TU unto themselves. Thus, the definition of SafeIntExceptionAssert does not exist for consumers of the header unit.

[dcleblanc]

I believe that's incorrect, they're local to the file they're included in. Having something be static inline means you could have the function defined more than once.

Also, I do not repro this on build, note that the CMakeLists specifies /Wall. Is there some off by default warning that would trigger this?

[cdacamar]

The compiler is not required to issue a diagnostic for internal linkage names within a header unit, however using such an internal linkage name is ill-formed as per the standard: [basic.link]/2.3. Since the header unit is its own translation unit, the internal linkage function SafeIntExceptionAssert is not usable from outside.

The best way to remove this restriction is to make the function inline, which would have the same effect as marking it static, but also allow the linker to deduplicate instances as well which could decrease binary sizes.

[dcleblanc]

Yes, that's fine, but if I'm going to fix something, then I need to be able to reproduce it so that I can add whatever it is to the CMake so it won't regress. I currently get no repro on VS2022, clang, or gcc. I'm not opposed to the fix, I just find it curious that there's not a repro on a fairly recent compiler.

[cdacamar]

Here's a minimal repro:

m.h:

#pragma once
static void f() { }

main.cpp:

import "m.h";

int main() {
  f();
}

$ cl /std:c++latest /exportHeader m.h
m.h
$ cl /std:c++latest /headerUnit m.h=m.h.ifc main.cpp
main.cpp
D:\msvc\m.h(3): error C2129: static function 'void f(void)' declared but not defined
D:\msvc\m.h(3): note: see declaration of 'f'

[dcleblanc]

I meant with this header. Also, that function only ever gets called in 4 places inside the header, and never on its own, so your repro isn't accurate. Out of all the compile test code and functionality test code, seems like this error should get hit somewhere.

[dcleblanc]

BTW, I thought your use of std:c++latest might be a clue, added a project with that, builds clean.

[cdacamar]

Even if the function is called indirectly through a function template or inline function, the static function is reachable:

m.h:

#pragma once

static void f() { }

inline void g() { f(); }

template <typename>
void h() { f(); }

main.cpp:

import "m.h";

int main() {
  g();
  h<int>();
}

We're not directly calling f, however it is reached through materializing another definition. Adding /std:c++latest on its own is likely insufficient in your workflows as that will not create a header unit out of the header, which is where the problem manifests. Note: this will also repro with the, now stable, /std:c++20 language mode.

[dcleblanc]

That's all good for the general case. What I need is either a specific alteration to the MsvcTests/CMakeLists.txt that will give me a repro, or simply a main.cpp that includes the header, and the exact compiler options needed to get a repro. Then I'll happily go fix it. I tried making a /std:c++20 project, no errors.

This has been this way for quite a while, something must have changed to create the issue. I also obviously have to go check that the fix doesn't break clang or gcc somehow, neither of which currently gripe about this. There's got to be some difference between @ZacharyHenkel 's environment and my build environment, or I'd see a repro.

[ZacharyHenkel]

Sorry we write our own build system so I cannot provide instructions for CMake. I tried Cameron's minimal repro and it fails with the same error.

Are you testing in two parts to create a header unit via /exportHeader and importing it with /headerUnit m.h=m.h.ifc?

[jacobl-at-ms]

For what its worth, it may not be possible to do this in CMAKE right now. From CMAKEs documentation here:
https://cmake.org/cmake/help/latest/manual/cmake-cxxmodules.7.html

"Header units are not supported."

[jacobl-at-ms]

I tried this set of minimum repro steps based on the info here and couldn't repro the failure either.

C:\Users\jacobl\source\repos\SafeInt>type example.cpp
import "SafeInt.hpp";

int main(int argc)
{
    SafeInt<int> x(argc);
    return x;
}
C:\Users\jacobl\source\repos\SafeInt>cl /std:c++latest /nologo /Wall /wd4668 /EHsc /exportHeader SafeInt.hpp
SafeInt.hpp

C:\Users\jacobl\source\repos\SafeInt>cl /std:c++latest /nologo /Wall /wd4668 /EHsc /headerUnit SafeInt.hpp=SafeInt.hpp.ifc example.cpp

@ZacharyHenkel or @cdacamar can you point out what command line option to CL would trigger the issue? Or is this going to require a specific version of the compiler? The version of cl I had is:

C:\Users\jacobl\source\repos\SafeInt>cl
Microsoft (R) C/C++ Optimizing Compiler Version 19.39.33519 for x86

[jacobl-at-ms]

Aha! Changing the safeint type to unsigned int did the trick:

C:\Users\jacobl\source\repos\SafeInt>type example.cpp
import "SafeInt.hpp";

int main(int argc)
{
    SafeInt<unsigned int> x(argc);
    return x;
}
C:\Users\jacobl\source\repos\SafeInt>cl /std:c++latest /nologo /Wall /wd4668 /EHsc /exportHeader SafeInt.hpp
SafeInt.hpp

C:\Users\jacobl\source\repos\SafeInt>cl /std:c++latest /nologo /Wall /wd4668 /EHsc /headerUnit SafeInt.hpp=SafeInt.hpp.ifc example.cpp
example.cpp
C:\Users\jacobl\source\repos\SafeInt\SafeInt.hpp(425): error C2129: static function 'void SafeIntExceptionAssert(void) noexcept' declared but not defined
C:\Users\jacobl\source\repos\SafeInt\SafeInt.hpp(425): note: see declaration of 'SafeIntExceptionAssert'

[dcleblanc]

That makes sense - you can't throw initializing a SafeInt with an int, nor when unboxing it to return an int. It appears that cmake does support this, but only in the most recent versions, so a non-hacky fix will take some effort to make it part of the normal build-test runs.

Thanks for providing the repro, I'll probably create an interim fix in a branch, and I still need to test that neither gcc or clang are going to do anything unexpected, else I might have to hide the change behind a feature test.