Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.c > #107177 > unrolled thread

singly linked list -- HCL interview question

Back to article view | Back to comp.lang.c

Contents

  singly linked list -- HCL interview question arnuld <sunrise@invalid.address> - 2017-03-27 16:58 +0000
    Re: singly linked list -- HCL interview question Barry Schwarz <schwarzb@dqel.com> - 2017-03-27 11:08 -0700
      Re: singly linked list -- HCL interview question Chad <cdalten@gmail.com> - 2017-03-27 12:02 -0700
      Re: singly linked list -- HCL interview question arnuld <sunrise@invalid.address> - 2017-03-28 03:18 +0000
        Re: singly linked list -- HCL interview question Barry Schwarz <schwarzb@dqel.com> - 2017-03-27 22:59 -0700
          Re: singly linked list -- HCL interview question arnuld <sunrise@invalid.address> - 2017-03-29 04:28 +0000
        Re: singly linked list -- HCL interview question scott@slp53.sl.home (Scott Lurndal) - 2017-03-28 12:56 +0000
      Re: singly linked list -- HCL interview question Ike Naar <ike@iceland.freeshell.org> - 2017-03-28 06:51 +0000
        Re: singly linked list -- HCL interview question arnuld <sunrise@invalid.address> - 2017-03-29 04:31 +0000
          Re: singly linked list -- HCL interview question Keith Thompson <kst-u@mib.org> - 2017-03-29 08:46 -0700

#107177 — singly linked list -- HCL interview question

Hi All, I am back finally :)

Long missed the programming

Any ideas for improving the program ? 
Can I make it better ?


/* A singly-linked-list (SLL) program to add nodes to SLL and remove Nth 
node
 * from the end 
 *
 * HCL Interview Question (2017)
 *
 * e.g Add 10 nodes to a SLL & remove the 7th node starting from the end 
 * 7th node from end is 4th node you added while building SLL
 * 
 * I am using a Stack (LIFO). Last node (the end) is always the latest. 
So, we can
 * go down from there easily. Any better ideas ?
 *
 ***/


#include <stdio.h>
#include <stdlib.h>
#include <errno.h>
#include <limits.h>

struct node {
  long nn;
  struct node* next;
};


struct StPtr {
  struct node* head;
  long tn; /* totoal number of nodes */
};


struct StPtr* st;

void print_diagnostics(void);
void printStack(void);
void convert_to_long(long*, long*, const char*, const char*);
void addNodes(long, long);
void removeNode(long, long);
struct StPtr* createStack(void);


int main(int argc, char* argv[]) {
  if(3 != argc) {
    print_diagnostics();
    return EXIT_FAILURE;
  }
  else {
    st = createStack();
    if(NULL == st) {
      printf("Out of Memory\n");
      return EXIT_FAILURE;
    }
    st->head = NULL;
    long t = 0;
    long r = 0;
    convert_to_long(&t, &r, argv[1], argv[2]);
    addNodes(t,r);
    printStack();
    removeNode(r, t);
    printStack();
  }

  return EXIT_SUCCESS;
}

void removeNode(long rem, long num) {
  if(0 >= rem || (rem > num)) {
    printf("Nothing to remove \n");
  }
  else {
    struct node* t = st->head;
    struct node* prev;
    long i = (t->nn - rem) + 1;
    while(i != t->nn) {
      prev = t;
      t = t->next;
    }

    prev->next = t->next;
    printf("Removing node#: %ld \n", i);
    st->tn--;
    free(t);
  }
}


void addNodes(long num, long rem) {
  if((0 >= num) || (rem > num)){
    printf("Nothing to add\n");
  }
  else {
    for(long i = 1; num >= i; ++i) {

      struct node* p = malloc(1 * (sizeof *p));
      if(NULL == p) {
	printf("Out of memory, will not add node\n");
      }
      else if(NULL == st->head) {
	printf("Adding 1st node\n");
	p->nn = i;
	st->head = p;
	st->tn++;
      }
      else {
	struct node* t = st->head;
	st->head = p;
	p->next = t;
	p->nn = i;
	st->tn++;
      }
      
    }
  }
}
  
   
	      
