From f375f20ffcbf15089d35aa0e2dffbd01d7024f90 Mon Sep 17 00:00:00 2001 From: ChillerDragon Date: Tue, 30 Jan 2024 11:16:46 +0800 Subject: [PATCH] Fix clang warning and UB when there is no game layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit clang 14.0.6 shows this: ``` [ 98%] Building CXX object CMakeFiles/game-client.dir/src/game/generated/client_data7.cpp.o In file included from /usr/include/c++/12/bits/stl_tempbuf.h:60, from /usr/include/c++/12/bits/stl_algo.h:61, from /usr/include/c++/12/algorithm:61, from /home/chiller/Desktop/git/ddnet/src/base/math.h:6, from /home/chiller/Desktop/git/ddnet/src/base/color.h:5, from /home/chiller/Desktop/git/ddnet/src/game/editor/popups.cpp:4: In function ‘void std::_Construct(_Tp*, _Args&& ...) [with _Tp = CEditorActionTileChanges; _Args = {CEditor*&, int&, int&, const char (&)[20], map, allocator > >, less, allocator, allocator > > > > >&}]’, inlined from ‘static void std::allocator_traits >::construct(allocator_type&, _Up*, _Args&& ...) [with _Up = CEditorActionTileChanges; _Args = {CEditor*&, int&, int&, const char (&)[20], std::map, std::allocator > >, std::less, std::allocator, std::allocator > > > > >&}]’ at /usr/include/c++/12/bits/alloc_traits.h:635:19, inlined from ‘std::_Sp_counted_ptr_inplace<_Tp, _Alloc, _Lp>::_Sp_counted_ptr_inplace(_Alloc, _Args&& ...) [with _Args = {CEditor*&, int&, int&, const char (&)[20], std::map, std::allocator > >, std::less, std::allocator, std::allocator > > > > >&}; _Tp = CEditorActionTileChanges; _Alloc = std::allocator; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic]’ at /usr/include/c++/12/bits/shared_ptr_base.h:604:39, inlined from ‘std::__shared_count<_Lp>::__shared_count(_Tp*&, std::_Sp_alloc_shared_tag<_Alloc>, _Args&& ...) [with _Tp = CEditorActionTileChanges; _Alloc = std::allocator; _Args = {CEditor*&, int&, int&, const char (&)[20], std::map, std::allocator > >, std::less, std::allocator, std::allocator > > > > >&}; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic]’ at /usr/include/c++/12/bits/shared_ptr_base.h:971:16, inlined from ‘std::__shared_ptr<_Tp, _Lp>::__shared_ptr(std::_Sp_alloc_shared_tag<_Tp>, _Args&& ...) [with _Alloc = std::allocator; _Args = {CEditor*&, int&, int&, const char (&)[20], std::map, std::allocator > >, std::less, std::allocator, std::allocator > > > > >&}; _Tp = CEditorActionTileChanges; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic]’ at /usr/include/c++/12/bits/shared_ptr_base.h:1712:14, inlined from ‘std::shared_ptr<_Tp>::shared_ptr(std::_Sp_alloc_shared_tag<_Tp>, _Args&& ...) [with _Alloc = std::allocator; _Args = {CEditor*&, int&, int&, const char (&)[20], std::map, std::allocator > >, std::less, std::allocator, std::allocator > > > > >&}; _Tp = CEditorActionTileChanges]’ at /usr/include/c++/12/bits/shared_ptr.h:464:59, inlined from ‘std::shared_ptr >::value), _Tp>::type> std::make_shared(_Args&& ...) [with _Tp = CEditorActionTileChanges; _Args = {CEditor*&, int&, int&, const char (&)[20], map, allocator > >, less, allocator, allocator > > > > >&}]’ at /usr/include/c++/12/bits/shared_ptr.h:1010:39, inlined from ‘static CUI::EPopupMenuFunctionResult CEditor::PopupGroup(void*, CUIRect, bool)’ at /home/chiller/Desktop/git/ddnet/src/game/editor/popups.cpp:450:85: /usr/include/c++/12/bits/stl_construct.h:119:7: warning: ‘GameLayerIndex’ may be used uninitialized [-Wmaybe-uninitialized] 119 | ::new((void*)__p) _Tp(std::forward<_Args>(__args)...); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/chiller/Desktop/git/ddnet/src/game/editor/popups.cpp: In static member function ‘static CUI::EPopupMenuFunctionResult CEditor::PopupGroup(void*, CUIRect, bool)’: /home/chiller/Desktop/git/ddnet/src/game/editor/popups.cpp:408:29: note: ‘GameLayerIndex’ was declared here 408 | int GameLayerIndex; | ^~~~~~~~~~~~~~ At global scope: cc1plus: note: unrecognized command-line option ‘-Wno-nullability-completeness’ may have been intended to silence earlier diagnostics ``` Which makes sense. GameLayerIndex is only set in a loop if a condition is met. And then its value is recorded for the undo feature. The condition should never be false. Because a game layer is a strict requirement for a functional map. But I still decided to avoid using an assert or silent ignore. If this breaks logs would be nice. If this breaks the editor should not crash. The editor should never crash or quit to not lose unsaved changes. --- src/game/editor/popups.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/game/editor/popups.cpp b/src/game/editor/popups.cpp index aad82f04f..3e367944a 100644 --- a/src/game/editor/popups.cpp +++ b/src/game/editor/popups.cpp @@ -405,7 +405,7 @@ CUI::EPopupMenuFunctionResult CEditor::PopupGroup(void *pContext, CUIRect View, { // gather all tile layers std::vector> vpLayers; - int GameLayerIndex; + int GameLayerIndex = -1; for(int LayerIndex = 0; LayerIndex < (int)pEditor->m_Map.m_pGameGroup->m_vpLayers.size(); LayerIndex++) { auto &pLayer = pEditor->m_Map.m_pGameGroup->m_vpLayers.at(LayerIndex); @@ -446,8 +446,15 @@ CUI::EPopupMenuFunctionResult CEditor::PopupGroup(void *pContext, CUIRect View, if(!pGameLayer->m_TilesHistory.empty()) { - // record undo - pEditor->m_EditorHistory.RecordAction(std::make_shared(pEditor, pEditor->m_SelectedGroup, GameLayerIndex, "Clean up game tiles", pGameLayer->m_TilesHistory)); + if(GameLayerIndex == -1) + { + dbg_msg("editor", "failed to record action (GameLayerIndex not found)"); + } + else + { + // record undo + pEditor->m_EditorHistory.RecordAction(std::make_shared(pEditor, pEditor->m_SelectedGroup, GameLayerIndex, "Clean up game tiles", pGameLayer->m_TilesHistory)); + } pGameLayer->ClearHistory(); }