0

I am learning to write programs in professional way. Like, by creating separate .C and .h file.i decided to write a simple code with structures but I am getting errors. I have done these things :

/*---list.h file-------*/
#ifndef LIST_H
#define LIST_H
struct list{
    int a;
    struct list *next;
};
typedef struct list LIST;
LIST *pHead=NULL,*pCurrent=NULL;
void display(void);

#endif


/*---list.c file ---*/
#include "main.h"


    void display()
    {
        pHead->a=100;
        printf("%d",pHead->a);
    }


/*----main.h file-----*/

#ifndef MAIN_H
#define MAIN_H

#include<stdio.h>

#include "list.h"
#endif


/*---main.c file---*/
#include "main.h"


void main(void)
{
LIST *New=pHead;
display();
printf("\n\n%d",New->a);
getch();

}

when i compile the code , I am getting following errors 1>main.obj : error LNK2005: _pCurrent already defined in list.obj 1>main.obj : error LNK2005: _pHead already defined in list.obj

can anyone please tell me what I am doing wrong ? Am I including something twice because of which I am getting redeclaration error ?

3
  • And where does pHead point to ?
    – joop
    May 27, 2014 at 15:12
  • possible duplicate of duplicate symbol error C++
    – Peter G.
    May 27, 2014 at 15:15
  • "I am learning to write programs in professional way" why not start with int main( ) ?
    – P0W
    May 27, 2014 at 15:19

3 Answers 3

2

This is because you define things in your header, as opposed to merely declaring them.

This:

LIST *pHead=NULL,*pCurrent=NULL;

means that every C file that includes the list header, tries to create two global variables. When you then link these C files together, those variables collide. This is broken, you should never do that. Never define things in a header.

1
  • i tried removing LIST *pHead=NULL,*pCurrent=NULL; from list.h and wrote that line in list.c now I am getting more errors
    – Priyanka
    May 27, 2014 at 15:17
2

You defined the objects in a header file and then included them in multiple source files thus breaking the one definition rule.

If you want to create global variables which you can use across different translation units, you should use the extern keyword.

6
  • can you please tell in more detail ? with respect to my code.
    – Priyanka
    May 27, 2014 at 15:16
  • 1
    @Priyanka: Anything that is present in header file gets merely copy pasted to every translation unit where you include the header.In C/C++ you are not allowed to have multiple definitions of a same named symbol(one definition rule),so defining objects(pHead,pCurrent) in a header file creates multiple definitions when you the include header in multiple source files.The correct way to do this is using the extern keyword(The link inline to the ans explains that part in detail).Hope this helps.
    – Alok Save
    May 27, 2014 at 15:21
  • i changed LIST *pHead=NULL,*pCurrent=NULL; to just LIST *pHead;LIST *pCurrent; and the error disappeared.
    – Priyanka
    May 27, 2014 at 15:33
  • 1
    @Priyanka: Because by doing that change, you now only declare and not define the objects.Read(stackoverflow.com/questions/1410563/…) however this doesn't solve your problem because your requirement is same object to be used in different source files, what your change gives you is, each translation unit having its own copy of the objects, though each copy of the object has same name,but they are not same each is different.You will need to use extern to achieve this.Check link in answer for example of it.
    – Alok Save
    May 27, 2014 at 15:39
  • hi alok, u meant I will have to declare as extern struct list e; for making objects in any .C file ?
    – Priyanka
    May 27, 2014 at 16:48
1

Generally speaking, .c files contain embodiment of variables, functions, etc.; while .h files contain prototypes of variables, functions, etc., found in it's companion .c file.

It is generally the case that variable and function bodies are not placed in a .h file; only variable and function prototypes should be placed in .h files.

When considering how to split-up code into separate files, it is important to consider which functions, structures and macros are the most primitive. For example, if you write two functions, and function 'a' calls function 'b', function 'b' is most primitive.

The idea is to group functions into a 'c' file that are related, and are at a similar primitive level.

In the case of this question, the more primitive list functions should be embodied in list.c. Then 'list.h' is used to prototype functions and structures used by other less primitive .c files such as main.c.

The most primitive functions are also the most self sufficient. While less primitive functions should call more primitive functions, the reverse makes for clumsy code-flow.

Now to review the question code:

/*---list.c file ---*/
#include "main.h"

list.c should be considered as more primitive than main.c. Hence, having list.c include main.h is (professionally) not a good idea. list.c, being more primitive should be more self-sufficient.