void convert_to_long(long* num, long* rem, const char* numptr, const 
char* remptr) {
  errno = 0;  /* To distinguish success/failure after call */
  long temp = strtol(numptr, NULL, 0);
  if( ((0 == temp) || (LONG_MAX == temp)) && (ERANGE == errno) ) {
    *num = 0;
    *rem = 0;
  }
  else {
    *num = temp;
    temp = strtol(remptr, NULL, 0);
    if( ((0 == temp) || (LONG_MAX == temp)) && (ERANGE == errno) ) {
    *num = 0;
    *rem = 0;
    }
    else { *rem = temp; } }
}
  


void print_diagnostics(void) {
  printf("Invalid Number of Args\n");
  printf("Provide 2 arguments:\n");
  printf("\t1st arg is number of nodes\n\t2nd arg is nuber of node to be 
removed\n");
}

struct StPtr* createStack(void) {
  struct StPtr* p = malloc(sizeof*p);
  return p;
}

void printStack(){
  struct node* t = st->head;
  for(t = st->head; t; t = t->next) {
    printf("%ld, ", t->nn);
  }
  printf("\n\t---> Total %ld nodes\n", st->tn);
  printf("\n==========================================\n\n\n");
}


========================= OUTPUT ===================================
[arnuld@arch64 programs]$ gcc -std=c99 -pedantic -Wall -Wextra ll.c
[arnuld@arch64 programs]$ ./a.out  3 #
Invalid Number of Args
Provide 2 arguments:
	1st arg is number of nodes
	2nd arg is nuber of node to be removed

[arnuld@arch64 programs]$ ./a.out 10 7
Adding 1st node
10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 
	---> Total 10 nodes

==========================================


Removing node#: 4 
10, 9, 8, 7, 6, 5, 3, 2, 1, 
	---> Total 9 nodes

==========================================

[arnuld@arch64 programs]$ 



-- 
arnuld
http://lispmachine.wordpress.com/

[toc] | [next] | [standalone]

#107189

On 27 Mar 2017 16:58:52 GMT, arnuld <sunrise@invalid.address> wrote:

>Hi All, I am back finally :)
>
>Long missed the programming
>
>Any ideas for improving the program ? 
>Can I make it better ?
>
>
>/* A singly-linked-list (SLL) program to add nodes to SLL and remove Nth 
>node
> * from the end 
> *
> * HCL Interview Question (2017)
> *
> * e.g Add 10 nodes to a SLL & remove the 7th node starting from the end 
> * 7th node from end is 4th node you added while building SLL
> * 
> * I am using a Stack (LIFO). Last node (the end) is always the latest. 
>So, we can
> * go down from there easily. Any better ideas ?
> *
> ***/
>
>
>#include <stdio.h>
>#include <stdlib.h>
>#include <errno.h>
>#include <limits.h>
>
>struct node {
>  long nn;
>  struct node* next;
>};
>
>
>struct StPtr {
>  struct node* head;
>  long tn; /* totoal number of nodes */
>};
>
>
>struct StPtr* st;
>
>void print_diagnostics(void);
>void printStack(void);
>void convert_to_long(long*, long*, const char*, const char*);
>void addNodes(long, long);
>void removeNode(long, long);
>struct StPtr* createStack(void);
>
>
>int main(int argc, char* argv[]) {
>  if(3 != argc) {
>    print_diagnostics();
>    return EXIT_FAILURE;
>  }
>  else {

This else serves no purpose.  When the previous if expression is true,
this code cannot be reached due to the return statement.  When the
expression is false, you want this code to execute unconditionally.

>    st = createStack();

createStack does nothing but call malloc.  Why go through two
functions when you can call malloc here and eliminate createStack
completely?

>    if(NULL == st) {
>      printf("Out of Memory\n");
>      return EXIT_FAILURE;
>    }

Here you recognized that the else is not needed.

>    st->head = NULL;
>    long t = 0;
>    long r = 0;
>    convert_to_long(&t, &r, argv[1], argv[2]);
>    addNodes(t,r);
>    printStack();
>    removeNode(r, t);

Why are the same arguments in different order from addNodes?

>    printStack();
>  }
>
>  return EXIT_SUCCESS;
>}
>
>void removeNode(long rem, long num) {

What is the purpose of num in this function?  See addNodes below.

>  if(0 >= rem || (rem > num)) {
>    printf("Nothing to remove \n");
>  }
>  else {
>    struct node* t = st->head;
>    struct node* prev;
>    long i = (t->nn - rem) + 1;
>    while(i != t->nn) {
>      prev = t;
>      t = t->next;
>    }
>
>    prev->next = t->next;
>    printf("Removing node#: %ld \n", i);
>    st->tn--;
>    free(t);
>  }
>}
>
>
>void addNodes(long num, long rem) {

What is the purpose of rem in this function?  Once you determine it is
smaller than num, you create num nodes regardless of its value.

>  if((0 >= num) || (rem > num)){
>    printf("Nothing to add\n");
>  }
>  else {
>    for(long i = 1; num >= i; ++i) {
>
>      struct node* p = malloc(1 * (sizeof *p));
>      if(NULL == p) {
>	printf("Out of memory, will not add node\n");
>      }
>      else if(NULL == st->head) {
>	printf("Adding 1st node\n");
>	p->nn = i;

You forgot to set p->next to NULL.  This eventually leads to undefined
behavior in printStack when the command line arguments are 10 and 5.

>	st->head = p;
>	st->tn++;
>      }
>      else {
>	struct node* t = st->head;
>	st->head = p;
>	p->next = t;
>	p->nn = i;

If you move the two statements that update members of p above the
first that updates st->head, t becomes unnecessary.

>	st->tn++;
>      }
>      
>    }
>  }
>}
>  
>   
>	      
>void convert_to_long(long* num, long* rem, const char* numptr, const 
>char* remptr) {
>  errno = 0;  /* To distinguish success/failure after call */
>  long temp = strtol(numptr, NULL, 0);
>  if( ((0 == temp) || (LONG_MAX == temp)) && (ERANGE == errno) ) {

This test is inconsistent and does not detect an error you want it to.
If temp is 0, errno is guaranteed not to be ERANGE.  Therefore, the
entire if expression will evaluate to false.  Thus a strtol failure to
perform any conversion (due to invalid input) will go undetected.

In the calling code, you test for both 0 and negative values.  Testing
for 0 here seems a waste.

If you do want to detect the "no-conversion" condition, you need to
provide a meaningful  second argument to strtol and test the object it
points to for equality to numptr.  

If you do that, you might also want to check that the entire input
string participated in the conversion.  As it stands, your code does
not distinguish between "12abc" and "12".

>    *num = 0;
>    *rem = 0;
>  }
>  else {
>    *num = temp;
>    temp = strtol(remptr, NULL, 0);
>    if( ((0 == temp) || (LONG_MAX == temp)) && (ERANGE == errno) ) {
>    *num = 0;
>    *rem = 0;

You mostly use a consistent indent style which makes your code easier
to read.  (thank you)  These two statements should be indented.

>    }
>    else { *rem = temp; } }
>}
>  
>
>
>void print_diagnostics(void) {
>  printf("Invalid Number of Args\n");
>  printf("Provide 2 arguments:\n");
>  printf("\t1st arg is number of nodes\n\t2nd arg is nuber of node to be 
>removed\n");
>}
>
>struct StPtr* createStack(void) {
>  struct StPtr* p = malloc(sizeof*p);
>  return p;
>}
>
>void printStack(){
>  struct node* t = st->head;
>  for(t = st->head; t; t = t->next) {
>    printf("%ld, ", t->nn);
>  }
>  printf("\n\t---> Total %ld nodes\n", st->tn);
>  printf("\n==========================================\n\n\n");
>}
>
>
>========================= OUTPUT ===================================
>[arnuld@arch64 programs]$ gcc -std=c99 -pedantic -Wall -Wextra ll.c
>[arnuld@arch64 programs]$ ./a.out  3 #
>Invalid Number of Args
>Provide 2 arguments:
>	1st arg is number of nodes
>	2nd arg is nuber of node to be removed
>
>[arnuld@arch64 programs]$ ./a.out 10 7
>Adding 1st node
>10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 
>	---> Total 10 nodes
>
>==========================================
>
>
>Removing node#: 4 
>10, 9, 8, 7, 6, 5, 3, 2, 1, 
>	---> Total 9 nodes
>
>==========================================
>
>[arnuld@arch64 programs]$ 

-- 
Remove del for email

[toc] | [prev] | [next] | [standalone]

#107195

This is a weak interview question. 

[toc] | [prev] | [next] | [standalone]

#107242

> On Mon, 27 Mar 2017 11:08:51 -0700, Barry Schwarz wrote:

> This else serves no purpose.  When the previous if expression is true,
> this code cannot be reached due to the return statement.  When the
> expression is false, you want this code to execute unconditionally.

Done

 
> createStack does nothing but call malloc.  Why go through two functions
> when you can call malloc here and eliminate createStack completely?

Done


 
> Why are the same arguments in different order from addNodes?

Done
 

> What is the purpose of num in this function?  See addNodes below.

> What is the purpose of rem in this function?  Once you determine it is
> smaller than num, you create num nodes regardless of its value.

Because removeNode() is being called whether addNode() adds any nodes or 
not. You remove that test from removeNode(), you get segfault.

I realized that when  its "./a.out 10 0" it segfaults. Added one more 
test in there:

if((0 >= num) || (0 >= rem) || (rem > num))

 

> You forgot to set p->next to NULL.  This eventually leads to undefined
> behavior in printStack when the command line arguments are 10 and 5.

Good catch :) ... Done
  

 
> If you move the two statements that update members of p above the first
> that updates st->head, t becomes unnecessary.

