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

>> 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

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

Code:

#ifndef RUN_FUNC1
#else
void function1(void)
{
printk("RUN_FUNC1 defined\n",);
}
#endif

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...

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

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...

>> ... 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

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.

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

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.

Originally Posted by brewbuck

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

Originally Posted by matsp

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.

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.

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;
}