Rather than including main.h, it would be better for list.c to include it's own list.h so that it has access to it's own `struct list' definition, etc.

void display()
{
    pHead->a=100;
    printf("%d",pHead->a);
}

In order to better isolate list.c, the above function should not reference a 'global' variable (pHead). Rather, it would be better to have the 'node to display' passed into the function as an argument.

With this in mind, here are how 'list.c' and 'list.h' might be improved:

/*---list.h file-------*/
#ifndef LIST_H
 #define LIST_H

typedef struct NODE_S
   {
   int a;
   struct list *next;
   } NODE_T;

typedef struct LIST_S
   {
   NODE_T *head;
   } LIST_T;

extern void NodeDisplay(NODE_T *node);

#endif


/*---list.c file ---*/
#include <stdio.h> // printf()
#include "list.h"  // NODE_T, LIST_T

void NodeDisplay(NODE_T *node)
   {
   printf("%d\n",pHead->a);
   return;
   }

Note that pHead and pCurrent are not prototyped, or embodied, in list.h or list.c Those variables are not used in list.c, and there is no functional reason to place them in list.h


Now examine main.h and main.c as they are in the question code:

/*----main.h file-----*/

#ifndef MAIN_H
 #define MAIN_H

#include<stdio.h>

#include "list.h"
#endif

In isolation, what is the purpose that main.h requires stdio.h and list.h? If they were removed, would there be something left 'undefined' in 'main.h'? Perhaps these two include files don't really belong in main.h. "But if they are removed from main.h, why even have a main.h?" Good point. perhaps main.h serves no purpose and perhaps should not even exist.

The main.c file is the least primitive of all files, and shouldn't generally export anything to other (more primitive) files.

/*---main.c file---*/
#include "main.h"

void main(void)
   {
   LIST *New=pHead;
   display();
   printf("\n\n%d",New->a);
   getch();
   }

So what exactly does main.c need? It needs calls printf(), so it will need to include stdio.h. It calls display(), and references the LIST structure, so it needs list.h.

Yes, those .h files were included in main.h; good point. However, the code will be less clumsy (more professional) if main.c includes exactly what it needs explicitly.

With this philosophy in mind, here is a reworked main.c, without a superfluous main.h:

/*---main.c file---*/
#include <stdio.h>   // printf()
#include <conio.h>   // getch()
#include "list.h"    // NodeDisplay(), LIST_T

int main(void)
   {
   LIST_T pList = 
      {
      .head = NULL
      };

   /* Allocate & Insert a node into the list. */
   NodeCreate(&pList, 100);
   NodeDisplay(pList.head);
   getch();
   return(0);
   }

This version of main.c includes exactly what is required, and appropriately calls less primitive functions. It has no need for 'global variables' because it passes its local storage to more primitive functions as needed.

Oh! you noticed the function NodeCreate()!

While the operation of allocating and inserting a new list node could be performed in main.c, such an operation is most likely a common occurrence that fits nicely with other linked list operations. Hence, add such a function to list.c:

/*---list.c file ---*/
#include <stdio.h>  // printf()
#include <stdlib.h> // malloc()
#include "list.h"   // NODE_T, LIST_T

void NodeDisplay(NODE_T *node)
   {
   printf("%d\n",node->a);
   return;
   }

void NodeCreate(LIST_T *list, int a)
   {
   NODE_T *newNode = malloc(sizeof(*newNode));
   if(NULL == newNode)
      {
      fprintf(stderr, "malloc(newNode) failed.\n");
      goto CLEANUP;
      }

   if(NULL == list)
      {
      fprintf(stderr, "Passing NULL as the list address not allowed.\n");
      goto CLEANUP;
      }

   /* Initialize new node fields (payload) */
   newNode->a = a;

   /* Link newNode as new 'list head' node. */
   newNode->next = list->head ? list->head->next : NULL;
   list->head = newNode;     
   newNode=NULL;

  CLEANUP:

     if(newNode)
        free(newNode);

     return;
     }

And so that this function can be called from the less primitive main.c, add a prototype of the function to list.h:

/*---list.h file-------*/
#ifndef LIST_H
 #define LIST_H

typedef struct NODE_S
   {
   int a;
   struct list *next;
   } NODE_T;

typedef struct LIST_S
   {
   NODE_T *head;
   };

extern void NodeDisplay(NODE_T *node);
extern void NodeCreate(LIST_T *list, int a);

#endif

See spoiler code here.

1
  • nice explnation ,actually now I understood the purpose of making .h corresponding to a .c file :)
    – Priyanka
    May 27, 2014 at 18:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.