zep icon indicating copy to clipboard operation
zep copied to clipboard

Potential problems with Zep::ZepTheme::GetColor() API change

Open samhocevar opened this issue 4 years ago • 1 comments

Describe the bug When integrating a more recent version of Zep, I was hit by a bug where my whole editor went black. This did not happen when compiling in debug mode, which made it a bit tricky to track down. I finally realised it was because Zep::ZepTheme::GetColor now returned a Zep::NVec4f const & (as opposed to a Zep::NVec4f previously), so when overriding that method I had to make sure I did not return a reference to a temporary because the compiler was (rightfully) assuming I did not care about the returned value.

I studied bf53e9cb9563d3d67d555bf32bf2a4c1d8f993d2 and could not understand a reason behind returning a reference. From what I found so far, the returned value is always eventually copied to a Zep::Nvec4f.

This is of course a very common mistake that every C++ programmer should avoid, but it is easily overlooked and I realise that the copiler did not warn me about it.

Also there is currently a real problem in ZepTheme due to that: since SetLightTheme() allocates more colors than SetDarkTheme(), any value returned by GetColor may be invalidated by a call to SetLightTheme().

My personal issue is solved but I’m still suggesting reverting the API change to avoid surprises for other users.

samhocevar avatar Mar 08 '20 21:03 samhocevar

OK, that makes sense; I was really just trying to save time here, since GetColor is called all the time during display. I didn't intend for the color to be kept around for very long; so didn't think about the Theme stuff; I'll revert it. I still have work to do to make the themes cleaner.

cmaughan avatar Mar 08 '20 22:03 cmaughan

I've decided to leave the const& return; it saves a lot of copying around of data when drawing the window. I have also fixed the mismatch in style counts.

cmaughan avatar Mar 24 '23 14:03 cmaughan