Skip to content

Conversation

MichealReed
Copy link
Contributor

@MichealReed MichealReed commented Aug 17, 2024

The way we were printing from LOG caused a crash when embedded on windows. This updates the logger to be more typesafe and generally compatible across platforms.

I removed exit(1) from the check function. We should never automatically exit the program as a library, this can prevent proper logging if there is indeed a crash.

Also introduces a NO_LOG definition that we should add throughout.

Also adds log level 0 for silent output.

@MichealReed
Copy link
Contributor Author

MichealReed commented Aug 17, 2024

Crash Log

Crash Log
 	ucrtbased.dll!00007ffea3b1ac87()	Unknown
 	ucrtbased.dll!00007ffea3b1b370()	Unknown
 	ucrtbased.dll!00007ffea3b1ab16()	Unknown
 	ucrtbased.dll!00007ffea3b1bf8d()	Unknown
 	ucrtbased.dll!00007ffea3a1f128()	Unknown
 	ucrtbased.dll!00007ffea3a1e242()	Unknown
 	ucrtbased.dll!00007ffea3afd4e2()	Unknown
 	ucrtbased.dll!00007ffea3af517f()	Unknown
 	ucrtbased.dll!00007ffea3ae1715()	Unknown
 	ucrtbased.dll!00007ffea3b09579()	Unknown
>	minigpu_ffi.dll!vsnprintf(char * const _Buffer, const unsigned __int64 _BufferCount, const char * const _Format, char * _ArgList) Line 1439	C++
 	minigpu_ffi.dll!snprintf(char * const _Buffer, const unsigned __int64 _BufferCount, const char * const _Format, ...) Line 1931	C++
 	minigpu_ffi.dll!gpu::LOG(gpu::Logger & logger, int level, const char * message, ...) Line 48	C++

@austinvhuang austinvhuang merged commit e124bb1 into AnswerDotAI:main Aug 22, 2024
1 check passed
@austinvhuang
Copy link
Contributor

+1 to the exit policy. Error handling in general will needs some consideration.

There's quite a few places where the return type should conceptionally return optional types (Error or Value), but that does add some complexity to the API and optional types aren't as clean / idiomatic in C++ as they are in rust or zig. OTOH i don't like the idea of throwing exceptions either, but we'll have to bite the bullet one way or another and probably commit to something in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants