Defines done outside of easylogging++.h does not work according to
official documentation
(https://github.com/zuhd-org/easyloggingpp/blob/master/README.md#configuration-macros)
"NOTE: All the macros can be defined in one of the following ways:
1. Define macros using -D option of compiler, for example in case of
g++ you will do g++ source.cpp ... -DELPP_SYSLOG -DELPP_THREAD_SAFE
... (recommended way)
2. Define macros inside "easylogging++.h" (defining macros in other
files won't work)"
Linking to this issue:
https://github.com/zuhd-org/easyloggingpp/issues/590#issuecomment-346753951
We are using the developer recommended way of defining them by command
line.
What is assumed to be the intention of enabling debug logs before
including easylogging++ never actually did anything, hence this define
is dropped. Newer versions of easylogging++ uses the macro
ELPP_DISABLE_DEBUG_LOGS to _disable_ debug logs. Leaving that macro
undefined satisfies the original intent.
Notice that the class has an identical var that is used in several
places. It is unknown if the original programmer intended to use the
member var in this function, but what is actually being done is a new
var being created, changed and returned. This may not respect the
original intention, but it does preserve the original mechanics.
Parameters with the same name as class members are ripe for introducing
bugs. The fact that the "this->" syntax is optional creates confusion
for maintaining programmers who can only guess at the intention.
Casting to void should shut up GCC about
error: statement has no effect [-Werror=unused-value]
static_cast<void>() is possible for all current types. Supposedly an old
C style cast could also be used, but they should not be used in modern
C++ code.
GCC gave error
../src/tabcontrollers/MoveCenterTabController.cpp:57:67: error:
operation on
‘((advsettings::MoveCenterTabController*)this)->advsettings::MoveCenterTabController::lastMoveButtonClick[0]’
may be undefined [-Werror=sequence-point]
Because assigning a value to itself twice is undefined behavior. It is
highly likely that intended behavior was to initialize both array
members to the same value at once.
Mostly casts and unsigned to signed comparisons.
Testing to ensure that the int isn't below 0. If it is, it can't be used
in the array later anyway. After we're sure it isn't below 0 it's safe
to static_cast<size_t>().
apperror was shadowing another variable in the scope outside of the if
statement, leading to a warning.
Adding an underscore changes the var name without changing the meaning.
In the future a different structure should probably be chosen for the
statements.
The warnings in dummy functions are preventing the project from being
compiled with warnings as errors.
There were two options:
* Using a define to create a noop (chosen option).
* Commenting out the variable name, leaving only the type.
The first option was chosen because the second option would leave out
information from the IDE and compiler, while the first option wouldn't.
The intent is also clearer with the first option, otherwise it would
likely be necessary to leave comments with every single parameter that
was commented out.
The install/remove manifest logic in main is currently duplicated. This
bloats up main and creates unnecessarily large indented logic. The only
thing that is different between the two large if blocks is a single
function call.
Refactoring to test for which function to call at the latest possible
stage reduces code duplication.
The command line argument parsing is done in a giant for loop in the
main function. This presents readability and extension problems.
Refactor the big for loop into smaller loops inside the function.
In theory this will affect performance slightly at startup, but when considering
the very low amount of possible command line arguments (5, currently) and the low
amount of arguments being passed on a typical launch (0 or 1, mostly),
the readability improvements are very much worth it.
The string literals have also been moved to the "argument" namespace as
constexpr, in order to avoid potential typos in string literals.
Should the amount of command line arguments increase severely, it might
be necessary to create a structure with only one pass through the
argument loop, but such a situation is very unlikely.
Vasily
Robin Appelman
ykeara
Moritz
Meredith