You mean:

  p->nn = i;
  p->next = st->head;
  st->head = p;
 
Done.



> This test is inconsistent and does not detect an error you want it to.
> If temp is 0, errno is guaranteed not to be ERANGE.  Therefore, the
> entire if expression will evaluate to false.  Thus a strtol failure to
> perform any conversion (due to invalid input) will go undetected.

> .. SNIP ...

H&S 5 -- 16.4, page 412-13

"If no conversion can be performed, then the functions return zero, *ptr 
is set to the value of *str, and errno is set to ERANGE. If the number to 
be converted would cause an overflow then  LONG_MAX,LONG_MIN,LLONG_MAX, 
LLONG_MIN, ULONG_MAX, or ULLONG_MAX (depending on the function's return 
type and the sign of the value); errno is set to ERANGE

I see there are 2 kinds of failures: (1) no conversion and (2) conversion 
causing overflow

(1) has 3 tests and (2) has 2 tests. Hence I am changing it to:

  errno = 0;  /* To distinguish success/failure after call */
  char* endptr;
  long temp = strtol(numptr, &endptr, 0);
  if( ((0 == temp) && (0 == strcmp(numptr, endptr)) && (errno == ERANGE))
      || ((LONG_MAX == temp) && (ERANGE == errno)) ) {
    *num = 0;
    *rem = 0;
  }



