Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Repositories] Add more precise types to repository generator #2391

Merged
merged 9 commits into from Aug 31, 2022

Conversation

mackal
Copy link
Member

@mackal mackal commented Aug 22, 2022

datatypes

This adds support for unsigned and more integer types. It also avoids
using parsing functions that require casting (still needed in some
cases)

Having the data types in the Repository structs better map to the types
in the database will allow us to avoid casting when we pull data out of
them. And as a benefit, assume something is wrong if we do :)

Hopefully clean up some warnings due to casting too.

@mackal mackal requested a review from Akkadius August 22, 2022 03:50
@Akkadius
Copy link
Member

I'd regenerate all repos with these changes and we need to go through testing

@Akkadius
Copy link
Member

Regenerated all base repos

@Akkadius
Copy link
Member

Build fail

FAILED: common/CMakeFiles/common.dir/shared_tasks.cpp.o 
ccache /usr/bin/c++  -DBOTS -DBUILD_LOGGING -DCOMMANDS_LOGGING -DCPPHTTPLIB_OPENSSL_SUPPORT -DEMBPERL -DEMBPERL_PLUGIN -DENABLE_SECURITY -DEQEMU_USE_OPENSSL -DGLM_ENABLE_EXPERIMENTAL -DGLM_FORCE_CTOR_INIT -DGLM_FORCE_RADIANS -DHAS_UNION_SEMUN -DLUA_EQEMU -DSANITIZE_LUA_LIBS -I../common/Patches -I../common/SocketLib -I../common/StackWalker -isystem libs/zlibng -isystem /usr/include/mysql -isystem ../libs/zlibng -isystem ../submodules/glm -isystem ../submodules/cereal/include -isystem ../submodules/fmt/include -isystem ../submodules/libuv/include -isystem ../submodules/recastnavigation/DebugUtils/Include -isystem ../submodules/recastnavigation/Detour/Include -isystem ../submodules/recastnavigation/DetourCrowd/Include -isystem ../submodules/recastnavigation/DetourTileCache/Include -isystem ../submodules/recastnavigation/Recast/Include -isystem ../submodules/websocketpp -isystem /usr/include/lua5.1 -isystem ../libs/luabind -isystem /usr/lib/x86_64-linux-gnu/perl/5.28/CORE -O0 -g -DNDEBUG   -std=c++17 -MD -MT common/CMakeFiles/common.dir/shared_tasks.cpp.o -MF common/CMakeFiles/common.dir/shared_tasks.cpp.o.d -o common/CMakeFiles/common.dir/shared_tasks.cpp.o -c ../common/shared_tasks.cpp
../common/shared_tasks.cpp: In static member function 'static SharedTaskRequest SharedTask::GetRequestCharacters(Database&, uint32_t)':
../common/shared_tasks.cpp:95:73: error: no matching function for call to 'min(int&, const uint32_t&)'
   request.lowest_level  = std::min(request.lowest_level, character.level);
                                                                         ^
In file included from /usr/include/c++/8/bits/char_traits.h:39,
                 from /usr/include/c++/8/ios:40,
                 from /usr/include/c++/8/ostream:38,
                 from /usr/include/c++/8/iostream:39,
                 from ../common/eqemu_logsys.h:24,
                 from ../common/database.h:25,
                 from ../common/shared_tasks.h:4,
                 from ../common/shared_tasks.cpp:1:
/usr/include/c++/8/bits/stl_algobase.h:195:5: note: candidate: 'template<class _Tp> constexpr const _Tp& std::min(const _Tp&, const _Tp&)'
     min(const _Tp& __a, const _Tp& __b)
     ^~~
/usr/include/c++/8/bits/stl_algobase.h:195:5: note:   template argument deduction/substitution failed:
../common/shared_tasks.cpp:95:73: note:   deduced conflicting types for parameter 'const _Tp' ('int' and 'uint32_t' {aka 'unsigned int'})
   request.lowest_level  = std::min(request.lowest_level, character.level);

@Akkadius
Copy link
Member

Akkadius commented Aug 22, 2022

This will have a lot of ramifications for creating more explicit types - the code will need to be worked through to accommodate the more explicit types.

Anytime we make changes to the base repository generator we should be regenning all of the base repositories.

@Akkadius Akkadius marked this pull request as draft August 22, 2022 05:23
@mackal
Copy link
Member Author

mackal commented Aug 22, 2022

