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 ?

Priyanka
  • 307
  • 2
  • 16

3 Answers3

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.

unwind
  • 391,730
  • 64
  • 469
  • 606
  • 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 '14 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.

Community
  • 1
  • 1
Alok Save
  • 202,538
  • 53
  • 430
  • 533
  • can you please tell in more detail ? with respect to my code. – Priyanka May 27 '14 at 15:16
  • 1
    @Priyanka: Anything that is present in header file gets merely copy pasted to every [translation unit](http://stackoverflow.com/questions/1106149/what-is-a-translation-unit-in-c) 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 '14 at 15:21
  • i changed LIST *pHead=NULL,*pCurrent=NULL; to just LIST *pHead;LIST *pCurrent; and the error disappeared. – Priyanka May 27 '14 at 15:33
  • 1
    @Priyanka: Because by doing that change, you now only ***declare*** and not ***define*** the objects.Read(http://stackoverflow.com/questions/1410563/what-is-the-difference-between-a-definition-and-a-declaration) 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 '14 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 '14 at 16:48
  • In a nutshell: You declare the object to be shared accross multiple files in one of the header .h files with an extern,you define this object exactly in one source cpp file and you include the first header .h file in all cpp files where you want to use the object. – Alok Save May 27 '14 at 16:58
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.

BenMorel
  • 34,448
  • 50
  • 182
  • 322
Mahonri Moriancumer
  • 5,993
  • 2
  • 18
  • 28