> If you do that, you might also want to check that the entire input
> string participated in the conversion.  As it stands, your code does not
> distinguish between "12abc" and "12".

I thought that was the job of strtol().

As per the program requirement "12abc" is same as "12" and even if 
"12abc" is considered empty, its fine by me. If I am asking for the user 
to enter a number and hi is trying to be too clever then all I need to 
take care is program must not have UB/segfaults. Same goes for input from 
some program to this program.

Though I could be a bit clever by telling user to "You need a nap" if he 
is trying to put anything other than a number

 
Though I don't know what to do about unusually large values. Program 
behaves fine but could there be  better way than this kind of output:

[arnuld@arch64 programs]$ ./a.out 10088754874487498398437985098323 10
Nothing to add

	---> Total 0 nodes

==========================================


Nothing to remove

	---> Total 0 nodes

==========================================




> You mostly use a consistent indent style which makes your code easier to
> read.  (thank you)  These two statements should be indented.
> 
>>    }
>>    else { *rem = temp; } }
>>}

Those are 3 lines but I did not understand you here. I use Emacs auto-
indentation, never ever do any indentation myself.  





-- 
arnuld
http://lispmachine.wordpress.com/

[toc] | [prev] | [next] | [standalone]

#107243

On 28 Mar 2017 03:18:37 GMT, arnuld <sunrise@invalid.address> wrote:

>> On Mon, 27 Mar 2017 11:08:51 -0700, Barry Schwarz wrote:

<snip>

>> This test is inconsistent and does not detect an error you want it to.
>> If temp is 0, errno is guaranteed not to be ERANGE.  Therefore, the
>> entire if expression will evaluate to false.  Thus a strtol failure to
>> perform any conversion (due to invalid input) will go undetected.
>
>> .. SNIP ...
>
>H&S 5 -- 16.4, page 412-13
>
>"If no conversion can be performed, then the functions return zero, *ptr 
>is set to the value of *str, and errno is set to ERANGE. If the number to 
>be converted would cause an overflow then  LONG_MAX,LONG_MIN,LLONG_MAX, 
>LLONG_MIN, ULONG_MAX, or ULLONG_MAX (depending on the function's return 
>type and the sign of the value); errno is set to ERANGE

The quote does not seem right for two reasons.  First, *ptr should be
set to the value of str, not *str.  Second, it is inconsistent with
the standard description from n1570:

"The strtol, strtoll, strtoul, and strtoull functions return the
converted value, if any. If no conversion could be performed, zero is
returned. If the correct value is outside the range of representable
values, LONG_MIN, LONG_MAX, LLONG_MIN, LLONG_MAX, ULONG_MAX, or
ULLONG_MAX is returned (according to the return type and sign of the
value, if any), and the value of the macro ERANGE is stored in errno."

The two if statements are not related and the first does not mention
setting errno.

>I see there are 2 kinds of failures: (1) no conversion and (2) conversion 
>causing overflow

To my mind, there are three kinds of failures: no conversion at all;
converted value out of range (the function never causes overflow); and
conversion terminated prematurely due to an invalid character in the
string detected after conversion has started.

>(1) has 3 tests and (2) has 2 tests. Hence I am changing it to:
>
>  errno = 0;  /* To distinguish success/failure after call */
>  char* endptr;
>  long temp = strtol(numptr, &endptr, 0);
>  if( ((0 == temp) && (0 == strcmp(numptr, endptr)) && (errno == ERANGE))

There is no need for strcmp.  It is sufficient to test if numptr ==
endptr.  If the two pointers are equal, strcmp must return 0.  If they
are not equal, strcmp cannot return 0.

Furthermore, if numptr == endptr, temp must be 0 and the value of
errno is irrelevant.

>      || ((LONG_MAX == temp) && (ERANGE == errno)) ) {

The test on temp is also irrelevant.  If errno == ERANGE, then temp
must be either LONG_MAX or LONG_MIN.  You don't care which.

>    *num = 0;
>    *rem = 0;
>  }
>
>
>
>> If you do that, you might also want to check that the entire input
>> string participated in the conversion.  As it stands, your code does not
>> distinguish between "12abc" and "12".
>
>I thought that was the job of strtol().

Once conversion has started, it only completes when an invalid
character is detected.  If the entire string has been converted, then
endptr will point to the '\0' that terminates the string.  If the
conversion stopped before the nul character, endptr will point to a
value other than '\0'.  strtol doesn't care which of these situations
actually happened.  If your program does care, it needs to test the
character endptr points to.

>As per the program requirement "12abc" is same as "12" and even if 

Surprising but I'll take your word for it.

>"12abc" is considered empty, its fine by me. If I am asking for the user 

strtol will never consider "12abc" empty.  It will parse as 12 and set
endptr to point to the 'a'.

>to enter a number and hi is trying to be too clever then all I need to 
>take care is program must not have UB/segfaults. Same goes for input from 
>some program to this program.
>
>Though I could be a bit clever by telling user to "You need a nap" if he 
>is trying to put anything other than a number

That seems to imply you want to distinguish between a well formed
string and an ill-formed one.
 
>Though I don't know what to do about unusually large values. Program 
>behaves fine but could there be  better way than this kind of output:
>
>[arnuld@arch64 programs]$ ./a.out 10088754874487498398437985098323 10
>Nothing to add
>
>	---> Total 0 nodes

Since you already test errno, you could produce a unique message when
it is set to ERANGE.

>==========================================
>
>
>Nothing to remove
>
>	---> Total 0 nodes
>
>==========================================
>
>
>
>
>> You mostly use a consistent indent style which makes your code easier to
>> read.  (thank you)  These two statements should be indented.
>> 
>>>    }
>>>    else { *rem = temp; } }
>>>}
>
>Those are 3 lines but I did not understand you here. I use Emacs auto-
>indentation, never ever do any indentation myself. 

Here is my statement in context

