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

Add file and line to NodeDef field deprecation warning #6708

Conversation

rubenwardy
Copy link
Member

@rubenwardy rubenwardy commented Nov 29, 2017

WARNING[Main]: Field dug_item: Deprecated; use 'drop' field (at ...netest/bin/../games/minetest_game/mods/default/nodes.lua:1668)

Replaces #3957

@rubenwardy
Copy link
Member Author

rubenwardy commented Nov 29, 2017

For some reason doing this results in Minetest printing out the wrong line

function foo()
    minetest.get_mapgen_params() -- should be this line
end 

foo() -- this line is given

Edit: this is present in master too???

return "error-badstack:0";

if (!lua_getinfo(L, "Sl", &ar))
return "error-badgetinfo:0";
Copy link
Member

@sfan5 sfan5 Nov 29, 2017

Choose a reason for hiding this comment

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

my brain parses this as badget info
no but wouldn't <invalid>:0 suffice in as error value?

{
lua_Debug ar;

if (!lua_getstack(L, 2 + offset, &ar))
Copy link
Member

Choose a reason for hiding this comment

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

there might be a case where you want the actual calling function, i propose 1 + offset

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a reason your code uses 2 rather than 1 here? It prints out the wrong position as detailed above

Copy link
Member

@sfan5 sfan5 Dec 1, 2017

Choose a reason for hiding this comment

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

since log_deprecated is called by a wrapper function through core.log() you'll want to get the function that called the wrapper, not the wrapper itself....
okay this turns out to be trickier as log_deprecated is also called directly from C++ in some cases

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that explains it. I'll have to pass an offset into log_deprecated and default to 0, unfortunately

@paramat paramat added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Dec 10, 2017
@rubenwardy rubenwardy force-pushed the feature/addFileLineToNodeDefDeprecation branch from 4eb37f2 to 26af74b Compare December 10, 2017 19:44
WARNING[Main]: Field dug_item: Deprecated; use 'drop' field (at ...netest/bin/../games/minetest_game/mods/default/nodes.lua:1668)
@rubenwardy rubenwardy force-pushed the feature/addFileLineToNodeDefDeprecation branch from 26af74b to ade7515 Compare December 10, 2017 19:59
@rubenwardy
Copy link
Member Author

So, currently it'll say that there's a deprecation in builtin/game/register.lua if you use a deprecate item def field. The only thing that can be done now is to manually crawl through the back stack to find the first element which isn't builtin

if (!lua_getinfo(L, "Sl", &ar))
return "error-badgetinfo:0";

return std::string(ar.short_src) + ":" + std::to_string(ar.currentline);
Copy link
Member

Choose a reason for hiding this comment

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

use append operator instead of + to prevent multiple memory string copy

@rubenwardy rubenwardy added Blocker The issue needs to be addressed before the next release. High priority Bugfix 🐛 PRs that fix a bug and removed Low priority Blocker The issue needs to be addressed before the next release. labels Mar 28, 2018
@rubenwardy
Copy link
Member Author

What should be done here? This PR fixes a crash when calling a deprecated field, however the location it gives may not be accurate.

I'm not sure how to properly pass in the path to builtin to work out where in the stack the last entry before builtin. I could instead try and find builtin in the path, but this will break if any mods or directories in mods are called builtin.

This could be merged as-is, as it's still more accurate than the current code on master and doesn't crash

if (!lua_getinfo(L, "Sl", &ar))
return "error-badgetinfo:0";

return std::string(ar.short_src) + ":" + std::to_string(ar.currentline);
Copy link
Member

Choose a reason for hiding this comment

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

use append function instead of "+"

@@ -132,7 +145,7 @@ void script_run_callbacks_f(lua_State *L, int nargs,
lua_remove(L, error_handler);
}

void log_deprecated(lua_State *L, const std::string &message)
void log_deprecated(lua_State *L, const std::string &message, unsigned int offset)
Copy link
Member

Choose a reason for hiding this comment

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

u32

@rubenwardy rubenwardy added Blocker The issue needs to be addressed before the next release. and removed High priority labels Jun 7, 2018
@paramat paramat added this to the 0.5.0 milestone Jun 7, 2018
@rubenwardy rubenwardy added Low priority and removed Blocker The issue needs to be addressed before the next release. labels Jun 14, 2018
@rubenwardy rubenwardy removed this from the 0.5.0 milestone Jun 14, 2018
@SmallJoker SmallJoker added the Rebase needed The PR needs to be rebased by its author. label Jun 19, 2018
@rubenwardy rubenwardy closed this Dec 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) Bugfix 🐛 PRs that fix a bug Low priority Rebase needed The PR needs to be rebased by its author. @ Script API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants