ehlo,
attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc The comments does not guarantee anything.
Author: Michal Zidek mzidek@redhat.com Date: Tue Mar 10 17:30:48 2015 +0100
DEBUG: Add missing strings for error messages
We had more error codes than corresponding messages. Also order of two messages was wrong.
Unfortunatelly, the index of enum do not start from 0 and need to be converted using SSSD_ERR_IDX. I used shorter version of macro "idx". Feel free to propose nicer name.
LS
On 08/12/2015 01:11 PM, Lukas Slebodnik wrote:
ehlo,
attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc The comments does not guarantee anything.
Author: Michal Zidek mzidek@redhat.com Date: Tue Mar 10 17:30:48 2015 +0100
DEBUG: Add missing strings for error messages We had more error codes than corresponding messages. Also order of two messages was wrong.Unfortunatelly, the index of enum do not start from 0 and need to be converted using SSSD_ERR_IDX. I used shorter version of macro "idx". Feel free to propose nicer name.
I would use the SSSD_ERR_IDX and not create an alias for it. I think it is always better to have properly namespaced macros even if the are a little longer.
LS
On (19/08/15 15:57), Michal Židek wrote:
On 08/12/2015 01:11 PM, Lukas Slebodnik wrote:
ehlo,
attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc The comments does not guarantee anything.
Author: Michal Zidek mzidek@redhat.com Date: Tue Mar 10 17:30:48 2015 +0100
DEBUG: Add missing strings for error messages We had more error codes than corresponding messages. Also order of two messages was wrong.Unfortunatelly, the index of enum do not start from 0 and need to be converted using SSSD_ERR_IDX. I used shorter version of macro "idx". Feel free to propose nicer name.
I would use the SSSD_ERR_IDX and not create an alias for it. I think it is always better to have properly namespaced macros even if the are a little longer.
a) the macro is not in public header. b) SSSD_ERR_IDX significantly decrease readability
LS
On 08/27/2015 12:52 PM, Lukas Slebodnik wrote:
On (19/08/15 15:57), Michal Židek wrote:
On 08/12/2015 01:11 PM, Lukas Slebodnik wrote:
ehlo,
attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc The comments does not guarantee anything.
Author: Michal Zidek mzidek@redhat.com Date: Tue Mar 10 17:30:48 2015 +0100
DEBUG: Add missing strings for error messages We had more error codes than corresponding messages. Also order of two messages was wrong.Unfortunatelly, the index of enum do not start from 0 and need to be converted using SSSD_ERR_IDX. I used shorter version of macro "idx". Feel free to propose nicer name.
I would use the SSSD_ERR_IDX and not create an alias for it. I think it is always better to have properly namespaced macros even if the are a little longer.
a) the macro is not in public header.
It is int util_errors.h
git grep '#include .*util_errors.h' src/providers/dp_refresh.c:#include "util/util_errors.h" src/providers/ipa/ipa_dyndns.h:#include "util/util_errors.h" src/providers/krb5/krb5_child.c:#include "src/util/util_errors.h" src/tests/sbus_codegen_tests.c:#include "util/util_errors.h" src/tests/sbus_tests.c:#include "util/util_errors.h" src/util/util.h:#include "util/util_errors.h" ^^^^^^^^ So it is almost everywhere with the exception of the client code.
b) SSSD_ERR_IDX significantly decrease readability
Not properly namespacing macros can bite us in the long term. I do not think that SSSD_ERR_IDX decreases readability, but even if it was the case I would say it is simply necessary. Both SSSD_ and ERR_ prefixes are important here.
Michal
LS
On (08/09/15 14:57), Michal Židek wrote:
On 08/27/2015 12:52 PM, Lukas Slebodnik wrote:
On (19/08/15 15:57), Michal Židek wrote:
On 08/12/2015 01:11 PM, Lukas Slebodnik wrote:
ehlo,
attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc The comments does not guarantee anything.
Author: Michal Zidek mzidek@redhat.com Date: Tue Mar 10 17:30:48 2015 +0100
DEBUG: Add missing strings for error messages We had more error codes than corresponding messages. Also order of two messages was wrong.Unfortunatelly, the index of enum do not start from 0 and need to be converted using SSSD_ERR_IDX. I used shorter version of macro "idx". Feel free to propose nicer name.
I would use the SSSD_ERR_IDX and not create an alias for it. I think it is always better to have properly namespaced macros even if the are a little longer.
a) the macro is not in public header.
It is int util_errors.h
git grep '#include .*util_errors.h' src/providers/dp_refresh.c:#include "util/util_errors.h" src/providers/ipa/ipa_dyndns.h:#include "util/util_errors.h" src/providers/krb5/krb5_child.c:#include "src/util/util_errors.h" src/tests/sbus_codegen_tests.c:#include "util/util_errors.h" src/tests/sbus_tests.c:#include "util/util_errors.h" src/util/util.h:#include "util/util_errors.h" ^^^^^^^^ So it is almost everywhere with the exception of the client code.
I meant that the local version of SSSD_ERR_IDX is not im public header. So it could not colide with other files.
b) SSSD_ERR_IDX significantly decrease readability
Not properly namespacing macros can bite us in the long term. I do not think that SSSD_ERR_IDX decreases readability, but even if it was the case I would say it is simply necessary. Both SSSD_ and ERR_ prefixes are important here.
OK, try to compare few followinf versions and choose the one which can be easily proceed/understood.
a) struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = { [SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" }, [SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" }, [SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" }, [SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
b) struct err_string error_to_str[ idx(ERR_LAST) + 1 ] = { [ idx(ERR_INVALID) ] = { "Invalid Error" }, [ idx(ERR_INTERNAL) ] = { "Internal Error" }, [ idx(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ idx(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ idx(ERR_NO_CREDS) ] = { "No credentials available" }, [ idx(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
c) struct err_string error_to_str[ _(ERR_LAST) + 1 ] = { [ _(ERR_INVALID) ] = { "Invalid Error" }, [ _(ERR_INTERNAL) ] = { "Internal Error" }, [ _(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ _(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ _(ERR_NO_CREDS) ] = { "No credentials available" }, [ _(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
d) struct err_string error_to_str[] = { { "Invalid Error" }, /* ERR_INVALID */ { "Internal Error" }, /* ERR_INTERNAL */ { "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */ { "Invalid credential type" }, /* ERR_INVALID_CRED_TYPE */ { "No credentials available" }, /* ERR_NO_CREDS */ { "Credentials are expired" }, /* ERR_CREDS_EXPIRED */
IMHO, the last one is the best, but there is not a compile time check.
LS
On 09/08/2015 03:09 PM, Lukas Slebodnik wrote:
On (08/09/15 14:57), Michal Židek wrote:
On 08/27/2015 12:52 PM, Lukas Slebodnik wrote:
On (19/08/15 15:57), Michal Židek wrote:
On 08/12/2015 01:11 PM, Lukas Slebodnik wrote:
ehlo,
attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc The comments does not guarantee anything.
Author: Michal Zidek mzidek@redhat.com Date: Tue Mar 10 17:30:48 2015 +0100
DEBUG: Add missing strings for error messages We had more error codes than corresponding messages. Also order of two messages was wrong.Unfortunatelly, the index of enum do not start from 0 and need to be converted using SSSD_ERR_IDX. I used shorter version of macro "idx". Feel free to propose nicer name.
I would use the SSSD_ERR_IDX and not create an alias for it. I think it is always better to have properly namespaced macros even if the are a little longer.
a) the macro is not in public header.
It is int util_errors.h
git grep '#include .*util_errors.h' src/providers/dp_refresh.c:#include "util/util_errors.h" src/providers/ipa/ipa_dyndns.h:#include "util/util_errors.h" src/providers/krb5/krb5_child.c:#include "src/util/util_errors.h" src/tests/sbus_codegen_tests.c:#include "util/util_errors.h" src/tests/sbus_tests.c:#include "util/util_errors.h" src/util/util.h:#include "util/util_errors.h" ^^^^^^^^ So it is almost everywhere with the exception of the client code.
I meant that the local version of SSSD_ERR_IDX is not im public header. So it could not colide with other files.
b) SSSD_ERR_IDX significantly decrease readability
Not properly namespacing macros can bite us in the long term. I do not think that SSSD_ERR_IDX decreases readability, but even if it was the case I would say it is simply necessary. Both SSSD_ and ERR_ prefixes are important here.
OK, try to compare few followinf versions and choose the one which can be easily proceed/understood.
a) struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = { [SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" }, [SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" }, [SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" }, [SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
b) struct err_string error_to_str[ idx(ERR_LAST) + 1 ] = { [ idx(ERR_INVALID) ] = { "Invalid Error" }, [ idx(ERR_INTERNAL) ] = { "Internal Error" }, [ idx(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ idx(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ idx(ERR_NO_CREDS) ] = { "No credentials available" }, [ idx(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
c) struct err_string error_to_str[ _(ERR_LAST) + 1 ] = { [ _(ERR_INVALID) ] = { "Invalid Error" }, [ _(ERR_INTERNAL) ] = { "Internal Error" }, [ _(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ _(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ _(ERR_NO_CREDS) ] = { "No credentials available" }, [ _(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
d) struct err_string error_to_str[] = { { "Invalid Error" }, /* ERR_INVALID */ { "Internal Error" }, /* ERR_INTERNAL */ { "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */ { "Invalid credential type" }, /* ERR_INVALID_CRED_TYPE */ { "No credentials available" }, /* ERR_NO_CREDS */ { "Credentials are expired" }, /* ERR_CREDS_EXPIRED */
IMHO, the last one is the best, but there is not a compile time check.
LS
I would choose a). It may be less readable than d) but has compile time checks. Both b) and c) are not properly namespaced.
Michal
On (08/09/15 15:21), Michal Židek wrote:
On 09/08/2015 03:09 PM, Lukas Slebodnik wrote:
On (08/09/15 14:57), Michal Židek wrote:
On 08/27/2015 12:52 PM, Lukas Slebodnik wrote:
On (19/08/15 15:57), Michal Židek wrote:
On 08/12/2015 01:11 PM, Lukas Slebodnik wrote:
ehlo,
attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc The comments does not guarantee anything.
Author: Michal Zidek mzidek@redhat.com Date: Tue Mar 10 17:30:48 2015 +0100
DEBUG: Add missing strings for error messages We had more error codes than corresponding messages. Also order of two messages was wrong.Unfortunatelly, the index of enum do not start from 0 and need to be converted using SSSD_ERR_IDX. I used shorter version of macro "idx". Feel free to propose nicer name.
I would use the SSSD_ERR_IDX and not create an alias for it. I think it is always better to have properly namespaced macros even if the are a little longer.
a) the macro is not in public header.
It is int util_errors.h
git grep '#include .*util_errors.h' src/providers/dp_refresh.c:#include "util/util_errors.h" src/providers/ipa/ipa_dyndns.h:#include "util/util_errors.h" src/providers/krb5/krb5_child.c:#include "src/util/util_errors.h" src/tests/sbus_codegen_tests.c:#include "util/util_errors.h" src/tests/sbus_tests.c:#include "util/util_errors.h" src/util/util.h:#include "util/util_errors.h" ^^^^^^^^ So it is almost everywhere with the exception of the client code.
I meant that the local version of SSSD_ERR_IDX is not im public header. So it could not colide with other files.
b) SSSD_ERR_IDX significantly decrease readability
Not properly namespacing macros can bite us in the long term. I do not think that SSSD_ERR_IDX decreases readability, but even if it was the case I would say it is simply necessary. Both SSSD_ and ERR_ prefixes are important here.
OK, try to compare few followinf versions and choose the one which can be easily proceed/understood.
a) struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = { [SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" }, [SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" }, [SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" }, [SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
b) struct err_string error_to_str[ idx(ERR_LAST) + 1 ] = { [ idx(ERR_INVALID) ] = { "Invalid Error" }, [ idx(ERR_INTERNAL) ] = { "Internal Error" }, [ idx(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ idx(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ idx(ERR_NO_CREDS) ] = { "No credentials available" }, [ idx(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
c) struct err_string error_to_str[ _(ERR_LAST) + 1 ] = { [ _(ERR_INVALID) ] = { "Invalid Error" }, [ _(ERR_INTERNAL) ] = { "Internal Error" }, [ _(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ _(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ _(ERR_NO_CREDS) ] = { "No credentials available" }, [ _(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
d) struct err_string error_to_str[] = { { "Invalid Error" }, /* ERR_INVALID */ { "Internal Error" }, /* ERR_INTERNAL */ { "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */ { "Invalid credential type" }, /* ERR_INVALID_CRED_TYPE */ { "No credentials available" }, /* ERR_NO_CREDS */ { "Credentials are expired" }, /* ERR_CREDS_EXPIRED */
IMHO, the last one is the best, but there is not a compile time check.
LS
I would choose a). It may be less readable than d) but has compile time checks. Both b) and c) are not properly namespaced.
I would rather used option d) than option a). (Do not change anything)
Option b) does not require namespacing, because it is a local macro. Option c) is the most readable, but there is a collision with gnu gettext. So it cannot be used.
LS
On 09/08/2015 03:31 PM, Lukas Slebodnik wrote:
I would rather used option d) than option a). (Do not change anything)
Option b) does not require namespacing, because it is a local macro. Option c) is the most readable, but there is a collision with gnu gettext. So it cannot be used.
LS
Only my 2 cents: Isn't compile time check really important feature? Petr
On 09/08/2015 03:31 PM, Lukas Slebodnik wrote:
On (08/09/15 15:21), Michal Židek wrote:
On 09/08/2015 03:09 PM, Lukas Slebodnik wrote:
On (08/09/15 14:57), Michal Židek wrote:
On 08/27/2015 12:52 PM, Lukas Slebodnik wrote:
On (19/08/15 15:57), Michal Židek wrote:
On 08/12/2015 01:11 PM, Lukas Slebodnik wrote: > ehlo, > > attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc > The comments does not guarantee anything. > > Author: Michal Zidek mzidek@redhat.com > Date: Tue Mar 10 17:30:48 2015 +0100 > > DEBUG: Add missing strings for error messages > > We had more error codes than corresponding > messages. Also order of two messages was wrong. > > Unfortunatelly, the index of enum do not start from 0 > and need to be converted using SSSD_ERR_IDX. I used shorter version of > macro "idx". Feel free to propose nicer name.
I would use the SSSD_ERR_IDX and not create an alias for it. I think it is always better to have properly namespaced macros even if the are a little longer.
a) the macro is not in public header.
It is int util_errors.h
git grep '#include .*util_errors.h' src/providers/dp_refresh.c:#include "util/util_errors.h" src/providers/ipa/ipa_dyndns.h:#include "util/util_errors.h" src/providers/krb5/krb5_child.c:#include "src/util/util_errors.h" src/tests/sbus_codegen_tests.c:#include "util/util_errors.h" src/tests/sbus_tests.c:#include "util/util_errors.h" src/util/util.h:#include "util/util_errors.h" ^^^^^^^^ So it is almost everywhere with the exception of the client code.
I meant that the local version of SSSD_ERR_IDX is not im public header. So it could not colide with other files.
Ah, yes sorry, I misunderstood your text.
b) SSSD_ERR_IDX significantly decrease readability
Not properly namespacing macros can bite us in the long term. I do not think that SSSD_ERR_IDX decreases readability, but even if it was the case I would say it is simply necessary. Both SSSD_ and ERR_ prefixes are important here.
OK, try to compare few followinf versions and choose the one which can be easily proceed/understood.
a) struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = { [SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" }, [SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" }, [SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" }, [SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
b) struct err_string error_to_str[ idx(ERR_LAST) + 1 ] = { [ idx(ERR_INVALID) ] = { "Invalid Error" }, [ idx(ERR_INTERNAL) ] = { "Internal Error" }, [ idx(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ idx(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ idx(ERR_NO_CREDS) ] = { "No credentials available" }, [ idx(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
c) struct err_string error_to_str[ _(ERR_LAST) + 1 ] = { [ _(ERR_INVALID) ] = { "Invalid Error" }, [ _(ERR_INTERNAL) ] = { "Internal Error" }, [ _(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ _(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ _(ERR_NO_CREDS) ] = { "No credentials available" }, [ _(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
d) struct err_string error_to_str[] = { { "Invalid Error" }, /* ERR_INVALID */ { "Internal Error" }, /* ERR_INTERNAL */ { "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */ { "Invalid credential type" }, /* ERR_INVALID_CRED_TYPE */ { "No credentials available" }, /* ERR_NO_CREDS */ { "Credentials are expired" }, /* ERR_CREDS_EXPIRED */
IMHO, the last one is the best, but there is not a compile time check.
LS
I would choose a). It may be less readable than d) but has compile time checks. Both b) and c) are not properly namespaced.
I would rather used option d) than option a). (Do not change anything)
Option b) does not require namespacing, because it is a local macro.
Well, I still think that it is better to namespace macros even if they are local. Unlike functions, there are ways to propagate macros by other means than header files so the collision may happen in some cases. But yes that would require someone to use not namespaced macro on place where it would be obviously dangerous (like in Makefile) so we would catch it. This makes option b) acceptable.
Please rebase the patches and if you do not decide to go with option a) (which I would prefer) I can accept the option b) as well.
Option c) is the most readable, but there is a collision with gnu gettext.
Option c) is not readable because it makes people think "What is gettext macro doing here?". The confusion is even bigger because we use human readable text nearby.
So it cannot be used.
LS
On Tue, Sep 08, 2015 at 04:11:26PM +0200, Michal Židek wrote:
On 09/08/2015 03:31 PM, Lukas Slebodnik wrote:
On (08/09/15 15:21), Michal Židek wrote:
On 09/08/2015 03:09 PM, Lukas Slebodnik wrote:
On (08/09/15 14:57), Michal Židek wrote:
On 08/27/2015 12:52 PM, Lukas Slebodnik wrote:
On (19/08/15 15:57), Michal Židek wrote: >On 08/12/2015 01:11 PM, Lukas Slebodnik wrote: >>ehlo, >> >>attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc >>The comments does not guarantee anything. >> >>Author: Michal Zidek mzidek@redhat.com >>Date: Tue Mar 10 17:30:48 2015 +0100 >> >> DEBUG: Add missing strings for error messages >> >> We had more error codes than corresponding >> messages. Also order of two messages was wrong. >> >>Unfortunatelly, the index of enum do not start from 0 >>and need to be converted using SSSD_ERR_IDX. I used shorter version of >>macro "idx". Feel free to propose nicer name. > >I would use the SSSD_ERR_IDX and not create an alias for it. >I think it is always better to have properly namespaced macros >even if the are a little longer. > a) the macro is not in public header.
It is int util_errors.h
git grep '#include .*util_errors.h' src/providers/dp_refresh.c:#include "util/util_errors.h" src/providers/ipa/ipa_dyndns.h:#include "util/util_errors.h" src/providers/krb5/krb5_child.c:#include "src/util/util_errors.h" src/tests/sbus_codegen_tests.c:#include "util/util_errors.h" src/tests/sbus_tests.c:#include "util/util_errors.h" src/util/util.h:#include "util/util_errors.h" ^^^^^^^^ So it is almost everywhere with the exception of the client code.
I meant that the local version of SSSD_ERR_IDX is not im public header. So it could not colide with other files.
Ah, yes sorry, I misunderstood your text.
b) SSSD_ERR_IDX significantly decrease readability
Not properly namespacing macros can bite us in the long term. I do not think that SSSD_ERR_IDX decreases readability, but even if it was the case I would say it is simply necessary. Both SSSD_ and ERR_ prefixes are important here.
OK, try to compare few followinf versions and choose the one which can be easily proceed/understood.
a) struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = { [SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" }, [SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" }, [SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" }, [SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
b) struct err_string error_to_str[ idx(ERR_LAST) + 1 ] = { [ idx(ERR_INVALID) ] = { "Invalid Error" }, [ idx(ERR_INTERNAL) ] = { "Internal Error" }, [ idx(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ idx(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ idx(ERR_NO_CREDS) ] = { "No credentials available" }, [ idx(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
c) struct err_string error_to_str[ _(ERR_LAST) + 1 ] = { [ _(ERR_INVALID) ] = { "Invalid Error" }, [ _(ERR_INTERNAL) ] = { "Internal Error" }, [ _(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ _(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ _(ERR_NO_CREDS) ] = { "No credentials available" }, [ _(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
d) struct err_string error_to_str[] = { { "Invalid Error" }, /* ERR_INVALID */ { "Internal Error" }, /* ERR_INTERNAL */ { "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */ { "Invalid credential type" }, /* ERR_INVALID_CRED_TYPE */ { "No credentials available" }, /* ERR_NO_CREDS */ { "Credentials are expired" }, /* ERR_CREDS_EXPIRED */
IMHO, the last one is the best, but there is not a compile time check.
LS
I would choose a). It may be less readable than d) but has compile time checks. Both b) and c) are not properly namespaced.
I would rather used option d) than option a). (Do not change anything)
Option b) does not require namespacing, because it is a local macro.
Well, I still think that it is better to namespace macros even if they are local. Unlike functions, there are ways to propagate macros by other means than header files so the collision may happen in some cases. But yes that would require someone to use not namespaced macro on place where it would be obviously dangerous (like in Makefile) so we would catch it. This makes option b) acceptable.
Please rebase the patches and if you do not decide to go with option a) (which I would prefer) I can accept the option b) as well.
Option c) is the most readable, but there is a collision with gnu gettext.
Option c) is not readable because it makes people think "What is gettext macro doing here?". The confusion is even bigger because we use human readable text nearby.
This thread has stalled, let's try to revive it. It would be nice to merge the patch.
tl;dr I mostly agree with Michal and would prefer namespaced macro, even though it's a local one.
On (05/10/15 09:35), Jakub Hrozek wrote:
On Tue, Sep 08, 2015 at 04:11:26PM +0200, Michal Židek wrote:
On 09/08/2015 03:31 PM, Lukas Slebodnik wrote:
On (08/09/15 15:21), Michal Židek wrote:
On 09/08/2015 03:09 PM, Lukas Slebodnik wrote:
On (08/09/15 14:57), Michal Židek wrote:
On 08/27/2015 12:52 PM, Lukas Slebodnik wrote: >On (19/08/15 15:57), Michal Židek wrote: >>On 08/12/2015 01:11 PM, Lukas Slebodnik wrote: >>>ehlo, >>> >>>attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc >>>The comments does not guarantee anything. >>> >>>Author: Michal Zidek mzidek@redhat.com >>>Date: Tue Mar 10 17:30:48 2015 +0100 >>> >>> DEBUG: Add missing strings for error messages >>> >>> We had more error codes than corresponding >>> messages. Also order of two messages was wrong. >>> >>>Unfortunatelly, the index of enum do not start from 0 >>>and need to be converted using SSSD_ERR_IDX. I used shorter version of >>>macro "idx". Feel free to propose nicer name. >> >>I would use the SSSD_ERR_IDX and not create an alias for it. >>I think it is always better to have properly namespaced macros >>even if the are a little longer. >> >a) the macro is not in public header.
It is int util_errors.h
git grep '#include .*util_errors.h' src/providers/dp_refresh.c:#include "util/util_errors.h" src/providers/ipa/ipa_dyndns.h:#include "util/util_errors.h" src/providers/krb5/krb5_child.c:#include "src/util/util_errors.h" src/tests/sbus_codegen_tests.c:#include "util/util_errors.h" src/tests/sbus_tests.c:#include "util/util_errors.h" src/util/util.h:#include "util/util_errors.h" ^^^^^^^^ So it is almost everywhere with the exception of the client code.
I meant that the local version of SSSD_ERR_IDX is not im public header. So it could not colide with other files.
Ah, yes sorry, I misunderstood your text.
>b) SSSD_ERR_IDX significantly decrease readability
Not properly namespacing macros can bite us in the long term. I do not think that SSSD_ERR_IDX decreases readability, but even if it was the case I would say it is simply necessary. Both SSSD_ and ERR_ prefixes are important here.
OK, try to compare few followinf versions and choose the one which can be easily proceed/understood.
a) struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = { [SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" }, [SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" }, [SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" }, [SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
b) struct err_string error_to_str[ idx(ERR_LAST) + 1 ] = { [ idx(ERR_INVALID) ] = { "Invalid Error" }, [ idx(ERR_INTERNAL) ] = { "Internal Error" }, [ idx(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ idx(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ idx(ERR_NO_CREDS) ] = { "No credentials available" }, [ idx(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
c) struct err_string error_to_str[ _(ERR_LAST) + 1 ] = { [ _(ERR_INVALID) ] = { "Invalid Error" }, [ _(ERR_INTERNAL) ] = { "Internal Error" }, [ _(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ _(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ _(ERR_NO_CREDS) ] = { "No credentials available" }, [ _(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
d) struct err_string error_to_str[] = { { "Invalid Error" }, /* ERR_INVALID */ { "Internal Error" }, /* ERR_INTERNAL */ { "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */ { "Invalid credential type" }, /* ERR_INVALID_CRED_TYPE */ { "No credentials available" }, /* ERR_NO_CREDS */ { "Credentials are expired" }, /* ERR_CREDS_EXPIRED */
IMHO, the last one is the best, but there is not a compile time check.
LS
I would choose a). It may be less readable than d) but has compile time checks. Both b) and c) are not properly namespaced.
I would rather used option d) than option a). (Do not change anything)
Option b) does not require namespacing, because it is a local macro.
Well, I still think that it is better to namespace macros even if they are local. Unlike functions, there are ways to propagate macros by other means than header files so the collision may happen in some cases. But yes that would require someone to use not namespaced macro on place where it would be obviously dangerous (like in Makefile) so we would catch it. This makes option b) acceptable.
Please rebase the patches and if you do not decide to go with option a) (which I would prefer) I can accept the option b) as well.
Option c) is the most readable, but there is a collision with gnu gettext.
Option c) is not readable because it makes people think "What is gettext macro doing here?". The confusion is even bigger because we use human readable text nearby.
This thread has stalled, let's try to revive it. It would be nice to merge the patch.
tl;dr I mostly agree with Michal and would prefer namespaced macro, even though it's a local one.
The main problem is a readability of code with such macro. It's hard to find name of error code there.
struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = { [ SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" }, [ SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" }, [ SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" }, [ SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
Try to propose something else. The ideal would be something short (or lowecased) as with "_".
struct err_string error_to_str[ _(ERR_LAST) + 1 ] = { [ _(ERR_INVALID) ] = { "Invalid Error" }, [ _(ERR_INTERNAL) ] = { "Internal Error" }, [ _(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ _(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ _(ERR_NO_CREDS) ] = { "No credentials available" }, [ _(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
LS
On Mon, Oct 05, 2015 at 09:50:39AM +0200, Lukas Slebodnik wrote:
On (05/10/15 09:35), Jakub Hrozek wrote:
On Tue, Sep 08, 2015 at 04:11:26PM +0200, Michal Židek wrote:
On 09/08/2015 03:31 PM, Lukas Slebodnik wrote:
On (08/09/15 15:21), Michal Židek wrote:
On 09/08/2015 03:09 PM, Lukas Slebodnik wrote:
On (08/09/15 14:57), Michal Židek wrote: >On 08/27/2015 12:52 PM, Lukas Slebodnik wrote: >>On (19/08/15 15:57), Michal Židek wrote: >>>On 08/12/2015 01:11 PM, Lukas Slebodnik wrote: >>>>ehlo, >>>> >>>>attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc >>>>The comments does not guarantee anything. >>>> >>>>Author: Michal Zidek mzidek@redhat.com >>>>Date: Tue Mar 10 17:30:48 2015 +0100 >>>> >>>> DEBUG: Add missing strings for error messages >>>> >>>> We had more error codes than corresponding >>>> messages. Also order of two messages was wrong. >>>> >>>>Unfortunatelly, the index of enum do not start from 0 >>>>and need to be converted using SSSD_ERR_IDX. I used shorter version of >>>>macro "idx". Feel free to propose nicer name. >>> >>>I would use the SSSD_ERR_IDX and not create an alias for it. >>>I think it is always better to have properly namespaced macros >>>even if the are a little longer. >>> >>a) the macro is not in public header. > >It is int util_errors.h > >git grep '#include .*util_errors.h' >src/providers/dp_refresh.c:#include "util/util_errors.h" >src/providers/ipa/ipa_dyndns.h:#include "util/util_errors.h" >src/providers/krb5/krb5_child.c:#include "src/util/util_errors.h" >src/tests/sbus_codegen_tests.c:#include "util/util_errors.h" >src/tests/sbus_tests.c:#include "util/util_errors.h" >src/util/util.h:#include "util/util_errors.h" > ^^^^^^^^ >So it is almost everywhere with the exception of the >client code. > I meant that the local version of SSSD_ERR_IDX is not im public header. So it could not colide with other files.
Ah, yes sorry, I misunderstood your text.
>>b) SSSD_ERR_IDX significantly decrease readability > >Not properly namespacing macros can bite us in the >long term. I do not think that SSSD_ERR_IDX decreases >readability, but even if it was the case I would say >it is simply necessary. Both SSSD_ and ERR_ prefixes are >important here. >
OK, try to compare few followinf versions and choose the one which can be easily proceed/understood.
a) struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = { [SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" }, [SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" }, [SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" }, [SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
b) struct err_string error_to_str[ idx(ERR_LAST) + 1 ] = { [ idx(ERR_INVALID) ] = { "Invalid Error" }, [ idx(ERR_INTERNAL) ] = { "Internal Error" }, [ idx(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ idx(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ idx(ERR_NO_CREDS) ] = { "No credentials available" }, [ idx(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
c) struct err_string error_to_str[ _(ERR_LAST) + 1 ] = { [ _(ERR_INVALID) ] = { "Invalid Error" }, [ _(ERR_INTERNAL) ] = { "Internal Error" }, [ _(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ _(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ _(ERR_NO_CREDS) ] = { "No credentials available" }, [ _(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
d) struct err_string error_to_str[] = { { "Invalid Error" }, /* ERR_INVALID */ { "Internal Error" }, /* ERR_INTERNAL */ { "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */ { "Invalid credential type" }, /* ERR_INVALID_CRED_TYPE */ { "No credentials available" }, /* ERR_NO_CREDS */ { "Credentials are expired" }, /* ERR_CREDS_EXPIRED */
IMHO, the last one is the best, but there is not a compile time check.
LS
I would choose a). It may be less readable than d) but has compile time checks. Both b) and c) are not properly namespaced.
I would rather used option d) than option a). (Do not change anything)
Option b) does not require namespacing, because it is a local macro.
Well, I still think that it is better to namespace macros even if they are local. Unlike functions, there are ways to propagate macros by other means than header files so the collision may happen in some cases. But yes that would require someone to use not namespaced macro on place where it would be obviously dangerous (like in Makefile) so we would catch it. This makes option b) acceptable.
Please rebase the patches and if you do not decide to go with option a) (which I would prefer) I can accept the option b) as well.
Option c) is the most readable, but there is a collision with gnu gettext.
Option c) is not readable because it makes people think "What is gettext macro doing here?". The confusion is even bigger because we use human readable text nearby.
This thread has stalled, let's try to revive it. It would be nice to merge the patch.
tl;dr I mostly agree with Michal and would prefer namespaced macro, even though it's a local one.
The main problem is a readability of code with such macro. It's hard to find name of error code there.
struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = { [ SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" }, [ SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" }, [ SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" }, [ SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
Looks fine to me.
Try to propose something else. The ideal would be something short (or lowecased) as with "_".
S_ERR_IDX?
struct err_string error_to_str[ _(ERR_LAST) + 1 ] = { [ _(ERR_INVALID) ] = { "Invalid Error" }, [ _(ERR_INTERNAL) ] = { "Internal Error" }, [ _(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ _(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ _(ERR_NO_CREDS) ] = { "No credentials available" }, [ _(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (05/10/15 09:57), Jakub Hrozek wrote:
On Mon, Oct 05, 2015 at 09:50:39AM +0200, Lukas Slebodnik wrote:
On (05/10/15 09:35), Jakub Hrozek wrote:
On Tue, Sep 08, 2015 at 04:11:26PM +0200, Michal Židek wrote:
On 09/08/2015 03:31 PM, Lukas Slebodnik wrote:
On (08/09/15 15:21), Michal Židek wrote:
On 09/08/2015 03:09 PM, Lukas Slebodnik wrote: >On (08/09/15 14:57), Michal Židek wrote: >>On 08/27/2015 12:52 PM, Lukas Slebodnik wrote: >>>On (19/08/15 15:57), Michal Židek wrote: >>>>On 08/12/2015 01:11 PM, Lukas Slebodnik wrote: >>>>>ehlo, >>>>> >>>>>attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc >>>>>The comments does not guarantee anything. >>>>> >>>>>Author: Michal Zidek mzidek@redhat.com >>>>>Date: Tue Mar 10 17:30:48 2015 +0100 >>>>> >>>>> DEBUG: Add missing strings for error messages >>>>> >>>>> We had more error codes than corresponding >>>>> messages. Also order of two messages was wrong. >>>>> >>>>>Unfortunatelly, the index of enum do not start from 0 >>>>>and need to be converted using SSSD_ERR_IDX. I used shorter version of >>>>>macro "idx". Feel free to propose nicer name. >>>> >>>>I would use the SSSD_ERR_IDX and not create an alias for it. >>>>I think it is always better to have properly namespaced macros >>>>even if the are a little longer. >>>> >>>a) the macro is not in public header. >> >>It is int util_errors.h >> >>git grep '#include .*util_errors.h' >>src/providers/dp_refresh.c:#include "util/util_errors.h" >>src/providers/ipa/ipa_dyndns.h:#include "util/util_errors.h" >>src/providers/krb5/krb5_child.c:#include "src/util/util_errors.h" >>src/tests/sbus_codegen_tests.c:#include "util/util_errors.h" >>src/tests/sbus_tests.c:#include "util/util_errors.h" >>src/util/util.h:#include "util/util_errors.h" >> ^^^^^^^^ >>So it is almost everywhere with the exception of the >>client code. >> >I meant that the local version of SSSD_ERR_IDX is not im public >header. So it could not colide with other files.
Ah, yes sorry, I misunderstood your text.
> >>>b) SSSD_ERR_IDX significantly decrease readability >> >>Not properly namespacing macros can bite us in the >>long term. I do not think that SSSD_ERR_IDX decreases >>readability, but even if it was the case I would say >>it is simply necessary. Both SSSD_ and ERR_ prefixes are >>important here. >> > >OK, >try to compare few followinf versions and choose the one which >can be easily proceed/understood. > >a) >struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = { > [SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" }, > [SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" }, > [SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, > [SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, > [SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" }, > [SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" }, > > >b) >struct err_string error_to_str[ idx(ERR_LAST) + 1 ] = { > [ idx(ERR_INVALID) ] = { "Invalid Error" }, > [ idx(ERR_INTERNAL) ] = { "Internal Error" }, > [ idx(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, > [ idx(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, > [ idx(ERR_NO_CREDS) ] = { "No credentials available" }, > [ idx(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" }, > > >c) >struct err_string error_to_str[ _(ERR_LAST) + 1 ] = { > [ _(ERR_INVALID) ] = { "Invalid Error" }, > [ _(ERR_INTERNAL) ] = { "Internal Error" }, > [ _(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, > [ _(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, > [ _(ERR_NO_CREDS) ] = { "No credentials available" }, > [ _(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" }, > > >d) >struct err_string error_to_str[] = { > { "Invalid Error" }, /* ERR_INVALID */ > { "Internal Error" }, /* ERR_INTERNAL */ > { "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */ > { "Invalid credential type" }, /* ERR_INVALID_CRED_TYPE */ > { "No credentials available" }, /* ERR_NO_CREDS */ > { "Credentials are expired" }, /* ERR_CREDS_EXPIRED */ > > >IMHO, the last one is the best, but there is not a compile time check. > >LS
I would choose a). It may be less readable than d) but has compile time checks. Both b) and c) are not properly namespaced.
I would rather used option d) than option a). (Do not change anything)
Option b) does not require namespacing, because it is a local macro.
Well, I still think that it is better to namespace macros even if they are local. Unlike functions, there are ways to propagate macros by other means than header files so the collision may happen in some cases. But yes that would require someone to use not namespaced macro on place where it would be obviously dangerous (like in Makefile) so we would catch it. This makes option b) acceptable.
Please rebase the patches and if you do not decide to go with option a) (which I would prefer) I can accept the option b) as well.
Option c) is the most readable, but there is a collision with gnu gettext.
Option c) is not readable because it makes people think "What is gettext macro doing here?". The confusion is even bigger because we use human readable text nearby.
This thread has stalled, let's try to revive it. It would be nice to merge the patch.
tl;dr I mostly agree with Michal and would prefer namespaced macro, even though it's a local one.
The main problem is a readability of code with such macro. It's hard to find name of error code there.
struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = { [ SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" }, [ SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" }, [ SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" }, [ SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
Looks fine to me.
Try to propose something else. The ideal would be something short (or lowecased) as with "_".
S_ERR_IDX?
It seems that you don't prefer lowercased macro for improving visibility.
I think it would be better if all active developers vote for prefered version. It would affect all of us and we would need to backport it to 1.13 to avoid conflicts with backporting.
If most of developers prefer SSSD_ERR_IDX I will prepare new version.
BTW what about ("__" or prefixed version "sss__")
LS
On Mon, Oct 05, 2015 at 10:25:20AM +0200, Lukas Slebodnik wrote:
On (05/10/15 09:57), Jakub Hrozek wrote:
On Mon, Oct 05, 2015 at 09:50:39AM +0200, Lukas Slebodnik wrote:
On (05/10/15 09:35), Jakub Hrozek wrote:
On Tue, Sep 08, 2015 at 04:11:26PM +0200, Michal Židek wrote:
On 09/08/2015 03:31 PM, Lukas Slebodnik wrote:
On (08/09/15 15:21), Michal Židek wrote: >On 09/08/2015 03:09 PM, Lukas Slebodnik wrote: >>On (08/09/15 14:57), Michal Židek wrote: >>>On 08/27/2015 12:52 PM, Lukas Slebodnik wrote: >>>>On (19/08/15 15:57), Michal Židek wrote: >>>>>On 08/12/2015 01:11 PM, Lukas Slebodnik wrote: >>>>>>ehlo, >>>>>> >>>>>>attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc >>>>>>The comments does not guarantee anything. >>>>>> >>>>>>Author: Michal Zidek mzidek@redhat.com >>>>>>Date: Tue Mar 10 17:30:48 2015 +0100 >>>>>> >>>>>> DEBUG: Add missing strings for error messages >>>>>> >>>>>> We had more error codes than corresponding >>>>>> messages. Also order of two messages was wrong. >>>>>> >>>>>>Unfortunatelly, the index of enum do not start from 0 >>>>>>and need to be converted using SSSD_ERR_IDX. I used shorter version of >>>>>>macro "idx". Feel free to propose nicer name. >>>>> >>>>>I would use the SSSD_ERR_IDX and not create an alias for it. >>>>>I think it is always better to have properly namespaced macros >>>>>even if the are a little longer. >>>>> >>>>a) the macro is not in public header. >>> >>>It is int util_errors.h >>> >>>git grep '#include .*util_errors.h' >>>src/providers/dp_refresh.c:#include "util/util_errors.h" >>>src/providers/ipa/ipa_dyndns.h:#include "util/util_errors.h" >>>src/providers/krb5/krb5_child.c:#include "src/util/util_errors.h" >>>src/tests/sbus_codegen_tests.c:#include "util/util_errors.h" >>>src/tests/sbus_tests.c:#include "util/util_errors.h" >>>src/util/util.h:#include "util/util_errors.h" >>> ^^^^^^^^ >>>So it is almost everywhere with the exception of the >>>client code. >>> >>I meant that the local version of SSSD_ERR_IDX is not im public >>header. So it could not colide with other files.
Ah, yes sorry, I misunderstood your text.
>> >>>>b) SSSD_ERR_IDX significantly decrease readability >>> >>>Not properly namespacing macros can bite us in the >>>long term. I do not think that SSSD_ERR_IDX decreases >>>readability, but even if it was the case I would say >>>it is simply necessary. Both SSSD_ and ERR_ prefixes are >>>important here. >>> >> >>OK, >>try to compare few followinf versions and choose the one which >>can be easily proceed/understood. >> >>a) >>struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = { >> [SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" }, >> [SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" }, >> [SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, >> [SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, >> [SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" }, >> [SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" }, >> >> >>b) >>struct err_string error_to_str[ idx(ERR_LAST) + 1 ] = { >> [ idx(ERR_INVALID) ] = { "Invalid Error" }, >> [ idx(ERR_INTERNAL) ] = { "Internal Error" }, >> [ idx(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, >> [ idx(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, >> [ idx(ERR_NO_CREDS) ] = { "No credentials available" }, >> [ idx(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" }, >> >> >>c) >>struct err_string error_to_str[ _(ERR_LAST) + 1 ] = { >> [ _(ERR_INVALID) ] = { "Invalid Error" }, >> [ _(ERR_INTERNAL) ] = { "Internal Error" }, >> [ _(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, >> [ _(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, >> [ _(ERR_NO_CREDS) ] = { "No credentials available" }, >> [ _(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" }, >> >> >>d) >>struct err_string error_to_str[] = { >> { "Invalid Error" }, /* ERR_INVALID */ >> { "Internal Error" }, /* ERR_INTERNAL */ >> { "Account Unknown" }, /* ERR_ACCOUNT_UNKNOWN */ >> { "Invalid credential type" }, /* ERR_INVALID_CRED_TYPE */ >> { "No credentials available" }, /* ERR_NO_CREDS */ >> { "Credentials are expired" }, /* ERR_CREDS_EXPIRED */ >> >> >>IMHO, the last one is the best, but there is not a compile time check. >> >>LS > >I would choose a). It may be less readable than d) but has >compile time checks. Both b) and c) are not properly namespaced. > I would rather used option d) than option a). (Do not change anything)
Option b) does not require namespacing, because it is a local macro.
Well, I still think that it is better to namespace macros even if they are local. Unlike functions, there are ways to propagate macros by other means than header files so the collision may happen in some cases. But yes that would require someone to use not namespaced macro on place where it would be obviously dangerous (like in Makefile) so we would catch it. This makes option b) acceptable.
Please rebase the patches and if you do not decide to go with option a) (which I would prefer) I can accept the option b) as well.
Option c) is the most readable, but there is a collision with gnu gettext.
Option c) is not readable because it makes people think "What is gettext macro doing here?". The confusion is even bigger because we use human readable text nearby.
This thread has stalled, let's try to revive it. It would be nice to merge the patch.
tl;dr I mostly agree with Michal and would prefer namespaced macro, even though it's a local one.
The main problem is a readability of code with such macro. It's hard to find name of error code there.
struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = { [ SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" }, [ SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" }, [ SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" }, [ SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" }, [ SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" }, [ SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
Looks fine to me.
Try to propose something else. The ideal would be something short (or lowecased) as with "_".
S_ERR_IDX?
It seems that you don't prefer lowercased macro for improving visibility.
I think it would be better if all active developers vote for prefered version. It would affect all of us and we would need to backport it to 1.13 to avoid conflicts with backporting.
If most of developers prefer SSSD_ERR_IDX I will prepare new version.
BTW what about ("__" or prefixed version "sss__")
I don't care about case[*], I care about namespace prefix and knowing what the macro does when I read the name. Namespaced lowecarcase sss_err_i or even s_err_i are fine by me..
[*] My general rule is that macros with side-effects like changing variables or jumping around the code should be uppercase.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek -
Petr Cech