"
>  else {
>    *num = temp;
>    temp = strtol(remptr, NULL, 0);
>    if( ((0 == temp) || (LONG_MAX == temp)) && (ERANGE == errno) ) {
>    *num = 0;
>    *rem = 0;

You mostly use a consistent indent style which makes your code easier
to read.  (thank you)  These two statements should be indented. "

Notice how the line *num = temp is indented two more spaces than the
preceding line.  This is the preferred practice following the { of a
compound statement.

Now notice how the two lines starting with *num = 0 are indented the
exact same amount as the preceding statement, even though that
statement also has a { to start a compound statement.  These lines
should have been indented an additional two spaces.

Look at the code in addNode to see proper indentation in a similar
nesting situation.  If Emacs can handle it there, it should be able to
handle it here.

-- 
Remove del for email

[toc] | [prev] | [next] | [standalone]

#107358

>> On 28 Mar 2017 03:18:37 GMT, arnuld <sunrise@invalid.address> wrote:
> On Mon, 27 Mar 2017 22:59:26 -0700, Barry Schwarz wrote:

>>H&S 5 -- 16.4, page 412-13
>>
>>"If no conversion can be performed, then the functions return zero, *ptr
>>is set to the value of *str, and errno is set to ERANGE. If the number
>>to be converted would cause an overflow then 
>>LONG_MAX,LONG_MIN,LLONG_MAX, LLONG_MIN, ULONG_MAX, or ULLONG_MAX
>>(depending on the function's return type and the sign of the value);
>>errno is set to ERANGE
 
> The quote does not seem right for two reasons.  First, *ptr should be
> set to the value of str, not *str.  Second, it is inconsistent with the
> standard description from n1570:
> ...SNIP ...
> The two if statements are not related and the first does not mention
> setting errno.

My mistake. It is "str".

I read n1570 and yes you are right, H&S5 and n1570 do not match. I 
checked man page then and came across this note:
 
"Since strtol() can legitimately return 0, LONG_MAX, or LONG_MIN 
(LLONG_MAX or LLONG_MIN for strtoll()) on both success and failure, the  
calling program should set errno to 0 before the call, and then determine 
if an error occurred by checking whether errno has a nonzero value after 
the call."

errno = 0;    /* To distinguish success/failure after call */
val = strtol(str, &endptr, base);
if ((errno == ERANGE && (val == LONG_MAX || val == LONG_MIN))
    || (errno != 0 && val == 0))

That code is in the man page. I think I should change my code according 
to that but still like you said standard does not mention setting errno 
and comparing numptr and endptr is better.


> You mostly use a consistent indent style which makes your code easier to
> read.  (thank you)  These two statements should be indented. "
> 
> Notice how the line *num = temp is indented two more spaces than the
> preceding line.  This is the preferred practice following the { of a
> compound statement.
>  .. SNIP ...

You caught it right :)  , C-x h, Alt-x indent-region solved this.





-- 
arnuld
http://lispmachine.wordpress.com/

[toc] | [prev] | [next] | [standalone]

#107256

arnuld <sunrise@invalid.address> writes:
>> On Mon, 27 Mar 2017 11:08:51 -0700, Barry Schwarz wrote:
>
>> This else serves no purpose.  When the previous if expression is true,
>> this code cannot be reached due to the return statement.  When the
>> expression is false, you want this code to execute unconditionally.
>
>Done
>
> 
>> createStack does nothing but call malloc.  Why go through two functions
>> when you can call malloc here and eliminate createStack completely?
>
>Done
>
>
> 
>> Why are the same arguments in different order from addNodes?
>
>Done
> 
>
>> What is the purpose of num in this function?  See addNodes below.
>
>> What is the purpose of rem in this function?  Once you determine it is
>> smaller than num, you create num nodes regardless of its value.
>
>Because removeNode() is being called whether addNode() adds any nodes or 
>not. You remove that test from removeNode(), you get segfault.
>
>I realized that when  its "./a.out 10 0" it segfaults. Added one more 
>test in there:
>
>if((0 >= num) || (0 >= rem) || (rem > num))
>
> 
>
>> You forgot to set p->next to NULL.  This eventually leads to undefined
>> behavior in printStack when the command line arguments are 10 and 5.
>
>Good catch :) ... Done
>  
>
> 
>> If you move the two statements that update members of p above the first
>> that updates st->head, t becomes unnecessary.
>
>You mean:
>
>  p->nn = i;
>  p->next = st->head;
>  st->head = p;
> 
>Done.
>
>
>
>> This test is inconsistent and does not detect an error you want it to.
>> If temp is 0, errno is guaranteed not to be ERANGE.  Therefore, the
>> entire if expression will evaluate to false.  Thus a strtol failure to
>> perform any conversion (due to invalid input) will go undetected.
>
>> .. SNIP ...
>
>H&S 5 -- 16.4, page 412-13
>
>"If no conversion can be performed, then the functions return zero, *ptr 
>is set to the value of *str, and errno is set to ERANGE. If the number to 
>be converted would cause an overflow then  LONG_MAX,LONG_MIN,LLONG_MAX, 
>LLONG_MIN, ULONG_MAX, or ULLONG_MAX (depending on the function's return 
>type and the sign of the value); errno is set to ERANGE

One way to test whether strtoul was successful:

        uint64 cpu_bar = strtoull(argv[arg], &cp, 0);
        if ((cp == argv[arg])    /* argv[arg] is non-numeric */
         || (*cp != '\0')        /* Trailing non-numeric , non-ws bytes */
         || (cpu_bar != 0)) {    /* Value range error */
            lp->log("Per-cpu bar must be zero, not '%s'\n", argv[arg]);
            return false;
        }

Or, perhaps:

        m_memsize = strtoull(argv[1],  &cp, 0);
        if (strcasecmp(cp, "k") == 0) {
            shift = 10ul;
        } else if (strcasecmp(cp, "m") == 0) {
            shift = 20ul;
        } else if (strcasecmp(cp, "g") == 0) {
            shift = 30ul;
        } else {
            if ((cp == argv[1])
             || (*cp != '\0')
             || (m_memsize == 0ull)) {
                lp->log("Invalid memory size '%s'\n", argv[1]);
                return false;
            }
        }
        m_memsize <<= shift;

[toc] | [prev] | [next] | [standalone]

#107245

On 2017-03-27, Barry Schwarz <schwarzb@dqel.com> wrote:
> On 27 Mar 2017 16:58:52 GMT, arnuld <sunrise@invalid.address> wrote:
>
>> [...]
>>
>>#include <stdio.h>
>>#include <stdlib.h>
>>#include <errno.h>
>>#include <limits.h>
>>
>>struct node {
>>  long nn;
>>  struct node* next;
>>};
>>
>>struct StPtr {
>>  struct node* head;
>>  long tn; /* totoal number of nodes */
>>};
>>
>>
>>struct StPtr* st;
>>
>>void print_diagnostics(void);
>>void printStack(void);
>>void convert_to_long(long*, long*, const char*, const char*);
>>void addNodes(long, long);
>>void removeNode(long, long);
>>struct StPtr* createStack(void);
>>
>>int main(int argc, char* argv[]) {
>>  if(3 != argc) {
>>    print_diagnostics();
>>    return EXIT_FAILURE;
>>  }
>>  else {
>
> This else serves no purpose.  When the previous if expression is true,
> this code cannot be reached due to the return statement.  When the
> expression is false, you want this code to execute unconditionally.
>
>>    st = createStack();
>
> createStack does nothing but call malloc.  Why go through two
> functions when you can call malloc here and eliminate createStack
> completely?

It might be a better idea to keep createStack() and let it not only
allocate the memory, but also initialize the members,
setting 'head' to NULL and 'tn' to 0.
('head' is now initialized in main(), 'tn' is never initialized,
leading to undefined behaviour in the functions that increment or
decrement it).

[toc] | [prev] | [next] | [standalone]

#107359

> On Tue, 28 Mar 2017 06:51:06 +0000, Ike Naar wrote:

> It might be a better idea to keep createStack() and let it not only
> allocate the memory, but also initialize the members,
> setting 'head' to NULL and 'tn' to 0.
> ('head' is now initialized in main(), 'tn' is never initialized, leading
> to undefined behaviour in the functions that increment or decrement it).


This idea was taught to me as a good coding practice by "The Other Keith" 
@ http://www.ghoti.net/~kst 



-- 
arnuld
http://lispmachine.wordpress.com/

[toc] | [prev] | [next] | [standalone]

#107399

arnuld <sunrise@invalid.address> writes:
>> On Tue, 28 Mar 2017 06:51:06 +0000, Ike Naar wrote:
>> It might be a better idea to keep createStack() and let it not only
>> allocate the memory, but also initialize the members,
>> setting 'head' to NULL and 'tn' to 0.
>> ('head' is now initialized in main(), 'tn' is never initialized, leading
>> to undefined behaviour in the functions that increment or decrement it).
>
> This idea was taught to me as a good coding practice by "The Other Keith" 
> @ http://www.ghoti.net/~kst 

FYI, that web page is out of date.  I just haven't bothered updating it
or my .signature.

-- 
Keith Thompson (The_Other_Keith) kst-u@mib.org  <http://www.ghoti.net/~kst>
Working, but not speaking, for JetHead Development, Inc.
"We must do something.  This is something.  Therefore, we must do this."
    -- Antony Jay and Jonathan Lynn, "Yes Minister"

[toc] | [prev] | [standalone]

Back to top | Article view | comp.lang.c

csiph-web