xNVSE / NVSE

New Vegas Script Extender (NVSE)
https://git.io/JfSAo
702 stars 55 forks source link

Make `ScriptToken*`-generators return unique_ptr instead. #109

Closed Demorome closed 2 years ago

Demorome commented 2 years ago

Should make it clearer to the caller that the resource returned has its ownership transferred, and should help avoid memory leaks like what happened with Cmd_Ternary.

Also applied some misc changes using ReSharper, like adding const and override, using nullptr instead of NULL, etc.

korri123 commented 2 years ago

Before merging this we need to look into seeing if there are any performance changes. I know unique_ptr with make_unique should in theory have no performance difference but I remember way back when we first took over NVSE I experimented with replacing the manual allocations with unique_ptr in ExpressionEvaluator::Evaluate and there were some performance regressions. I suspect that was because I may have been wrapping already newed instances in unique_ptr and if I had done it properly with make_unique it wouldn't have mattered but it made me vary of refactoring everything to use it still.

Good change though, just feel we need to verify it first. Though in these cases I don't think the performance impact will be noticeable at all as the code sections aren't particular hotspots like ExpressionEvaluator::Evaluate.

korri123 commented 2 years ago

Actually nevermind, should be fine.