Thread: ifdef is harmful - but simple workaround is not working

  1. #1
    Registered User
    Join Date
    Oct 2006
    Posts
    12

    ifdef is harmful - but simple workaround is not working

    Hi,
    I need to write a simple function which will work only when a specific definition is defined, but i can't use ifdef in C file according to linux coding guidelines
    so i wrote this example but i keep getting this error:
    file.c: error: redefinition of 'function1'
    file.h: error: previous definition of 'function1' was here

    in this example all i want is that function1 will be called only if RUN_FUNC1 is defined, else i want the inline function1 to be called.


    Code:
    file.h
    ...
    #ifdef RUN_FUNC1
    void function1(void);
    #else
    static inline void function1(void)
    {
    printk("RUN_FUNC1 not defined\n");
    };
    #endif
    ...
    
    file.c
    ...
    void function1(void)
    {
    printk("RUN_FUNC1 defined\n",);
    }
    ...
    any ideas?
    10x

  2. #2
    Registered User Codeplug's Avatar
    Join Date
    Mar 2003
    Posts
    4,981
    >> but i can't use ifdef in C file according to linux coding guidelines
    Seems silly to allow it in headers but not source...
    Code:
    file.h
    ...
    #ifdef RUN_FUNC1
    void function1(void);
    #else
    inline void function1(void)
    {
    printk("RUN_FUNC1 not defined\n");
    }
    #endif
    ...
    
    file.c
    ...
    #ifdef RUN_FUNC1
    void function1(void)
    {
    printk("RUN_FUNC1 defined\n",);
    }
    #endif
    ...
    This approach requires it in both.

    gg

  3. #3
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    I'm with Codeplug here, it's pretty pointless to have such a rule. However, if that IS the rule, then you need to find another solution.

    One of those would be to have a define on the command-line that is only used when compiling the .c file, not other files using the .h file. Let's call it "HIDE_FUNC1" and do this in the .h file:
    Code:
    #if defined(HIDE_FUNC1) && !defined(RUN_FUNC1)
    #define FUNC1_NAME not_function1
    #else
    #define FUNC1_NAME function1
    #define
    then do this in the .c file:
    Code:
    void FUNC1_NAME(void)
    However, I'm not sure what you are actually trying to achieve, which makes it hard to give good advice.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  4. #4
    & the hat of GPL slaying Thantos's Avatar
    Join Date
    Sep 2001
    Posts
    5,681
    Code:
    #ifndef RUN_FUNC1
    #else
    void function1(void)
    {
    printk("RUN_FUNC1 defined\n",);
    }
    #endif

  5. #5
    Registered User
    Join Date
    Oct 2006
    Posts
    12
    According to linux coding guidelines:
    No ifdefs in .c Code
    With the wide number of different processors, different configuration options and variations of the same base hardware types that Linux runs on, it is easy to start having a lot of #ifdef statements in your code. This is not the proper thing to do. Instead, place the #ifdef in a header file, and provide empty inline functions if the code is not to be included.


    http://www.linuxjournal.com/article/5780

    Again, what i'm trying to do is the following:
    Every time i call function1 and RUN_FUNC1 is defined - I want to run function1 which is defined in file.c
    Every time i call function1 and RUN_FUNC1 is NOT defined - I want to run the static function1 which is defined in file.h
    All of these without using ifdef in the c file...

  6. #6
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    So, another solution would be to have another include file that, depending on the value of the RUN_FUNC1 either implements the function, or implements the "dummy" function.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  7. #7
    Registered User
    Join Date
    Oct 2006
    Posts
    12
    I don't want to implement the function in a header file, this is not the right way..
    I'm sure there is a better solution...

  8. #8
    Registered User Codeplug's Avatar
    Join Date
    Mar 2003
    Posts
    4,981
    >> ... and provide empty inline functions if the code is not to be included.
    "Included" in this context means that function1() does not exist in any of the source files. In other words, you would conditionally compile and link a "function1.c" based on RUN_FUNC1 being defined or not.

    The intent of the rule is for readability and maintainability. Having #ifdef surrounding the entire function is better then having them within the function body. I think you could relax the rule for that case.

    gg

  9. #9
    Registered User
    Join Date
    Oct 2006
    Posts
    12
    No, "Included" in this context means that function1() can exist in the source files , and the linkage should be to the inline "dummy" function instead of the "real" function.

    In the pervious link there is an example:


    As an example, consider the following code in drivers/usb/hid-core.c:
    Code:
    static void hid_process_event (struct hid_device *hid,
                                   struct hid_field *field,
                                   struct hid_usage *usage,
                                   __s32 value)
    {
       hid_dump_input(usage, value);
       if (hid->claimed & HID_CLAIMED_INPUT)
             hidinput_hid_event(hid, field, usage, value);
    #ifdef CONFIG_USB_HIDDEV
       if (hid->claimed & HID_CLAIMED_HIDDEV)
             hiddev_hid_event(hid, usage->hid, value);
    #endif
    }
    Here the author does not want to call hiddev_hid_event() if a specific configuration option is not enabled. This is because that function will not even be present if the configuration option is not enabled.

    To remove this #ifdef, the following change was made to include/linux/hiddev.h:
    Code:
    #ifdef CONFIG_USB_HIDDEV
       extern void hiddev_hid_event (struct hid_device *,
                                     unsigned int usage,
                                     int value);
    #else
       static inline void
       hiddev_hid_event (struct hid_device
    *hid,
                         unsigned int usage,
                         int value) { }
    #endif
    Then drivers/usb/hid-core.c was changed to:
    Code:
    static void hid_process_event
                               (struct hid_device *hid,
                                struct hid_field *field,
                                struct hid_usage *usage,
                                __s32 value)
    {
       hid_dump_input(usage, value);
       if (hid->claimed & HID_CLAIMED_INPUT)
             hidinput_hid_event(hid, field, usage, value);
       if (hid->claimed & HID_CLAIMED_HIDDEV)
             hiddev_hid_event(hid, usage->hid, value);
    }
    If CONFIG_USB_HIDDEV is not enabled, the compiler will replace the call to hiddev_hid_event() with a null function call and then optimize away the if statement entirely. This keeps the code readable and easier to maintain over time.

  10. #10
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Yes, but the whole idea of having all ifdefs in header files is the EITHER the function is implemented as an empty inline in the header file, or it's implemented as a real function in your .c file. So your .c file is then should not have this function.

    Another variant would be to implement function1 in a separate file, and then only link that file in if you need function1, but my guess is that you will reject that idea too, at which point you will have to contact someone who maintains the sources for that file/component and see what they have to say about it.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  11. #11
    Officially An Architect brewbuck's Avatar
    Join Date
    Mar 2007
    Location
    Portland, OR
    Posts
    7,396
    Your problem is imaginary. Just don't use the #ifdefs in the .c file. It's harmless to have a module which contains an implementation of some function which has been inlined elsewhere -- it just won't get linked.
    Code:
    //try
    //{
    	if (a) do { f( b); } while(1);
    	else   do { f(!b); } while(1);
    //}

  12. #12
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by brewbuck View Post
    Your problem is imaginary. Just don't use the #ifdefs in the .c file. It's harmless to have a module which contains an implementation of some function which has been inlined elsewhere -- it just won't get linked.
    Good point - just add a "disable warning" pragma to the turn off the warning (or don't, if warnings are "ok").

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  13. #13
    Officially An Architect brewbuck's Avatar
    Join Date
    Mar 2007
    Location
    Portland, OR
    Posts
    7,396
    Quote Originally Posted by matsp View Post
    Good point - just add a "disable warning" pragma to the turn off the warning (or don't, if warnings are "ok").
    You could avoid the warning altogether by not #including file.h from file.c at all -- so file.c will never see that the function was defined inline. You have to manually keep prototypes in sync with definitions anyway, so this doesn't change anything from a maintenance perspective.

    This would require breaking each such optional function into its own .h/.c pair, but you should do that anyway.
    Code:
    //try
    //{
    	if (a) do { f( b); } while(1);
    	else   do { f(!b); } while(1);
    //}

  14. #14
    Banned master5001's Avatar
    Join Date
    Aug 2001
    Location
    Visalia, CA, USA
    Posts
    3,685
    You do understand that this is an issue of guideline and not a rule. Like Linus Torvalds is not going to jump out of the bushes with a crow bar shouting obsceneties or anything. Its just bad form.

  15. #15
    Registered User
    Join Date
    Oct 2006
    Posts
    12
    This guideline is a rule in my company, and i can't avoid getting the warning because its not a warning its an error..

    anyway i've searched in kernel and found this example that for some reason compiled without any errors, can you explain why?


    just look on function hiddev_connect, if CONFIG_USB_HIDDEV is defined, then when calling hiddev_connect - this will call hiddev_connect from hiddev.c
    and if CONFIG_USB_HIDDEV is not defined, then when calling hiddev_connect - this will call the inline function from hiddev.h

    why didn't this cause the redefinition error in the compilation?...


    in include/linux/hiddev.h
    Code:
    #ifdef CONFIG_USB_HIDDEV
    int hiddev_connect(struct hid_device *);
    void hiddev_disconnect(struct hid_device *);
    void hiddev_hid_event(struct hid_device *hid, struct hid_field *field,
    		      struct hid_usage *usage, __s32 value);
    void hiddev_report_event(struct hid_device *hid, struct hid_report *report);
    int __init hiddev_init(void);
    void hiddev_exit(void);
    #else
    static inline int hiddev_connect(struct hid_device *hid) { return -1; }
    static inline void hiddev_disconnect(struct hid_device *hid) { }
    static inline void hiddev_hid_event(struct hid_device *hid, struct hid_field *field,
    		      struct hid_usage *usage, __s32 value) { }
    static inline void hiddev_report_event(struct hid_device *hid, struct hid_report *report) { }
    static inline int hiddev_init(void) { return 0; }
    static inline void hiddev_exit(void) { }
    #endif
    in drivers/hid/usbhid/hiddev.c
    Code:
    int hiddev_connect(struct hid_device *hid)
    {
    	struct hiddev *hiddev;
    	struct usbhid_device *usbhid = hid->driver_data;
    	int i;
    	int retval;
    
    	for (i = 0; i < hid->maxcollection; i++)
    		if (hid->collection[i].type ==
    		    HID_COLLECTION_APPLICATION &&
    		    !IS_INPUT_APPLICATION(hid->collection[i].usage))
    			break;
    
    	if (i == hid->maxcollection && (hid->quirks & HID_QUIRK_HIDDEV) == 0)
    		return -1;
    
    	if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
    		return -1;
    
    	retval = usb_register_dev(usbhid->intf, &hiddev_class);
    	if (retval) {
    		err_hid("Not able to get a minor for this device.");
    		kfree(hiddev);
    		return -1;
    	}
    
    	init_waitqueue_head(&hiddev->wait);
    	INIT_LIST_HEAD(&hiddev->list);
    	spin_lock_init(&hiddev->list_lock);
    	hiddev->hid = hid;
    	hiddev->exist = 1;
    
    	hid->minor = usbhid->intf->minor;
    	hid->hiddev = hiddev;
    
    	hiddev_table[usbhid->intf->minor - HIDDEV_MINOR_BASE] = hiddev;
    
    	return 0;
    }

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. unable to get simple program working
    By toom in forum Linux Programming
    Replies: 1
    Last Post: 10-11-2003, 05:05 AM
  2. simple macro not working???
    By COBOL2C++ in forum C++ Programming
    Replies: 9
    Last Post: 09-10-2003, 01:43 PM
  3. Replies: 5
    Last Post: 02-02-2003, 10:56 AM
  4. Simple Program not working right (in C)
    By DruzeTito in forum C Programming
    Replies: 5
    Last Post: 06-01-2002, 10:14 PM
  5. simple program not working
    By Unregistered in forum Windows Programming
    Replies: 2
    Last Post: 03-04-2002, 11:36 PM