[pacman-dev] [PATCH] Some options were refactored
./src/pacman/pacman.h:
- The defines were placed here that are used in the
pacman.c source file
./src/pacman/pacman.c
- Long options were refactored because of a safer and more
comfortable programming style, instead of hard coding
10-15 or more integer value into the code
Signed-off-by: Laszlo Papp
Laszlo Papp wrote:
./src/pacman/pacman.h: - The defines were placed here that are used in the pacman.c source file ./src/pacman/pacman.c - Long options were refactored because of a safer and more comfortable programming style, instead of hard coding 10-15 or more integer value into the code
Please try and make better commit messages. The title tells me nothing about the patch and the description is not much better. Allan
On Thu, Sep 24, 2009 at 11:17 PM, Allan McRae
Laszlo Papp wrote:
./src/pacman/pacman.h: - The defines were placed here that are used in the pacman.c source file ./src/pacman/pacman.c - Long options were refactored because of a safer and more comfortable programming style, instead of hard coding 10-15 or more integer value into the code
Please try and make better commit messages. The title tells me nothing about the patch and the description is not much better.
By way of example, I would have done something like: ----- Replace hardcoded option numbers with #defines pacman's long option parsing used hardcoded numbers to identify them. This is not good practice, so replace them with #defined constants. Signed-off-by: Skeletor -----
Laszlo Papp wrote:
./src/pacman/pacman.h: - The defines were placed here that are used in the pacman.c source file ./src/pacman/pacman.c - Long options were refactored because of a safer and more comfortable programming style, instead of hard coding 10-15 or more integer value into the code
Signed-off-by: Laszlo Papp
--- src/pacman/pacman.c | 56 +++++++++++++++++++++++++------------------------- src/pacman/pacman.h | 16 ++++++++++++++ 2 files changed, 44 insertions(+), 28 deletions(-) <snip>
+#define NOCONFIRM_OPTID 1000 +#define CONFIG_OPTID 1001 +#define IGNORE_OPTID 1002 +#define DEBUG_OPTID 1003 +#define NOPROGRESSBAR_OPTID 1004 +#define NOSCRIPTLET_OPTID 1005 +#define ASK_OPTID 1006 +#define CACHEDIR_OPTID 1007 +#define ASDEPS_OPTID 1008 +#define LOGFILE_OPTID 1009 +#define IGNOREGROUP_OPTID 1010 +#define NEEDED_OPTID 1011 +#define ASEXPLICIT_OPTID 1012 +#define ARCH_OPTID 1013
This patch seems fine to me. You probably want to use spaces rather than tabs to align those values. Variable tabs sizes do not matter much for start of line code indentation but they do matter in the middle of lines. I'm not sure about the variable names. Would it be better if they all started the same? i.e. OPT_FOO. That just seems nicer stylistically to me. Allan
On Fri, Sep 25, 2009 at 8:09 AM, Allan McRae
Laszlo Papp wrote:
./src/pacman/pacman.h: - The defines were placed here that are used in the pacman.c source file ./src/pacman/pacman.c - Long options were refactored because of a safer and more comfortable programming style, instead of hard coding 10-15 or more integer value into the code
Signed-off-by: Laszlo Papp
--- src/pacman/pacman.c | 56 +++++++++++++++++++++++++------------------------- src/pacman/pacman.h | 16 ++++++++++++++ 2 files changed, 44 insertions(+), 28 deletions(-) <snip> +#define NOCONFIRM_OPTID 1000 +#define CONFIG_OPTID 1001 +#define IGNORE_OPTID 1002 +#define DEBUG_OPTID 1003 +#define NOPROGRESSBAR_OPTID 1004 +#define NOSCRIPTLET_OPTID 1005 +#define ASK_OPTID 1006 +#define CACHEDIR_OPTID 1007 +#define ASDEPS_OPTID 1008 +#define LOGFILE_OPTID 1009 +#define IGNOREGROUP_OPTID 1010 +#define NEEDED_OPTID 1011 +#define ASEXPLICIT_OPTID 1012 +#define ARCH_OPTID 1013
This patch seems fine to me. You probably want to use spaces rather than tabs to align those values. Variable tabs sizes do not matter much for start of line code indentation but they do matter in the middle of lines.
I'm not sure about the variable names. Would it be better if they all started the same? i.e. OPT_FOO. That just seems nicer stylistically to me.
Allan
And what about using an enum here ?
participants (4)
-
Aaron Griffin
-
Allan McRae
-
Laszlo Papp
-
Xavier