Skip to content

move prints to console out of help_db get_commands() #5622

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

move prints to console out of help_db get_commands() #5622

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c703f3b
move prints to console out of help_db get_commands()
dhthwy Nov 11, 2025
1b460ed
fix compile.
dhthwy Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions library/Core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ distribution.
#include <type_traits>
#include <cstdarg>
#include <filesystem>
#include <optional>
#include <SDL_events.h>

#ifdef _WIN32
Expand Down Expand Up @@ -416,36 +417,39 @@ bool is_builtin(color_ostream &con, const std::string &command) {
return lua_toboolean(L, -1);
}

void get_commands(color_ostream &con, std::vector<std::string> &commands) {
std::optional<std::string> get_commands(color_ostream &con, std::vector<std::string> &commands) {
Copy link
Member

@ab9rf ab9rf Nov 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function currently uses an out parameter - i would argue that this is a use case for returning a std::expected<std::vector<std::string>>, std::string> instead of having an out parameter

oh, nevermind, std::expected is c++23, so let's notate this for review when we move to c++23 -- which won't be prior to 2029 unless we decide to change our Linux support policy - while MSVC already supports std::expected, gcc doesn't until gcc 14, and we don't get gcc 14 until Ubuntu LTS 24.04 goes EOL in April 2029

Copy link
Member

@ab9rf ab9rf Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking a bit more about this, i think this is a candidate for using an exception. the condition is exceptional and should rarely occur, so using an exception for the exceptional case is pretty much what exceptions are for

Copy link
Contributor Author

@dhthwy dhthwy Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

DFHack]# o
o is not recognized. Possible completions: once-per-save on-new-fortress open-legends orders overlay
[DFHack]# o
Cannot acquire core lock in helpdb.get_commands
o is not a recognized command.
[DFHack]# o

that's dfhack posix console. There's contention when fps is low.

Copy link
Contributor Author

@dhthwy dhthwy Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing is one possible way, but they should be caught. It wouldn't be nice to crash just because the simulation thread has a big workload that's dropped fps to 10 or happened to hold the lock for too long for some reason or another. If it happens at 10 fps (and often), it's sure to happen at higher fps (just less likely).

Copy link
Contributor Author

@dhthwy dhthwy Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not big on the std::optional approach for returning an error message.

std::expected would be cleaner, but as you said, it's not c++20.

Exception makes sense to handle it as well. I just wouldn't let it crash over a lag condition.

ConditionalCoreSuspender suspend{};

if (!suspend) {
con.printerr("Cannot acquire core lock in helpdb.get_commands\n");
commands.clear();
return;
return "Cannot acquire core lock in helpdb.get_commands";
}

auto L = DFHack::Core::getInstance().getLuaState();
Lua::StackUnwinder top(L);

if (!lua_checkstack(L, 1) ||
!Lua::PushModulePublic(con, L, "helpdb", "get_commands")) {
con.printerr("Failed to load helpdb Lua code\n");
return;
return "Failed to load helpdb Lua code";
}

if (!Lua::SafeCall(con, L, 0, 1)) {
con.printerr("Failed Lua call to helpdb.get_commands.\n");
return "Failed Lua call to helpdb.get_commands.";
}

Lua::GetVector(L, commands, top + 1);
return std::nullopt;
}

static bool try_autocomplete(color_ostream &con, const std::string &first, std::string &completed)
{
std::vector<std::string> commands, possible;

get_commands(con, commands);
if (auto err = get_commands(con, commands)) {
con.printerr("%s\n", err->c_str());
return false;
}

for (auto &command : commands)
if (command.substr(0, first.size()) == first)
possible.push_back(command);
Expand Down