On (12/10/15 14:45), Pavel Reichl wrote:
>On 10/12/2015 02:36 PM, Lukas Slebodnik wrote:
>>On (12/10/15 14:06), Pavel Reichl wrote:
>>>
>>>
>>>On 10/12/2015 01:52 PM, Lukas Slebodnik wrote:
>>>>On (12/10/15 13:05), Pavel Reichl wrote:
>>>>>
>>>>>
>>>>>On 10/12/2015 07:32 AM, Lukas Slebodnik wrote:
>>>>>>On (11/10/15 23:11), Pavel Reichl wrote:
>>>>>>>Hello,
>>>>>>>
>>>>>>>while messing around with some code analyses tool I noticed that some
>>>>>>>header files are not including directly header files which they use and
>>>>>>>thus are dependent on being included in particular order.
>>>>>>>
>>>>>>>Please see simple patch attached that fixes some of such files (but
>>>>>>>definitely not all).
>>>>>>
>>>>>>>From 12c273af1c8ffe727efc526c9e993240904bcbf1 Mon Sep 17 00:00:00 2001
>>>>>>>From: Pavel Reichl <reichl.pa...@gmail.com>
>>>>>>>Date: Sun, 11 Oct 2015 22:29:44 +0200
>>>>>>>Subject: [PATCH 1/4] SDAP: add missing header files
>>>>>>>
>>>>>>>---
>>>>>>>src/providers/ldap/ldap_auth.h | 4 ++++
>>>>>>>src/providers/ldap/sdap_async_enum.h | 6 ++++++
>>>>>>>src/providers/ldap/sdap_autofs.h | 6 ++++++
>>>>>>>src/providers/ldap/sdap_id_op.h | 4 ++++
>>>>>>>src/providers/ldap/sdap_sudo.h | 6 ++++++
>>>>>>>src/providers/ldap/sdap_users.h | 4 ++++
>>>>>>>6 files changed, 30 insertions(+)
>>>>>>>
>>>>>>>diff --git a/src/providers/ldap/ldap_auth.h
>>>>>>>b/src/providers/ldap/ldap_auth.h
>>>>>>>index
>>>>>>>5fbddd7087dc65ab8bd1df5fb57492d2fc26d0bb..1848af5de19796c8287117b9afe5c6d3e1a248ec
>>>>>>> 100644
>>>>>>>--- a/src/providers/ldap/ldap_auth.h
>>>>>>>+++ b/src/providers/ldap/ldap_auth.h
>>>>>>>@@ -22,6 +22,10 @@
>>>>>>>
>>>>>>>#include "config.h"
>>>>>>>
>>>>>>>+#include <talloc.h>
>>>>>>>+
>>>>>>>+#include "util/util.h"
>>>>>>>+
>>>>>>util.h already includes talloc.h.
>>>>>>Could you explain why do we need to include both of them?
>>>>>
>>>>>Thanks for noticing that. This seems to be quite common in our code base:
>>>>>git grep -l 'util.h' -- '*.[ch]' | xargs grep 'talloc.h' | wc -l
>>>>>88
>>>>>
>>>>>But it's glitch in the patch nonetheless.
>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>>enum pwexpire {
>>>>>>> PWEXPIRE_NONE = 0,
>>>>>>> PWEXPIRE_LDAP_PASSWORD_POLICY,
>>>>>>>diff --git a/src/providers/ldap/sdap_async_enum.h
>>>>>>>b/src/providers/ldap/sdap_async_enum.h
>>>>>>>index
>>>>>>>2da38f988913fa0d6f252697925e50e05eb794a6..7d4ff82c5e003ffbd14aca76f321a8a0fc41b3d6
>>>>>>> 100644
>>>>>>>--- a/src/providers/ldap/sdap_async_enum.h
>>>>>>>+++ b/src/providers/ldap/sdap_async_enum.h
>>>>>>>@@ -26,6 +26,12 @@
>>>>>>>#ifndef _SDAP_ASYNC_ENUM_H_
>>>>>>>#define _SDAP_ASYNC_ENUM_H_
>>>>>>>
>>>>>>>+#include "config.h"
>>>>>>>+
>>>>>>>+#include <talloc.h>
>>>>>>>+
>>>>>>>+#include "util/util.h"
>>>>>>>+
>>>>>>
>>>>>>util.h already includes config.h and talloc.h.
>>>>>>Could you explain why we need to include all mentioned header files?
>>>>>
>>>>>Same as above. Thanks for noticing.
>>>>>
>>>>>git grep -l 'util.h' -- '*.[ch]' | xargs grep -l 'talloc.h' | xargs grep
>>>>>'config.h' | wc -l
>>>>>23
>>>>>
>>>>>Please see updated patch.
>>>>
>>>>I'm sorry but I still miss an explanation why do we need to include these
>>>>header files. You just updated patches but the explanation is neither in
>>>>mail
>>>>nor in commit message.
>>>>
>>>>LS
>>>
>>>In first mail I wrote:
>>>
>>>>>>> I noticed that some header files are not including directly header
>>>>>>> files which they use and thus are dependent on being included in
>>>>>>> particular order.
>>>
>>>If we have header file A that uses declarations from other header file B and
>>>B is not directly included from A then compilation of source file that
>>>includes header file A will fail unless header file B is included before
>>>header A. This is not desired behavior. I don't think this is a big problem
>>>and I don't think we should spent time actively fixing this. Still I hit this
>>>problem and I fixed it in the files that failed for me. I don't see any
>>>reason why not to merge such improvements. Do you?
>>>
>>I'm not stupid I know why we need to include missing header file.
>I'm sorry if I offended you. It was not my intention.
>
>>But it's not cleaner which header file need to be included and why?
>
>OK, I'll add explanation to commit messages.
>>
>>So could you be more concreate?
>>
>>e.g.: We need to include talloc.h because talloc memory context are used in
>>header file.
>>
>>BTW the main problem with including util.h is that include many header files,
>>which is not necessary in many cases. And header files should not include
>>unnecessary header files. It significantly slow down compilation on slow
>>processors (arm development boards e.g. raspberry pi, ...)
>
>Yes I agree, so if header file is missing just declaration of errno_t then
>including just 'util/util_errors.h' is fine with you?
>
>BTW should every header file contain as first header file 'config.h'?
>
If it is required then it should be included otherwise no.
ATM we do not have any compilation warnings caused by missing header files.
So could you explain why we need to include them? It would make sense
to do it as part of refactoring.
But there are higher priority things which could be done instead.
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
