surge icon indicating copy to clipboard operation
surge copied to clipboard

Review and ideally remediate old style C string handling

Open baconpaul opened this issue 2 years ago • 1 comments

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
  • [x] src/common/UnitConversions.h: 1
  • [x] 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
  • [x] src/common/dsp/utilities/DSPUtils.h: 1
  • [x] src/common/dsp/effects/NimbusEffect.cpp: 9
  • [ ] src/common/dsp/effects/airwindows/AirWindowsEffect.h: 2
  • [x] src/common/dsp/effects/CombulatorEffect.cpp: 6
  • [x] src/surge-fx/SurgeFXProcessor.cpp: 14
  • [x] src/surge-xt/gui/overlays/LuaEditors.cpp: 1
  • [x] src/surge-xt/gui/overlays/MSEGEditor.cpp: 5
  • [x] src/surge-xt/gui/SurgeGUIEditorValueCallbacks.cpp: 5
  • [x] src/surge-xt/gui/SurgeGUIEditor.cpp: 4
  • [x] src/surge-xt/gui/widgets/LFOAndStepDisplay.cpp: 6
  • [x] src/surge-xt/gui/widgets/XMLConfiguredMenus.cpp: 5

sprintf

  • [x] src/common/dsp/filters/BiquadFilter.cpp: 1
  • [x] src/surge-xt/gui/overlays/Oscilloscope.cpp: 7
  • [x] src/surge-xt/gui/UndoManager.cpp: 1
  • [x] src/surge-xt/gui/SurgeGUIEditorInfowindow.cpp: 1
  • [x] src/surge-xt/gui/SurgeGUIEditorValueCallbacks.cpp: 2
  • [x] src/surge-xt/gui/SurgeGUIEditor.cpp: 15
  • [x] 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
  • [x] 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
  • [x] src/surge-xt/gui/UndoManager.cpp: 1
  • [ ] src/surge-xt/gui/SurgeGUIEditorValueCallbacks.cpp: 1
  • [ ] src/surge-xt/gui/SurgeGUIEditor.h: 1
  • [x] 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 avatar Dec 04 '22 23:12 baconpaul

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.

mx avatar Mar 11 '23 23:03 mx