Update ErrorReport.h #190
Update ErrorReport.h #190
Conversation
At least it's organized and you can understand something.
VER.02
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @dracy21, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I have reviewed this pull request, which fundamentally redefines the role of the ErrorReport.h file. It transitions from a simple declaration header to a full-fledged implementation containing a robust error reporting and system diagnostics utility. The changes significantly expand the application's ability to gather and log detailed information about the host system's hardware and software environment, while also enhancing the underlying log file management capabilities.
Highlights
- File Transformation: The
ErrorReport.hfile has been completely transformed from a header file containing only declarations into a comprehensive implementation file for theCErrorReportclass and related system information gathering functions. - System Information Gathering: Extensive new functionality has been added to collect and log detailed system diagnostics, including OS version, CPU details (vendor, model, frequency), total physical memory, DirectX version, OpenGL capabilities, IME settings, and sound card information.
- Advanced System Metrics: The pull request introduces methods for measuring CPU frequency using the RDTSC instruction and a detailed, multi-stage process for detecting the installed DirectX version (from DX1 up to DX9).
- Log File Management Enhancements: The
CErrorReportclass now includes improved log file handling, such as automatically deleting existing log files, creating new ones with a UTF-16 Byte Order Mark (BOM), and a mechanism to truncate older log entries to manage file size.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The code changes introduce a new implementation for the ErrorReport class, which handles error logging and system information gathering. The new implementation uses Windows API calls for file operations and system information retrieval. The code includes functions for writing debug information, system information, OpenGL information, IME information, and sound card information to a log file.
| wchar_t lpszBuffer[128 * 1024]; | ||
| ReadFile(m_hFile, lpszBuffer, 128 * 1024 - 1, &dwNumber, NULL); |
There was a problem hiding this comment.
Using a fixed-size buffer of 128KB on the stack could lead to stack overflow issues. Consider using dynamic allocation or a smaller, more reasonable size.[^1]
| wchar_t lpszBuffer[2048] = { 0 }; | ||
| va_list va; | ||
| va_start(va, lpszFormat); | ||
| vswprintf(lpszBuffer, _countof(lpszBuffer), lpszFormat, va); | ||
| va_end(va); |
There was a problem hiding this comment.
Consider using std::format or fmt::format for safer and more readable string formatting.[^1] vswprintf is prone to buffer overflows if the format string doesn't match the arguments.
| void CErrorReport::WriteLabelValue(const wchar_t* label, const wchar_t* value) { | ||
| const int labelWidth = 25; | ||
| wchar_t buffer[256]; | ||
| swprintf(buffer, L"%-*s: %s\r\n", labelWidth, label, value); | ||
| WriteDebugInfoStr(buffer); | ||
| } |
There was a problem hiding this comment.
Consider using std::filesystem::path for cross-platform file path manipulation instead of wchar_t[MAX_PATH].[^1] This can improve code readability and maintainability.
| m_hFile = ::CreateFile(m_lpszFileName, // Using :: to explicitly call Windows API | ||
| GENERIC_READ | GENERIC_WRITE, // Read/write access | ||
| FILE_SHARE_READ, // Allow others to read | ||
| NULL, | ||
| CREATE_ALWAYS, // Overwrite if exists | ||
| FILE_ATTRIBUTE_NORMAL, // Normal file attributes | ||
| NULL); |
There was a problem hiding this comment.
Consider using std::ofstream for file operations instead of the Windows API. This can simplify the code and make it more portable.[^1]
| __int64 GetCPUFrequency(unsigned int uiMeasureMSecs) | ||
| { | ||
| assert(uiMeasureMSecs > 0); |
There was a problem hiding this comment.
Consider using std::chrono for time measurements instead of Sleep and QueryPerformanceCounter. This can provide more accurate and portable time measurements.[^1]
| __asm | ||
| { | ||
| mov eax, 1 | ||
| cpuid | ||
| mov reg, edx | ||
| } |
There was a problem hiding this comment.
The use of inline assembly (__asm) is not portable and can be difficult to maintain. Consider using platform-independent alternatives if possible.[^1]
| __asm | ||
| { | ||
| mov eax, 0 | ||
| cpuid | ||
| mov eaxreg, eax | ||
| mov ebxreg, ebx | ||
| mov edxreg, edx | ||
| mov ecxreg, ecx | ||
| } |
There was a problem hiding this comment.
The use of inline assembly (__asm) is not portable and can be difficult to maintain. Consider using platform-independent alternatives if possible.[^1]
| DWORD GetDXVersion() | ||
| { | ||
| public: | ||
| CErrorReport(); | ||
| virtual ~CErrorReport(); | ||
| DIRECTDRAWCREATE DirectDrawCreate = NULL; | ||
| DIRECTDRAWCREATEEX DirectDrawCreateEx = NULL; | ||
| DIRECTINPUTCREATE DirectInputCreate = NULL; | ||
| HINSTANCE hDDrawDLL = NULL; | ||
| HINSTANCE hDInputDLL = NULL; | ||
| HINSTANCE hD3D8DLL = NULL; | ||
| HINSTANCE hD3D9DLL = NULL; | ||
| LPDIRECTDRAW pDDraw = NULL; | ||
| LPDIRECTDRAW2 pDDraw2 = NULL; | ||
| LPDIRECTDRAWSURFACE pSurf = NULL; | ||
| LPDIRECTDRAWSURFACE3 pSurf3 = NULL; | ||
| LPDIRECTDRAWSURFACE4 pSurf4 = NULL; | ||
| DWORD dwDXVersion = 0; | ||
| HRESULT hr; | ||
|
|
||
| // First see if DDRAW.DLL even exists. | ||
| hDDrawDLL = LoadLibrary(L"DDRAW.DLL"); | ||
| if (hDDrawDLL == NULL) | ||
| { | ||
| dwDXVersion = 0; | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| // See if we can create the DirectDraw object. | ||
| DirectDrawCreate = (DIRECTDRAWCREATE)GetProcAddress(hDDrawDLL, "DirectDrawCreate"); | ||
| if (DirectDrawCreate == NULL) | ||
| { | ||
| dwDXVersion = 0; | ||
| FreeLibrary(hDDrawDLL); | ||
|
|
||
| __TraceF(TEXT("===> Couldn't LoadLibrary DDraw\r\n")); | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| void Clear(void); | ||
| hr = DirectDrawCreate(NULL, &pDDraw, NULL); | ||
| if (FAILED(hr)) | ||
| { | ||
| dwDXVersion = 0; | ||
| FreeLibrary(hDDrawDLL); | ||
| __TraceF(TEXT("===> Couldn't create DDraw\r\n")); | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| protected: | ||
| HANDLE m_hFile; | ||
| wchar_t m_lpszFileName[MAX_PATH]; | ||
| int m_iKey; | ||
| public: | ||
| void Create(wchar_t* lpszFileName); | ||
| void Destroy(void); | ||
| protected: | ||
| void CutHead(void); | ||
| wchar_t* CheckHeadToCut(wchar_t* lpszBuffer, DWORD dwNumber); | ||
| // So DirectDraw exists. We are at least DX1. | ||
| dwDXVersion = 0x100; | ||
|
|
||
| protected: | ||
| BOOL WriteFile(HANDLE hFile, void* lpBuffer, DWORD nNumberOfBytesToWrite, LPDWORD lpNumberOfBytesWritten, LPOVERLAPPED lpOverlapped); | ||
| public: | ||
| void WriteDebugInfoStr(wchar_t* lpszToWrite); | ||
| void Write(const wchar_t* lpszFormat, ...); | ||
| void HexWrite(void* pBuffer, int iSize); | ||
| // Let's see if IID_IDirectDraw2 exists. | ||
| hr = pDDraw->QueryInterface(IID_IDirectDraw2, (VOID**)&pDDraw2); | ||
| if (FAILED(hr)) | ||
| { | ||
| // No IDirectDraw2 exists... must be DX1 | ||
| pDDraw->Release(); | ||
| FreeLibrary(hDDrawDLL); | ||
| __TraceF(TEXT("===> Couldn't QI DDraw2\r\n")); | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| void AddSeparator(void); | ||
| void WriteLogBegin(void); | ||
| void WriteCurrentTime(BOOL bLineShift = TRUE); | ||
| // IDirectDraw2 exists. We must be at least DX2 | ||
| pDDraw2->Release(); | ||
| dwDXVersion = 0x200; | ||
|
|
||
| //------------------------------------------------------------------------- | ||
| // DirectX 3.0 Checks | ||
| //------------------------------------------------------------------------- | ||
|
|
||
| // DirectInput was added for DX3 | ||
| hDInputDLL = LoadLibrary(L"DINPUT.DLL"); | ||
| if (hDInputDLL == NULL) | ||
| { | ||
| // No DInput... must not be DX3 | ||
| __TraceF(TEXT("===> Couldn't LoadLibrary DInput\r\n")); | ||
| pDDraw->Release(); | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| DirectInputCreate = (DIRECTINPUTCREATE)GetProcAddress(hDInputDLL, | ||
| "DirectInputCreateA"); | ||
| if (DirectInputCreate == NULL) | ||
| { | ||
| // No DInput... must be DX2 | ||
| FreeLibrary(hDInputDLL); | ||
| FreeLibrary(hDDrawDLL); | ||
| pDDraw->Release(); | ||
| __TraceF(TEXT("===> Couldn't GetProcAddress DInputCreate\r\n")); | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| // DirectInputCreate exists. We are at least DX3 | ||
| dwDXVersion = 0x300; | ||
| FreeLibrary(hDInputDLL); | ||
|
|
||
| // Can do checks for 3a vs 3b here | ||
|
|
||
| //------------------------------------------------------------------------- | ||
| // DirectX 5.0 Checks | ||
| //------------------------------------------------------------------------- | ||
|
|
||
| // We can tell if DX5 is present by checking for the existence of | ||
| // IDirectDrawSurface3. First, we need a surface to QI off of. | ||
| DDSURFACEDESC ddsd; | ||
| ZeroMemory(&ddsd, sizeof(ddsd)); | ||
| ddsd.dwSize = sizeof(ddsd); | ||
| ddsd.dwFlags = DDSD_CAPS; | ||
| ddsd.ddsCaps.dwCaps = DDSCAPS_PRIMARYSURFACE; | ||
|
|
||
| hr = pDDraw->SetCooperativeLevel(NULL, DDSCL_NORMAL); | ||
| if (FAILED(hr)) | ||
| { | ||
| // Failure. This means DDraw isn't properly installed. | ||
| pDDraw->Release(); | ||
| FreeLibrary(hDDrawDLL); | ||
| dwDXVersion = 0; | ||
| __TraceF(TEXT("===> Couldn't Set coop level\r\n")); | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| hr = pDDraw->CreateSurface(&ddsd, &pSurf, NULL); | ||
| if (FAILED(hr)) | ||
| { | ||
| // Failure. This means DDraw isn't properly installed. | ||
| pDDraw->Release(); | ||
| FreeLibrary(hDDrawDLL); | ||
| dwDXVersion = 0; | ||
| __TraceF(TEXT("===> Couldn't CreateSurface\r\n")); | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| // Query for the IDirectDrawSurface3 interface | ||
| if (FAILED(pSurf->QueryInterface(IID_IDirectDrawSurface3, | ||
| (VOID**)&pSurf3))) | ||
| { | ||
| pDDraw->Release(); | ||
| FreeLibrary(hDDrawDLL); | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| // QI for IDirectDrawSurface3 succeeded. We must be at least DX5 | ||
| dwDXVersion = 0x500; | ||
|
|
||
| //------------------------------------------------------------------------- | ||
| // DirectX 6.0 Checks | ||
| //------------------------------------------------------------------------- | ||
|
|
||
| // The IDirectDrawSurface4 interface was introduced with DX 6.0 | ||
| if (FAILED(pSurf->QueryInterface(IID_IDirectDrawSurface4, | ||
| (VOID**)&pSurf4))) | ||
| { | ||
| pDDraw->Release(); | ||
| FreeLibrary(hDDrawDLL); | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| // IDirectDrawSurface4 was create successfully. We must be at least DX6 | ||
| dwDXVersion = 0x600; | ||
| pSurf->Release(); | ||
| pDDraw->Release(); | ||
|
|
||
| //------------------------------------------------------------------------- | ||
| // DirectX 6.1 Checks | ||
| //------------------------------------------------------------------------- | ||
|
|
||
| // Check for DMusic, which was introduced with DX6.1 | ||
| LPDIRECTMUSIC pDMusic = NULL; | ||
| CoInitialize(NULL); | ||
| hr = CoCreateInstance(CLSID_DirectMusic, NULL, CLSCTX_INPROC_SERVER, | ||
| IID_IDirectMusic, (VOID**)&pDMusic); | ||
| if (FAILED(hr)) | ||
| { | ||
| __TraceF(TEXT("===> Couldn't create CLSID_DirectMusic\r\n")); | ||
| FreeLibrary(hDDrawDLL); | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| // DirectMusic was created successfully. We must be at least DX6.1 | ||
| dwDXVersion = 0x601; | ||
| pDMusic->Release(); | ||
| CoUninitialize(); | ||
|
|
||
| //------------------------------------------------------------------------- | ||
| // DirectX 7.0 Checks | ||
| //------------------------------------------------------------------------- | ||
|
|
||
| // Check for DirectX 7 by creating a DDraw7 object | ||
| LPDIRECTDRAW7 pDD7; | ||
| DirectDrawCreateEx = (DIRECTDRAWCREATEEX)GetProcAddress(hDDrawDLL, | ||
| "DirectDrawCreateEx"); | ||
| if (NULL == DirectDrawCreateEx) | ||
| { | ||
| FreeLibrary(hDDrawDLL); | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| if (FAILED(DirectDrawCreateEx(NULL, (VOID**)&pDD7, IID_IDirectDraw7, | ||
| NULL))) | ||
| { | ||
| FreeLibrary(hDDrawDLL); | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| // DDraw7 was created successfully. We must be at least DX7.0 | ||
| dwDXVersion = 0x700; | ||
| pDD7->Release(); | ||
|
|
||
| //------------------------------------------------------------------------- | ||
| // DirectX 8.0 Checks | ||
| //------------------------------------------------------------------------- | ||
|
|
||
| // Simply see if D3D8.dll exists. | ||
| hD3D8DLL = LoadLibrary(L"D3D8.DLL"); | ||
| if (hD3D8DLL == NULL) | ||
| { | ||
| FreeLibrary(hDDrawDLL); | ||
| return dwDXVersion; | ||
| } | ||
|
|
||
| // D3D8.dll exists. We must be at least DX8.0 | ||
| dwDXVersion = 0x800; | ||
|
|
||
| //------------------------------------------------------------------------- | ||
| // DirectX 9.0 Checks | ||
| //------------------------------------------------------------------------- | ||
| hD3D9DLL = LoadLibrary(L"D3D9.DLL"); | ||
| if (hD3D9DLL == NULL) | ||
| { | ||
| FreeLibrary(hDDrawDLL); | ||
| FreeLibrary(hD3D8DLL); | ||
| return dwDXVersion; | ||
| } | ||
| dwDXVersion = 0x900; | ||
|
|
||
| //------------------------------------------------------------------------- | ||
| // End of checking for versions of DirectX | ||
| //------------------------------------------------------------------------- | ||
|
|
||
| // Close open libraries and return | ||
| FreeLibrary(hDDrawDLL); | ||
| FreeLibrary(hD3D8DLL); | ||
| FreeLibrary(hD3D9DLL); | ||
|
|
||
| return dwDXVersion; | ||
| } |
There was a problem hiding this comment.
This function GetDXVersion has a lot of code and nested if statements, which makes it hard to read and maintain. Consider refactoring it into smaller, more manageable functions.[^1]
At least it's organized and you can understand something.
------------------------VERsIOn.2.0🚀 ----------------------------