| Message ID | 1399569594-31678-1-git-send-email-mjt@msgid.tls.msk.ru |
|---|---|
| State | New |
| Headers | show |
On 05/08/2014 08:19 PM, Michael Tokarev wrote: > libcacard has many functions which initializes local variables > at declaration time, which are always assigned some values later > (often right after declaration). Clean up these initializers. How is this an improvement? Doesn't the compiler ignore this anyhow? > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > --- > libcacard/cac.c | 14 +++++++------- > libcacard/card_7816.c | 5 ++--- > libcacard/vcard.c | 4 ++-- > libcacard/vcard_emul_nss.c | 6 +++--- > libcacard/vreader.c | 10 +++++----- > libcacard/vscclient.c | 4 ++-- > 6 files changed, 21 insertions(+), 22 deletions(-) > > diff --git a/libcacard/cac.c b/libcacard/cac.c > index 122129e..d1d9ee2 100644 > --- a/libcacard/cac.c > +++ b/libcacard/cac.c > @@ -93,8 +93,8 @@ cac_common_process_apdu(VCard *card, VCardAPDU *apdu, VCardResponse **response) > static VCardStatus > cac_applet_pki_reset(VCard *card, int channel) > { > - VCardAppletPrivate *applet_private = NULL; > - CACPKIAppletData *pki_applet = NULL; > + VCardAppletPrivate *applet_private; > + CACPKIAppletData *pki_applet; > applet_private = vcard_get_current_applet_private(card, channel); > assert(applet_private); > pki_applet = &(applet_private->u.pki_data); > @@ -113,8 +113,8 @@ static VCardStatus > cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu, > VCardResponse **response) > { > - CACPKIAppletData *pki_applet = NULL; > - VCardAppletPrivate *applet_private = NULL; > + CACPKIAppletData *pki_applet; > + VCardAppletPrivate *applet_private; > int size, next; > unsigned char *sign_buffer; > vcard_7816_status_t status; > @@ -288,7 +288,7 @@ cac_applet_container_process_apdu(VCard *card, VCardAPDU *apdu, > static void > cac_delete_pki_applet_private(VCardAppletPrivate *applet_private) > { > - CACPKIAppletData *pki_applet_data = NULL; > + CACPKIAppletData *pki_applet_data; > > if (applet_private == NULL) { > return; > @@ -336,8 +336,8 @@ static VCardApplet * > cac_new_pki_applet(int i, const unsigned char *cert, > int cert_len, VCardKey *key) > { > - VCardAppletPrivate *applet_private = NULL; > - VCardApplet *applet = NULL; > + VCardAppletPrivate *applet_private; > + VCardApplet *applet; > unsigned char pki_aid[] = { 0xa0, 0x00, 0x00, 0x00, 0x79, 0x01, 0x00 }; > int pki_aid_len = sizeof(pki_aid); > > diff --git a/libcacard/card_7816.c b/libcacard/card_7816.c > index bca8c4a..a54f880 100644 > --- a/libcacard/card_7816.c > +++ b/libcacard/card_7816.c > @@ -416,7 +416,7 @@ VCARD_RESPONSE_NEW_STATIC_STATUS(VCARD7816_STATUS_ERROR_GENERAL) > VCardResponse * > vcard_make_response(vcard_7816_status_t status) > { > - VCardResponse *response = NULL; > + VCardResponse *response; > > switch (status) { > /* known 7816 response codes */ > @@ -543,9 +543,8 @@ vcard_make_response(vcard_7816_status_t status) > return VCARD_RESPONSE_GET_STATIC( > VCARD7816_STATUS_EXC_ERROR_MEMORY_FAILURE); > } > + return response; > } > - assert(response); > - return response; > } > > /* > diff --git a/libcacard/vcard.c b/libcacard/vcard.c > index 227e477..6aaf085 100644 > --- a/libcacard/vcard.c > +++ b/libcacard/vcard.c > @@ -166,8 +166,8 @@ vcard_reference(VCard *vcard) > void > vcard_free(VCard *vcard) > { > - VCardApplet *current_applet = NULL; > - VCardApplet *next_applet = NULL; > + VCardApplet *current_applet; > + VCardApplet *next_applet; > > if (vcard == NULL) { > return; > diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c > index 75b9d79..3f38a4c 100644 > --- a/libcacard/vcard_emul_nss.c > +++ b/libcacard/vcard_emul_nss.c > @@ -367,7 +367,7 @@ vcard_7816_status_t > vcard_emul_login(VCard *card, unsigned char *pin, int pin_len) > { > PK11SlotInfo *slot; > - unsigned char *pin_string = NULL; > + unsigned char *pin_string; > int i; > SECStatus rv; > > @@ -423,7 +423,7 @@ static VReader * > vcard_emul_find_vreader_from_slot(PK11SlotInfo *slot) > { > VReaderList *reader_list = vreader_get_reader_list(); > - VReaderListEntry *current_entry = NULL; > + VReaderListEntry *current_entry; > > if (reader_list == NULL) { > return NULL; > @@ -1050,7 +1050,7 @@ void > vcard_emul_replay_insertion_events(void) > { > VReaderListEntry *current_entry; > - VReaderListEntry *next_entry = NULL; > + VReaderListEntry *next_entry; > VReaderList *list = vreader_get_reader_list(); > > for (current_entry = vreader_list_get_first(list); current_entry; > diff --git a/libcacard/vreader.c b/libcacard/vreader.c > index 9304a28..22dfe43 100644 > --- a/libcacard/vreader.c > +++ b/libcacard/vreader.c > @@ -341,7 +341,7 @@ void > vreader_list_delete(VReaderList *list) > { > VReaderListEntry *current_entry; > - VReaderListEntry *next_entry = NULL; > + VReaderListEntry *next_entry; > for (current_entry = vreader_list_get_first(list); current_entry; > current_entry = next_entry) { > next_entry = vreader_list_get_next(current_entry); > @@ -432,8 +432,8 @@ vreader_list_unlock(void) > static VReaderList * > vreader_copy_list(VReaderList *list) > { > - VReaderList *new_list = NULL; > - VReaderListEntry *current_entry = NULL; > + VReaderList *new_list; > + VReaderListEntry *current_entry; > > new_list = vreader_list_new(); > if (new_list == NULL) { > @@ -465,7 +465,7 @@ VReader * > vreader_get_reader_by_id(vreader_id_t id) > { > VReader *reader = NULL; > - VReaderListEntry *current_entry = NULL; > + VReaderListEntry *current_entry; > > if (id == (vreader_id_t) -1) { > return NULL; > @@ -489,7 +489,7 @@ VReader * > vreader_get_reader_by_name(const char *name) > { > VReader *reader = NULL; > - VReaderListEntry *current_entry = NULL; > + VReaderListEntry *current_entry; > > vreader_list_lock(); > for (current_entry = vreader_list_get_first(vreader_list); current_entry; > diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c > index 3477ab3..17ff075 100644 > --- a/libcacard/vscclient.c > +++ b/libcacard/vscclient.c > @@ -131,8 +131,8 @@ static void * > event_thread(void *arg) > { > unsigned char atr[MAX_ATR_LEN]; > - int atr_len = MAX_ATR_LEN; > - VEvent *event = NULL; > + int atr_len; > + VEvent *event; > unsigned int reader_id; > > >
11.05.2014 11:58, Alon Levy wrote: > On 05/08/2014 08:19 PM, Michael Tokarev wrote: >> libcacard has many functions which initializes local variables >> at declaration time, which are always assigned some values later >> (often right after declaration). Clean up these initializers. > > How is this an improvement? Doesn't the compiler ignore this anyhow? Just less code. To me, when I see something like Type *var = NULL; in a function, it somehow "translates" to a construct like Type *found = NULL; That is -- so this variable will be used either as an accumulator or a search result, so that initial value is really important. So when I see the same variable receives its initial value in the next line, I start wondering what's missed in the code which should be there. Or why I don't read the code correctly. Or something like this. So, basically, this is a cleanup patch just to avoid confusion, it most likely not needed for current compiler who can figure it out by its own. And for consistency - why not initialize other variables too? Maybe that's just my old-scool mind works this way. At any rate you can just ignore this patch. Thanks, /mjt
Michael Tokarev <mjt@tls.msk.ru> writes: > 11.05.2014 11:58, Alon Levy wrote: >> On 05/08/2014 08:19 PM, Michael Tokarev wrote: >>> libcacard has many functions which initializes local variables >>> at declaration time, which are always assigned some values later >>> (often right after declaration). Clean up these initializers. >> >> How is this an improvement? Doesn't the compiler ignore this anyhow? > > Just less code. > > To me, when I see something like > > Type *var = NULL; > > in a function, it somehow "translates" to a construct like > > Type *found = NULL; > > That is -- so this variable will be used either as an accumulator > or a search result, so that initial value is really important. > > So when I see the same variable receives its initial value in > the next line, I start wondering what's missed in the code which > should be there. Or why I don't read the code correctly. Or > something like this. > > So, basically, this is a cleanup patch just to avoid confusion, > it most likely not needed for current compiler who can figure > it out by its own. And for consistency - why not initialize > other variables too? I hate redundant initializers for yet another reason: when I change the code, and accidentally add a path bypassing the *real* initialization, I don't get a "may be used uninitialized" warning, I get the stupid redundant initialization and quite possibly a crash to debug some time later. > Maybe that's just my old-scool mind works this way. > > At any rate you can just ignore this patch. Please consider it.
So, should we apply this or not? It's been waiting for quite some time, and during this time we've found a very good example of why it should be applied (I think anyway). Thanks, /mjt 12.05.2014 13:20, Markus Armbruster wrote: > Michael Tokarev <mjt@tls.msk.ru> writes: > >> 11.05.2014 11:58, Alon Levy wrote: >>> On 05/08/2014 08:19 PM, Michael Tokarev wrote: >>>> libcacard has many functions which initializes local variables >>>> at declaration time, which are always assigned some values later >>>> (often right after declaration). Clean up these initializers. >>> >>> How is this an improvement? Doesn't the compiler ignore this anyhow? >> >> Just less code. >> >> To me, when I see something like >> >> Type *var = NULL; >> >> in a function, it somehow "translates" to a construct like >> >> Type *found = NULL; >> >> That is -- so this variable will be used either as an accumulator >> or a search result, so that initial value is really important. >> >> So when I see the same variable receives its initial value in >> the next line, I start wondering what's missed in the code which >> should be there. Or why I don't read the code correctly. Or >> something like this. >> >> So, basically, this is a cleanup patch just to avoid confusion, >> it most likely not needed for current compiler who can figure >> it out by its own. And for consistency - why not initialize >> other variables too? > > I hate redundant initializers for yet another reason: when I change the > code, and accidentally add a path bypassing the *real* initialization, I > don't get a "may be used uninitialized" warning, I get the stupid > redundant initialization and quite possibly a crash to debug some time > later. > >> Maybe that's just my old-scool mind works this way. >> >> At any rate you can just ignore this patch. > > Please consider it. >
On 05/23/2014 11:59 PM, Michael Tokarev wrote: > So, should we apply this or not? It's been waiting for quite some time, > and during this time we've found a very good example of why it should > be applied (I think anyway). I'm fine with applying it, I changed my mind. > > Thanks, > > /mjt > > > 12.05.2014 13:20, Markus Armbruster wrote: >> Michael Tokarev <mjt@tls.msk.ru> writes: >> >>> 11.05.2014 11:58, Alon Levy wrote: >>>> On 05/08/2014 08:19 PM, Michael Tokarev wrote: >>>>> libcacard has many functions which initializes local variables >>>>> at declaration time, which are always assigned some values later >>>>> (often right after declaration). Clean up these initializers. >>>> >>>> How is this an improvement? Doesn't the compiler ignore this anyhow? >>> >>> Just less code. >>> >>> To me, when I see something like >>> >>> Type *var = NULL; >>> >>> in a function, it somehow "translates" to a construct like >>> >>> Type *found = NULL; >>> >>> That is -- so this variable will be used either as an accumulator >>> or a search result, so that initial value is really important. >>> >>> So when I see the same variable receives its initial value in >>> the next line, I start wondering what's missed in the code which >>> should be there. Or why I don't read the code correctly. Or >>> something like this. >>> >>> So, basically, this is a cleanup patch just to avoid confusion, >>> it most likely not needed for current compiler who can figure >>> it out by its own. And for consistency - why not initialize >>> other variables too? >> >> I hate redundant initializers for yet another reason: when I change the >> code, and accidentally add a path bypassing the *real* initialization, I >> don't get a "may be used uninitialized" warning, I get the stupid >> redundant initialization and quite possibly a crash to debug some time >> later. >> >>> Maybe that's just my old-scool mind works this way. >>> >>> At any rate you can just ignore this patch. >> >> Please consider it. >> > >
diff --git a/libcacard/cac.c b/libcacard/cac.c index 122129e..d1d9ee2 100644 --- a/libcacard/cac.c +++ b/libcacard/cac.c @@ -93,8 +93,8 @@ cac_common_process_apdu(VCard *card, VCardAPDU *apdu, VCardResponse **response) static VCardStatus cac_applet_pki_reset(VCard *card, int channel) { - VCardAppletPrivate *applet_private = NULL; - CACPKIAppletData *pki_applet = NULL; + VCardAppletPrivate *applet_private; + CACPKIAppletData *pki_applet; applet_private = vcard_get_current_applet_private(card, channel); assert(applet_private); pki_applet = &(applet_private->u.pki_data); @@ -113,8 +113,8 @@ static VCardStatus cac_applet_pki_process_apdu(VCard *card, VCardAPDU *apdu, VCardResponse **response) { - CACPKIAppletData *pki_applet = NULL; - VCardAppletPrivate *applet_private = NULL; + CACPKIAppletData *pki_applet; + VCardAppletPrivate *applet_private; int size, next; unsigned char *sign_buffer; vcard_7816_status_t status; @@ -288,7 +288,7 @@ cac_applet_container_process_apdu(VCard *card, VCardAPDU *apdu, static void cac_delete_pki_applet_private(VCardAppletPrivate *applet_private) { - CACPKIAppletData *pki_applet_data = NULL; + CACPKIAppletData *pki_applet_data; if (applet_private == NULL) { return; @@ -336,8 +336,8 @@ static VCardApplet * cac_new_pki_applet(int i, const unsigned char *cert, int cert_len, VCardKey *key) { - VCardAppletPrivate *applet_private = NULL; - VCardApplet *applet = NULL; + VCardAppletPrivate *applet_private; + VCardApplet *applet; unsigned char pki_aid[] = { 0xa0, 0x00, 0x00, 0x00, 0x79, 0x01, 0x00 }; int pki_aid_len = sizeof(pki_aid); diff --git a/libcacard/card_7816.c b/libcacard/card_7816.c index bca8c4a..a54f880 100644 --- a/libcacard/card_7816.c +++ b/libcacard/card_7816.c @@ -416,7 +416,7 @@ VCARD_RESPONSE_NEW_STATIC_STATUS(VCARD7816_STATUS_ERROR_GENERAL) VCardResponse * vcard_make_response(vcard_7816_status_t status) { - VCardResponse *response = NULL; + VCardResponse *response; switch (status) { /* known 7816 response codes */ @@ -543,9 +543,8 @@ vcard_make_response(vcard_7816_status_t status) return VCARD_RESPONSE_GET_STATIC( VCARD7816_STATUS_EXC_ERROR_MEMORY_FAILURE); } + return response; } - assert(response); - return response; } /* diff --git a/libcacard/vcard.c b/libcacard/vcard.c index 227e477..6aaf085 100644 --- a/libcacard/vcard.c +++ b/libcacard/vcard.c @@ -166,8 +166,8 @@ vcard_reference(VCard *vcard) void vcard_free(VCard *vcard) { - VCardApplet *current_applet = NULL; - VCardApplet *next_applet = NULL; + VCardApplet *current_applet; + VCardApplet *next_applet; if (vcard == NULL) { return; diff --git a/libcacard/vcard_emul_nss.c b/libcacard/vcard_emul_nss.c index 75b9d79..3f38a4c 100644 --- a/libcacard/vcard_emul_nss.c +++ b/libcacard/vcard_emul_nss.c @@ -367,7 +367,7 @@ vcard_7816_status_t vcard_emul_login(VCard *card, unsigned char *pin, int pin_len) { PK11SlotInfo *slot; - unsigned char *pin_string = NULL; + unsigned char *pin_string; int i; SECStatus rv; @@ -423,7 +423,7 @@ static VReader * vcard_emul_find_vreader_from_slot(PK11SlotInfo *slot) { VReaderList *reader_list = vreader_get_reader_list(); - VReaderListEntry *current_entry = NULL; + VReaderListEntry *current_entry; if (reader_list == NULL) { return NULL; @@ -1050,7 +1050,7 @@ void vcard_emul_replay_insertion_events(void) { VReaderListEntry *current_entry; - VReaderListEntry *next_entry = NULL; + VReaderListEntry *next_entry; VReaderList *list = vreader_get_reader_list(); for (current_entry = vreader_list_get_first(list); current_entry; diff --git a/libcacard/vreader.c b/libcacard/vreader.c index 9304a28..22dfe43 100644 --- a/libcacard/vreader.c +++ b/libcacard/vreader.c @@ -341,7 +341,7 @@ void vreader_list_delete(VReaderList *list) { VReaderListEntry *current_entry; - VReaderListEntry *next_entry = NULL; + VReaderListEntry *next_entry; for (current_entry = vreader_list_get_first(list); current_entry; current_entry = next_entry) { next_entry = vreader_list_get_next(current_entry); @@ -432,8 +432,8 @@ vreader_list_unlock(void) static VReaderList * vreader_copy_list(VReaderList *list) { - VReaderList *new_list = NULL; - VReaderListEntry *current_entry = NULL; + VReaderList *new_list; + VReaderListEntry *current_entry; new_list = vreader_list_new(); if (new_list == NULL) { @@ -465,7 +465,7 @@ VReader * vreader_get_reader_by_id(vreader_id_t id) { VReader *reader = NULL; - VReaderListEntry *current_entry = NULL; + VReaderListEntry *current_entry; if (id == (vreader_id_t) -1) { return NULL; @@ -489,7 +489,7 @@ VReader * vreader_get_reader_by_name(const char *name) { VReader *reader = NULL; - VReaderListEntry *current_entry = NULL; + VReaderListEntry *current_entry; vreader_list_lock(); for (current_entry = vreader_list_get_first(vreader_list); current_entry; diff --git a/libcacard/vscclient.c b/libcacard/vscclient.c index 3477ab3..17ff075 100644 --- a/libcacard/vscclient.c +++ b/libcacard/vscclient.c @@ -131,8 +131,8 @@ static void * event_thread(void *arg) { unsigned char atr[MAX_ATR_LEN]; - int atr_len = MAX_ATR_LEN; - VEvent *event = NULL; + int atr_len; + VEvent *event; unsigned int reader_id;
libcacard has many functions which initializes local variables at declaration time, which are always assigned some values later (often right after declaration). Clean up these initializers. Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- libcacard/cac.c | 14 +++++++------- libcacard/card_7816.c | 5 ++--- libcacard/vcard.c | 4 ++-- libcacard/vcard_emul_nss.c | 6 +++--- libcacard/vreader.c | 10 +++++----- libcacard/vscclient.c | 4 ++-- 6 files changed, 21 insertions(+), 22 deletions(-)