Fix nullptr errors in Windows VS builds #18
Conversation
WalkthroughThe pull request makes extensive modifications across project configuration files, dependency management scripts, external submodules, and source code. Updates include adding ignore entries for various GLFW libraries, introducing new submodules (GLM), and defining comprehensive Lua headers and configuration files. Changes in Visual Studio settings and CMake files add new build configurations for both x86 and x64 targets. In the source code, error handling, const-correctness, initialization order, and safety checks have been improved. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as EntryPoint::main
participant Engine as Engine::CreateApplication
participant App as SandboxApp
Main->>Engine: Call CreateApplication()
alt Successful creation
Engine->>App: Instantiate SandboxApp()
App-->>Engine: Return app pointer
Engine-->>Main: Return valid pointer
Main->>Main: Execute application
else Exception thrown
App-->>Engine: Exception raised during instantiation
Engine-->>Main: Return nullptr
Main->>Main: Log error and throw runtime_error
end
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
external/glfw/README.md (3)
8-13: Fix Markdown Unordered List Indentation.
Static analysis (MD007) indicates that these list items are indented with an extra space. Removing the extra indentation so that each bullet is flush left will improve markdown consistency.- - Visual C++ 2022 (built with 17.9.0) - - Visual C++ 2019 (built with 16.11.34) - - Visual C++ 2017 (built with 15.9.60) - - Visual C++ 2015 (built with 14.0.25431.01) - - Visual C++ 2013 (built with 12.0.40629.00) - - MinGW-w64 (built with 13.2.0-win32-dwarf-msvcrt) + - Visual C++ 2022 (built with 17.9.0) + - Visual C++ 2019 (built with 16.11.34) + - Visual C++ 2017 (built with 15.9.60) + - Visual C++ 2015 (built with 14.0.25431.01) + - Visual C++ 2013 (built with 12.0.40629.00) + - MinGW-w64 (built with 13.2.0-win32-dwarf-msvcrt)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
8-8: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
9-9: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
11-11: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
13-13: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
70-75: Grammar Correction Needed in Static Library Section.
The phrase “do not contain debug information” should be corrected to “does not contain debug information” for grammatical accuracy.- The library is built in release mode and do not contain debug information. + The library is built in release mode and does not contain debug information.
77-77: Remove Extraneous Trailing Line.
The line numbered "77" appears to be an unintended leftover. Remove it if it doesn’t serve a purpose.sandbox/src/SandboxApp.cpp (1)
11-11: Empty namespace is unnecessaryThis empty namespace declaration serves no purpose and can be removed.
-namespace Engine {}CMakeLists.txt (4)
10-12: Consider using target-based CMake for GLM.
While settingGLM_DIRmanually here is functional, modern CMake best practices suggest usingtarget_include_directories(on a target) rather than globalinclude_directories. This helps avoid polluting include paths for unrelated targets.
21-33: Unify GLFW handling across platforms.
Currently, Windows uses a manually specified path to glfw, while other platforms rely onfind_package(glfw3). This approach can become inconsistent. Consider providing a uniform workflow or allow an override to rely on system-installed glfw for Windows as well.
45-66: Verify library paths for Lua on Windows.
Using an imported library for Lua is fine, but ensure that debug vs. release library paths are handled appropriately and that the.dllis compatible with your build configuration. If needed, consider marking the imported target asIMPORTED GLOBALif it’s referenced in multiple subdirectories.
153-160: Confirm DLL copying for Lua.
Copyinglua54.dllis necessary for runtime, but verify that it exists at the specified location and whether a separate debug DLL is needed for debug builds. Consider adding checks or fallback logic to handle missing DLL scenarios gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
external/glfw/lib-vc2022/glfw3.dllis excluded by!**/*.dllexternal/lua/lua54.dllis excluded by!**/*.dll
📒 Files selected for processing (31)
.gitignore(1 hunks).gitmodules(1 hunks).vs/ProjectSettings.json(1 hunks).vs/VSWorkspaceState.json(2 hunks).vs/launch.vs.json(1 hunks)CMakeLists.txt(6 hunks)CMakePresets.json(1 hunks)CMakeSettings.json(1 hunks)CppProperties.json(1 hunks)external/glfw/LICENSE.md(1 hunks)external/glfw/README.md(1 hunks)external/glfw/include/GLFW/glfw3native.h(1 hunks)external/glm(1 hunks)external/lua/include/lauxlib.h(1 hunks)external/lua/include/lua.h(1 hunks)external/lua/include/lua.hpp(1 hunks)external/lua/include/luaconf.h(1 hunks)external/lua/include/lualib.h(1 hunks)external/lua/lua54.def(1 hunks)external/sol2/sol.hpp(2 hunks)imgui.ini(3 hunks)sandbox/src/SandboxApp.cpp(2 hunks)src/Application.cpp(1 hunks)src/Debug/Profiler.h(1 hunks)src/EntryPoint.h(1 hunks)src/Input/InputSystem.cpp(4 hunks)src/Input/InputSystem.h(1 hunks)src/UI/ImGuiOverlay.cpp(1 hunks)src/UI/ImGuiOverlay.h(1 hunks)src/Window/OpenGLWindow.cpp(1 hunks)src/pch.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (10)
- src/pch.cpp
- .vs/ProjectSettings.json
- external/lua/include/lua.hpp
- external/glm
- external/lua/include/luaconf.h
- .gitignore
- CppProperties.json
- external/sol2/sol.hpp
- external/glfw/LICENSE.md
- external/lua/lua54.def
🧰 Additional context used
🪛 Biome (1.9.4)
CMakePresets.json
[error] 1-1: Expected an array, an object, or a literal but instead found the end of the file.
Expected an array, an object, or a literal here.
(parse)
🪛 markdownlint-cli2 (0.17.2)
external/glfw/README.md
8-8: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
9-9: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
11-11: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
13-13: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze with SonarQube
🔇 Additional comments (39)
external/glfw/README.md (7)
1-7: Clear and Informative Introduction.
The header and the introductory lines clearly state that these are the GLFW binaries for 32-bit Windows and list what the archive contains.
14-20: Clear Deprecation Notice for Original MinGW.
The explanation regarding the discontinuation of the original MinGW distribution is detailed and helps direct users to use MinGW-w64 instead.
21-27: Well-Outlined Visual C++ Compatibility.
The section detailing Visual C++ compatibility with Windows XP, along with future deprecation notes, is clear and informative.
28-33: Concise Guidelines for Using GLFW as a DLL (Visual C++).
The instructions to link againstglfw3dll.libfor DLL usage are clear and provide the necessary context for users.
34-38: Additional DLL Configuration Details.
The note about the alternate DLL and import library available in thelib-static-ucrtdirectory is useful for Visual C++ 2019 users.
40-45: Detailed Instructions for Static Library Linking.
The directions for linking againstglfw3.liborglfw3_mt.libdepending on the runtime are accurate and helpful.
52-68: Comprehensive Guidance for MinGW-w64 Configurations.
Both the DLL and static library sections for MinGW-w64 are well documented, ensuring developers have clear instructions regardless of their preferred linking method..gitmodules (1)
5-7: GLM library integration looks goodThe addition of the GLM library as a submodule is correctly configured with the appropriate repository URL.
src/Input/InputSystem.h (1)
128-128: Good const-correctness improvementAdding the
constqualifier toHandleSpeedModifiersis a best practice that ensures the method doesn't modify class state. This improves code clarity and helps prevent unintended modifications.src/UI/ImGuiOverlay.h (1)
46-46: Good improvement to const-correctness.Adding
constto theRenderProfilermethod declaration is appropriate since it doesn't modify any member variables. This allows the method to be called on const instances of the class and follows C++ best practices.src/UI/ImGuiOverlay.cpp (1)
166-166: Proper implementation of const method.The method implementation correctly matches the declaration with the
constqualifier. The method doesn't modify any member variables, confirming that the const qualifier is appropriate.src/Application.cpp (2)
75-79: Good initialization and error handling improvement.The
InputSystemis now initialized earlier in the constructor with proper null check. This helps catch initialization issues promptly and provides clear error logging.
83-95: Better initialization sequence and error handling.Moving the
ImGuiOverlayinitialization next to theImGuiLayerinitialization improves code organization. The added null checks with appropriate fatal error messages enhance error detection and reporting.imgui.ini (3)
37-38: Updated ImGui window positioning system.Changing from
PostoViewportPosand addingViewportIdupdates the window positioning to work with ImGui's multi-viewport system, which is important for proper window handling in GLFW.
55-56: Consistent viewport positioning update.The "Recent Events" window positioning has been properly updated to use the viewport-based positioning system, maintaining consistency with other windows.
68-69: Proper viewport configuration for Lua Console.The Lua Console window has been updated to use the viewport positioning system, ensuring consistent window management across the application.
sandbox/src/SandboxApp.cpp (1)
194-203: Good error handling improvementThe try-catch block correctly prevents exceptions from propagating out of application creation and returns nullptr on failure, which fixes potential nullptr errors in Windows VS builds.
src/EntryPoint.h (2)
10-11: Good addition of required headersThese headers are necessary for the exception handling code added below.
16-37: Excellent error handling implementationThis comprehensive error handling implementation properly:
- Checks if the application creation returned nullptr
- Handles standard exceptions with specific error messages
- Catches any unexpected exceptions with a fallback message
- Returns appropriate error codes
This complements the nullptr handling in CreateApplication().
.vs/launch.vs.json (1)
5-15: Good addition of x64-Debug configurationThis configuration properly sets up the Visual Studio debugger to work with the x64-Debug build of the application. The PATH environment variable modification ensures runtime libraries are found correctly.
src/Input/InputSystem.cpp (5)
19-23: Great addition of null pointer check.Adding this null pointer check in the InputSystem constructor prevents potential crashes if a null window pointer is passed. This aligns perfectly with the PR objective of fixing nullptr errors.
87-87: Good improvement to const-correctness.Making
HandleSpeedModifiersa const method correctly indicates that it doesn't modify the InputSystem's state, improving const-correctness of the codebase.
98-98: Performance improvement using reference instead of copy.Changing from a copy to a reference avoids unnecessary copying of the camera object and prevents potential null dereference issues.
105-105: Performance improvement using reference instead of copy.Using a reference instead of a copy for
perspCamerais more efficient and safer, as it avoids unnecessary copying while preserving the pointer semantics.
176-176: Improved reference capture in lambda.Changing to reference capture for the perspCamera in the lambda expression ensures that we're working with the current state of the perspCamera and avoids any potential nullptr issues.
CMakeSettings.json (1)
23-47: Good addition of x86 build configurations.Adding x86 build configurations (both Debug and Release) is a valuable improvement that will help identify and prevent platform-specific issues, particularly null pointer errors that might occur differently across architectures.
The configurations are well-structured with appropriate settings for the x86 architecture and match the pattern of the existing x64 configurations.
external/lua/include/lualib.h (1)
1-59: Well-structured Lua standard libraries header.The addition of this header file provides proper declarations for Lua standard libraries, which is crucial for correctly interfacing with Lua. The file includes appropriate header guards, version suffix definitions, and function declarations with correct parameter types.
This will help prevent null pointer issues when working with Lua state objects by ensuring all functions are properly declared before use.
.vs/VSWorkspaceState.json (1)
3-36: Updated VSWorkspaceState for new build configurations.The workspace state has been properly updated to reflect the new build configurations and output folders for both x64 and x86 targets. This ensures Visual Studio correctly handles the project structure and builds, which supports the PR's objective of fixing Windows VS build issues.
CMakeLists.txt (4)
82-82: Confirm if pch.cpp is correctly configured.
Addingsrc/pch.cppto your engine sources is good, but make sure that precompiled headers are actually being generated and consumed as intended, especially on different build platforms.
139-139: Conditional usage of GLFW library.
The$<IF:$<BOOL:${WIN32}>,${GLFW_LIBRARY},glfw>generator expression is a valid method. No immediate concerns, assuming the non-Windows path always has “glfw” available.
142-142: Linking Lua libraries.
Linking with${LUA_LIBRARIES}is straightforward. Ensure correct matching of static/dynamic library types across your build.
162-168: Check side effects of enabling Edit & Continue.
Overriding/Ziwith/ZIcan cause minimal runtime overhead in debug. Typically safe, but confirm it doesn’t conflict with any external libraries or tooling in your pipeline.external/glfw/include/GLFW/glfw3native.h (1)
1-664: Treat this officially sourced code as read-only.
This appears to be the standardglfw3native.hfrom GLFW, introducing platform-specific native access APIs. Direct modifications risk diverging from official GLFW updates and may introduce maintenance overhead. If no local patches are needed, keep it synchronized with the upstream version.external/lua/include/lauxlib.h (1)
1-277: Standard Lua auxiliary library header.
This file is part of Lua’s codebase (auxiliary library). Typically, it should remain unmodified to stay compatible with official Lua updates. Avoid local changes unless absolutely necessary, as it may complicate future upgrades or patching.external/lua/include/lua.h (5)
1-6: Standard license header
These lines contain standard Lua header comments and license details. No issues here.
19-21: Confirm Lua version usage
These version macros declare Lua 5.4.2. Ensure this version aligns with your project's requirements and any dependencies.Would you like to verify that all dependent modules and scripts are compatible with Lua 5.4.2?
153-158: Lua state management declarations
The function prototypeslua_newstate,lua_close,lua_newthread, andlua_resetthreadmatch the standard Lua C API. They appear correct and well-structured for cross-platform use.
360-364: Macro clarity
The helper macros (e.g.,lua_pop,lua_newtable, etc.) are concise and improve code readability. They correctly wrap existing Lua API calls.
493-514: MIT License notice
This section accurately reflects Lua’s license terms. No additional modifications are necessary.
| // Ensure lru_idx is within bounds | ||
| if (lru_idx < FAST_PATH_SIZE) { | ||
| entries[lru_idx] = {name, data, accessCount}; | ||
| } |
There was a problem hiding this comment.
Critical safety fix: Added bounds check
This bounds check prevents potential buffer overflow when accessing the entries array with lru_idx, which could have caused memory corruption or crashes.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Analysis Details0 Issues
Coverage and Duplications
Project ID: arozx_voxels_d0e4bd76-3798-45bc-a3e0-f1357e3eed04 |
Docstrings generation was requested by @arozx. * #18 (comment) The following files were modified: * `sandbox/src/SandboxApp.cpp` * `src/Application.cpp` * `src/Debug/Profiler.h` * `src/EntryPoint.h` * `src/Input/InputSystem.cpp` * `src/UI/ImGuiOverlay.cpp` * `src/UI/ImGuiOverlay.h` * `src/Window/OpenGLWindow.cpp`
|
Note Generated docstrings for this pull request at #19 |
Docstrings generation was requested by @arozx. * #18 (comment) The following files were modified: * `sandbox/src/SandboxApp.cpp` * `src/Application.cpp` * `src/Debug/Profiler.h` * `src/EntryPoint.h` * `src/Input/InputSystem.cpp` * `src/UI/ImGuiOverlay.cpp` * `src/UI/ImGuiOverlay.h` * `src/Window/OpenGLWindow.cpp` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores