C++ Code cleanup #566

TheMagnetar · 2018-08-11T13:00:50Z

When merged this pull request will:

Clean c++ code
Const correctness
Use of base types (int16_t, int32_t, ...)
Get rid of microsoft specific types (DWORD, BYTE,...)
Get rid of microsoft "safe" printf variants
Get rid of macro codes (DECLARE_MEMBER, ...)
Consistent naming of arguments and member types

NouberNou

In general not a big fan of the naming of function arguments or member variables. Hungarian type notation should really be avoided as it is totally redundant information.

NouberNou · 2018-08-26T03:10:55Z

extensions/src/ACRE2Core/AmplitudeAttenuation.cpp

-    for (int i = 0; i < sampleCount*channels && i < 4095; i++, iter++) {
-        if (samples[i] > 0) {
-            currentSample = 20.0f * log10((float)samples[i]);
+ACRE_RESULT CAmplitudeAttenuation::process(int16_t *const a_samples, const int32_t ac_sampleCount, const int32_t ac_channels, CPlayer *const a_player) {


Can we please not do pseudo hungarian notation? If you want to signify a variable is an argument I've always prefered a trailing _, so player_. Noting the constiness of an argument is also not super helpful in most modern editors/IDEs. It should show a redline.

TheMagnetar · 2018-08-26T15:20:41Z

Can do

dedmen · 2019-02-12T07:33:04Z

extensions/src/ACRE2Arma/arma2ts/CallExt_DllMain.cpp

@@ -16,13 +16,13 @@

 // trim from start
 static inline std::string &ltrim(std::string &s) {
-    s.erase(s.begin(), std::find_if(s.begin(), s.end(), std::not1(std::ptr_fun<int, int>(std::isspace))));
+    s.erase(s.begin(), std::find_if(s.begin(), s.end(), std::not1(std::ptr_fun<int32_t, int32_t>(std::isspace))));


This should really be converted to using a lambda. std::ptr_fun has been deprecated in C++11. and std::not1 has been deprecated in c++17.

Here is example code for ltrim/rtrim https://github.com/dedmen/armake/blob/cpp/src/utils.cpp#L251
Also supporting tabs.

dedmen · 2019-02-12T07:35:15Z

extensions/src/ACRE2Arma/arma2ts/CallExt_DllMain.cpp

@@ -210,7 +212,7 @@ void __stdcall RVExtension(char *output, int outputSize, const char *function)
                char *value = new char[4096];
                //DEBUG("Read from pipe [%d]\n", hPipe);
                ret = ReadFile(readHandle, (LPVOID)value, 4096, &cbRead, NULL);
-                if ( ! ret) {
+                if ( !ret) {


Suggested change

if ( !ret) {

if (!ret) {

?

dedmen · 2019-02-12T07:39:19Z

extensions/src/ACRE2Core/BabbelEffect.h

+        m_sfreq = 48000.0f;
+        m_mfreq = 5200.0f;
+        lpFilter1.setup(4, (float64_t) m_sfreq, (float64_t) m_mfreq*0.3);
+        lpFilter2.setup(4, (float64_t) m_sfreq, (float64_t) m_mfreq*0.5);


Should use proper c++ static_cast

dedmen · 2019-02-12T07:39:50Z

extensions/src/ACRE2Core/BabbelEffect.h

-        short maxAmp = 1;
-        for (int i = 0; i < sampleCount; ++i) {
-            samples[i] = (short)(((float)tempSamples[i]*1.0f)*cos(((float)i/(sfreq/mfreq))*1.4*M_PI));
+        int16_t *tempSamples = new int16_t[sampleCount];


use std::vector with resize for a temp array. That way you don't have to worry about cleanup.

dedmen · 2019-02-12T07:40:52Z

extensions/src/ACRE2Core/Engine.cpp


-    g_Log = (Log *)new Log(const_cast<char *>(loggingPath.c_str()));
+    g_Log = (Log *) new Log(const_cast<char *>(loggingPath.c_str()));


Why is the reason of casting Log* to Log* here?

And why is it casting a constant string to a non constant char*, That might be undefined behavior.

dedmen · 2019-02-12T07:44:03Z

extensions/src/ACRE2Core/FFT.cpp

-        for (int i = 0; i < n; ++i)
-            result[i] /= n;
-    return result;
+    if (m_inverse == false)


== false?

Suggested change

if (m_inverse == false)

if (!m_inverse)

dedmen · 2019-02-12T07:45:03Z

extensions/src/ACRE2Core/FilterOcclusion.cpp

-#define TS_SAMPLE_RATE 48000
-#define CUTTOFF_FREQ    15000
+#define TS_SAMPLE_RATE_Hz 48000
+#define CUTTOFF_FREQ_Hz    15000


Suggested change

#define CUTTOFF_FREQ_Hz 15000

#define CUTTOFF_FREQ_Hz 15000

Macros are usually identified by being all upper case. The z is not.

This is accepted when trying to define units. mHz is not the same as MHz and so on.

dedmen · 2019-02-12T07:49:07Z

extensions/src/ACRE2Core/FilterOcclusion.h


-    DECLARE_MEMBER(int, ChannelCount);
+    virtual __inline void setChannelCount(const int32_t value) { this->m_channelCount = value; }


__inline is MSVC specific, inline is the real keyword.
Also inline doesn't make much sense on virtual functions. If the compiler doesn't know exactly that the class will be that exact type, then it won't be able to inline. And if it knew that it was that type, then it would also inline if you don't put the inline keyword there.

dedmen · 2019-02-12T07:52:30Z

extensions/src/ACRE2Core/NamedPipeServer.h

+    bool getConnected() const { return (this->getConnectedRead() && this->getConnectedWrite()); };
+    void setConnected (const bool value) { this->setConnectedRead(value); this->setConnectedWrite(value); };
+
+    virtual __inline void setPipeHandleRead(const HANDLE value) { this->m_pipeHandleRead = value; }


These functions should be override, not virtual

Suggested change

virtual __inline void setPipeHandleRead(const HANDLE value) { this->m_pipeHandleRead = value; }

void setPipeHandleRead(const HANDLE value) override { this->m_pipeHandleRead = value; }

dedmen · 2019-02-12T07:56:08Z

extensions/src/ACRE2Core/SoundMonoChannel.cpp

-        delete this->buffer;
-        this->buffer = NULL;
+    if (this->m_buffer) {
+        delete this->m_buffer;


just make it a vector and it will cleanup itself.

dedmen · 2019-02-12T07:58:38Z

extensions/src/ACRE2Core/ext_remoteStopSpeaking.h

@@ -21,11 +21,11 @@ RPC_FUNCTION(ext_remoteStopSpeaking) {
            this->getSelf()->getCurveScale()
        ) */

-    ACRE_ID id = (ACRE_ID)vMessage->getParameterAsInt(0);
+    ACRE_ID id = (ACRE_ID) vMessage->getParameterAsInt(0);


static_cast and auto?
Is the cast even necessary? Can't it implictly convert on =?

dedmen · 2019-02-12T08:00:29Z

extensions/src/ACRE2Shared/Log.cpp

@@ -5,7 +5,7 @@

 Log *g_Log = NULL;

-Log::Log(char *logFile) { 
+Log::Log(char *logFile) {


const char* ?

dedmen · 2019-02-12T08:02:10Z

extensions/src/ACRE2TS/TS3Client.h

+    ACRE_RESULT updateShouldSwitchTS3Channel(const bool state);
+    bool shouldSwitchTS3Channel();
+
+    virtual __inline void sethadVAD(const bool value) { this->m_hadVAD = value; }


setHadVad* ?
Also override.

dedmen · 2019-02-12T08:03:39Z

extensions/src/Wav2B64/Wave.h

@@ -6,53 +6,48 @@
 #include <string>


-typedef struct __WAVEDESCR
+typedef struct __waveDescr


This is a library struct
https://github.com/xinshangbo/WendySpeech/blob/master/src/CutVoiceCPP/Wave.h#L6

jonpas · 2023-09-14T00:35:20Z

No further activity and tons of conflicts, would need to get re-done.

Shared + Core cleanup

7500774

TheMagnetar added cleanup WIP labels Aug 11, 2018

TheMagnetar added this to the 2.7.0 milestone Aug 11, 2018

TheMagnetar added 2 commits August 11, 2018 17:13

Wav2B64

e7298a7

ACRE2TS

e47d7d2

NouberNou reviewed Aug 26, 2018

View reviewed changes

Remove hungarian notation

dc5347e

dedmen reviewed Feb 12, 2019

View reviewed changes

TheMagnetar modified the milestones: 2.7.0, Ongoing Apr 3, 2019

jonpas modified the milestones: Ongoing, Backlog Feb 24, 2021

jonpas closed this Sep 14, 2023

jonpas deleted the CppCleanup branch September 14, 2023 00:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C++ Code cleanup #566

C++ Code cleanup #566

TheMagnetar commented Aug 11, 2018

NouberNou left a comment

NouberNou Aug 26, 2018

TheMagnetar commented Aug 26, 2018

dedmen Feb 12, 2019

dedmen Feb 12, 2019

dedmen Feb 12, 2019

dedmen Feb 12, 2019

dedmen Feb 12, 2019

dedmen Feb 12, 2019

dedmen Feb 12, 2019 •

edited

dedmen Feb 12, 2019

dedmen Feb 12, 2019

dedmen Feb 12, 2019

TheMagnetar Feb 12, 2019

dedmen Feb 12, 2019

dedmen Feb 12, 2019

dedmen Feb 12, 2019

dedmen Feb 12, 2019

dedmen Feb 12, 2019

dedmen Feb 12, 2019

dedmen Feb 12, 2019

jonpas commented Sep 14, 2023


		g_Log = (Log )new Log(const_cast<char >(loggingPath.c_str()));
		g_Log = (Log ) new Log(const_cast<char >(loggingPath.c_str()));


		DECLARE_MEMBER(int, ChannelCount);
		virtual __inline void setChannelCount(const int32_t value) { this->m_channelCount = value; }

	virtual __inline void setPipeHandleRead(const HANDLE value) { this->m_pipeHandleRead = value; }
	void setPipeHandleRead(const HANDLE value) override { this->m_pipeHandleRead = value; }

C++ Code cleanup #566

C++ Code cleanup #566

Conversation

TheMagnetar commented Aug 11, 2018

NouberNou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheMagnetar commented Aug 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dedmen Feb 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonpas commented Sep 14, 2023

dedmen Feb 12, 2019 •

edited