diff --git a/common/shared_tasks.cpp b/common/shared_tasks.cpp
index 894f9c280..d9cb1d4d3 100644
--- a/common/shared_tasks.cpp
+++ b/common/shared_tasks.cpp
@@ -92,8 +92,8 @@ SharedTaskRequest SharedTask::GetRequestCharacters(Database &db, uint32_t reques
 	request.members.reserve(characters.size());
 	for (const auto& character: characters)
 	{
-		request.lowest_level  = std::min(request.lowest_level, character.level);
-		request.highest_level = std::max(request.highest_level, character.level);
+		request.lowest_level  = std::min(request.lowest_level, static_cast<int32_t>(character.level));
+		request.highest_level = std::max(request.highest_level, static_cast<int32_t>(character.level));
 		request.character_ids.emplace_back(character.id); // convenience
 
 		SharedTaskMember member = {};
diff --git a/common/tasks.h b/common/tasks.h
index 62f537668..9f8f5684a 100644
--- a/common/tasks.h
+++ b/common/tasks.h
@@ -368,7 +368,7 @@ namespace Tasks {
 			if (activity_states[i].activity_state != ActivityCompleted)
 			{
 				completed_ids[i] = false;
-				current_step = std::min(current_step, el.step);
+				current_step = std::min(current_step, static_cast<int>(el.step));
 				if (!el.optional)
 				{
 					result.is_task_complete = false;
diff --git a/world/shared_task_manager.cpp b/world/shared_task_manager.cpp
index 4ea3866a7..e4ef008cf 100644
--- a/world/shared_task_manager.cpp
+++ b/world/shared_task_manager.cpp
@@ -1460,8 +1460,8 @@ bool SharedTaskManager::CanAddPlayer(SharedTask *s, uint32_t character_id, std::
 		int highest_level = cle->level();
 
 		for (const auto &character : characters) {
-			lowest_level  = std::min(lowest_level, character.level);
-			highest_level = std::max(highest_level, character.level);
+			lowest_level  = std::min(lowest_level, static_cast<int32_t>(character.level));
+			highest_level = std::max(highest_level, static_cast<int32_t>(character.level));
 		}
 
 		if ((highest_level - lowest_level) > s->GetTaskData().level_spread) {

So I figured I would get it compiling cleanly via casts just to see how many issues have appeared. These are the immediate blockers at least. I would think changing task_activities.step to a signed int would actually be the best fit here. The character level is a bit more not obvious. The client uses both int and uint8_t to represent level in a different data structures and I'm not 100% sure which that table most closely matches :P

Either way, I think @hgtw might want to comment

@hgtw
Copy link
Contributor

hgtw commented Aug 22, 2022

I have no issue with either changing task_activities.step to signed or changing the structs to unsigned. I guess I'd lean more towards just using int but your call. Since that method is templated to (hackily) support the different zone and world structs they just need to match to avoid casting.

I guess for levels it'd be easier to just change the local request variables to uint32_t to match signs in character_data, though realistically int will hold all player levels. I don't really have an issue with either choice

@mackal
Copy link
Member Author

mackal commented Aug 23, 2022

I'm aware there is still 1 more issue to solve for this, I just haven't gotten around to it.

@mackal mackal marked this pull request as ready for review August 25, 2022 01:55
@Akkadius Akkadius changed the title Make utils/scripts/generators/repository-generator.pl aware of more [Repositories] Add more precise types to repository generator Aug 31, 2022
@Akkadius Akkadius force-pushed the feature/repository-generator-datatypes branch from ccef6e4 to e90bc6a Compare August 31, 2022 04:28
mackal and others added 7 commits August 30, 2022 23:33
datatypes

This adds support for unsigned and more integer types. It also avoids
using parsing functions that require casting (still needed in some
cases)

Having the data types in the Repository structs better map to the types
in the database will allow us to avoid casting when we pull data out of
them. And as a benefit, assume something is wrong if we do :)

Hopefully clean up some warnings due to casting too.
With default clang warning settings these were rather noisy, so let's
use the new script for them already.
We may decide to change this at a later date depending if we change the
character table.

World usage I think it makes sense for CLE to have uint8_t, but we'll
still cast for now
@Akkadius Akkadius force-pushed the feature/repository-generator-datatypes branch from e90bc6a to a2d5c49 Compare August 31, 2022 04:35
@Akkadius Akkadius merged commit 6f7fa98 into master Aug 31, 2022
@Akkadius Akkadius deleted the feature/repository-generator-datatypes branch August 31, 2022 05:04
mackal added a commit that referenced this pull request Sep 3, 2022
* Make utils/scripts/generators/repository-generator.pl aware of more
datatypes

This adds support for unsigned and more integer types. It also avoids
using parsing functions that require casting (still needed in some
cases)

Having the data types in the Repository structs better map to the types
in the database will allow us to avoid casting when we pull data out of
them. And as a benefit, assume something is wrong if we do :)

Hopefully clean up some warnings due to casting too.

Co-authored-by: Akkadius <akkadius1@gmail.com>
joligario added a commit to ProjectEQ/peqphpeditor that referenced this pull request Sep 6, 2022
catapultam-habeo pushed a commit to catapultam-habeo/pyrelight-server that referenced this pull request Mar 27, 2023
…2391)

* Make utils/scripts/generators/repository-generator.pl aware of more
datatypes

This adds support for unsigned and more integer types. It also avoids
using parsing functions that require casting (still needed in some
cases)

Having the data types in the Repository structs better map to the types
in the database will allow us to avoid casting when we pull data out of
them. And as a benefit, assume something is wrong if we do :)

Hopefully clean up some warnings due to casting too.

Co-authored-by: Akkadius <akkadius1@gmail.com>
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.

None yet

3 participants