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

Review and ideally remediate old style C string handling #6730

Open
22 of 38 tasks
baconpaul opened this issue Dec 4, 2022 · 1 comment
Open
22 of 38 tasks

Review and ideally remediate old style C string handling #6730

baconpaul opened this issue Dec 4, 2022 · 1 comment
Labels
Code Refactoring General code refactoring and cleanup issues like names, unused variables, warnings, fixme
Milestone

Comments

@baconpaul
Copy link
Collaborator

baconpaul commented Dec 4, 2022

We have std::string and we have fmt::format

There's a few places where we have to use char * but nowhere near as many as we do.

snprintf

  • src/common/Parameter.cpp: 246
  • src/common/SurgeSynthesizer.cpp: 9
  • src/common/UnitConversions.h: 1
  • src/common/PatchDB.cpp: 2
  • src/common/SurgePatch.cpp: 4
  • src/common/StringOps.h: 2
  • src/common/dsp/modulators/FormulaModulationHelper.cpp: 6
  • src/common/dsp/oscillators/TwistOscillator.cpp: 2
  • src/common/dsp/utilities/DSPUtils.h: 1
  • src/common/dsp/effects/NimbusEffect.cpp: 9
  • src/common/dsp/effects/airwindows/AirWindowsEffect.h: 2
  • src/common/dsp/effects/CombulatorEffect.cpp: 6
  • src/surge-fx/SurgeFXProcessor.cpp: 14
  • src/surge-xt/gui/overlays/LuaEditors.cpp: 1
  • src/surge-xt/gui/overlays/MSEGEditor.cpp: 5
  • src/surge-xt/gui/SurgeGUIEditorValueCallbacks.cpp: 5
  • src/surge-xt/gui/SurgeGUIEditor.cpp: 4
  • src/surge-xt/gui/widgets/LFOAndStepDisplay.cpp: 6
  • src/surge-xt/gui/widgets/XMLConfiguredMenus.cpp: 5

sprintf

  • src/common/dsp/filters/BiquadFilter.cpp: 1
  • src/surge-xt/gui/overlays/Oscilloscope.cpp: 7
  • src/surge-xt/gui/UndoManager.cpp: 1
  • src/surge-xt/gui/SurgeGUIEditorInfowindow.cpp: 1
  • src/surge-xt/gui/SurgeGUIEditorValueCallbacks.cpp: 2
  • src/surge-xt/gui/SurgeGUIEditor.cpp: 15
  • src/surge-xt/gui/widgets/OscillatorWaveformDisplay.cpp: 1

strncpy

  • src/surge-testrunner/UnitTestsIO.cpp: 1
  • src/common/SurgeStorage.cpp: 4
  • src/common/Parameter.cpp: 5
  • src/common/Parameter.h: 1
  • src/common/SurgeSynthesizerIO.cpp: 1
  • src/common/StringOps.h: 1
  • src/common/dsp/oscillators/ModernOscillator.cpp: 1
  • src/common/dsp/effects/airwindows/AirWindowsEffect.cpp: 1
  • src/surge-xt/gui/UndoManager.cpp: 1
  • src/surge-xt/gui/SurgeGUIEditorValueCallbacks.cpp: 1
  • src/surge-xt/gui/SurgeGUIEditor.h: 1
  • src/surge-xt/gui/widgets/OscillatorWaveformDisplay.cpp: 9

My comment from discord, edited.

The constraint is: Parameter needs to be copyable with memcpy so it can't have complex types like std::string as data members

But other than that everything can be std::string and it can easily have complex types as interface points

So, like

  void get_display(char *txt, bool external = false, float ef = 0.f) const;

could easily change to

std::string get_display(bool external = false, float ef = 0.f);

and then mark old get_display as deprecated, make it call get_display string version and do a strncpy

But these members in parameter

 char name[NAMECHARS]{}, dispname[NAMECHARS]{}, name_storage[NAMECHARS]{}, fullname[NAMECHARS]{};

And as to that override I meant

void get_display(char *t, bool ext = false, float ef = 0)
{
   auto gd = get_display(ext, ef);
   strncpy(t, TEXT_SIZE-1, gd.c_str());
}

until you remove all get_displays etc....

@baconpaul baconpaul added the Feature Request New feature request label Dec 4, 2022
@baconpaul baconpaul added this to the Surge XT 1.1.x milestone Dec 4, 2022
@mkruselj mkruselj added Code Refactoring General code refactoring and cleanup issues like names, unused variables, warnings, fixme and removed Feature Request New feature request labels Dec 4, 2022
@mx
Copy link
Collaborator

mx commented Mar 11, 2023

The constraint is: Parameter needs to be copyable with memcpy so it can't have complex types like std::string as data members

I know I fixed some of these cases to use C++ copy constructor/operator instead; I might have fixed all of them, can't remember. Let's double check if this is still a requirement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Refactoring General code refactoring and cleanup issues like names, unused variables, warnings, fixme
Projects
None yet
Development

No branches or pull requests

3 participants