loop only running once(if at all)

[ostar2]

This program is meant as practice. However, it is not working as intended. The for loop is meant to take the argument from the user and use it as the amount of times to loop. Instead it simply outputting: . It is supposed to output i++ but does not. Any help is much appreciated.

Here is the code:

main.cpp:

#include <iostream>
#include <stdlib.h>
int main(int main, char* arg[], char* arg2[])
{
    int iterator = atoi(arg2[0]);
    std::string z = "-c";

    if (str1 == str2)
    {
        for (int i = 0; i <= iterator; i++)
        {
            std::cout << (i) << std::endl;
        }
    }
    else
    {
        std::cout << "Usage: " << ("Floop") << (" -c ") << ("10") << std::endl;
    }
    return 0;
}

[keskiverto]

I had to look up that unfamiliar form of main. http://en.m.wikipedia.org/wiki/Main_function

Command line parameters are all in arg, not in your arg2.

[TheIdeasMan]

I am trying to figure out how you managed to get it to compile:

1. There is no string header included (maybe your compiler has it in iostream)
2. line 3 is weird, 1 convention is this:

int main(int argc, char **argv)

that might be why line 5 probably doesn't do anything

3. variables str1 & str2 are not declared.
4. Don't use iterator as a variable name - it has a meaning in C++.

HTH

[ostar2]

I changed some stuff around and now it outputs 0 through 3.

#include <iostream>
#include <stdlib.h>
int main(int arg1, char* arg)
{
    if (atoi(arg) == atoi("-c"))
    {
        for (int i = 0; i <= arg1; i++)
        {
            std::cout << (i) << std::endl;
        }
    }
    else
    {
        std::cout << "Usage: " << ("Floop") << (" -c ") << ("10") << std::endl;
    }
    return 0;
}

[TheIdeasMan]

You are not using the command line parameters correctly. Why are you doing something radically different to normal conventions?

The first one is an argument count (argc), including the program name - it is always at least 1. You should always test this at the start - what if argc is 1?

The second one is an array of pointer to char - which holds all the actual arguments. Maybe this form is better:

int main (int argc, char *argv[]){

return 0;
}

argv[0] is the program name.

The idiom for doing something n times is this:

int a; //variable a stays in scope after the for loop
int n = 10;
for(a=0; a < n; a++){
    //do something
}

If you use the <= operator, you run the risk of going past the end of an array, or doing 1 more operation than intended. This is a very common source of error.

[ostar2]

Ok, took your advice and tried this:

#include <stdio.h>
#include <stdlib.h>
int main(int arg, char *arg1[])
{
    if (atoi(arg1[1]) == atoi("-c") && (arg > 2))
    {
        for (int i = 0; i < atoi(arg1[2]); ++i)
        {
            printf("\n%i",i);
        }
        printf("\n");
    }
    else if (atoi(arg1[1]) != atoi("-c") || (arg < 3))
    {
        printf("Usage: Loop -c 10\n");
    }
    return 0;
}

Now when I run it without arguments it segmentationfaults. Otherwise it works.

[TheIdeasMan]

Can you tell us what the atoi call does, and specifically why it doesn't work with a string literal containing alpha chars?

I would be happier if you changed your code to this:

int main(int argc, char *argv[])

just to keep with the normal convention. Conventions are somewhat important because everyone immediately knows what you mean. I could change it to this:

int main(int Fred, char *Ginger[])

but that wouldn't be very helpful would it?

It would also be better to have your else if & if swapped around - do error checking & validation first.

HTH

[keskiverto]

It would also be better to have your else if & if swapped around - do error checking & validation first.

This in other words:
expr1 && expr2
This evaluates expr1 first. Not only that, if expr1 is false it already knows that the AND (&&) will fail and lazily does not even bother to evaluate expr2.

Now, in your code you do dereference arg1[1] and check that arg > 2. arg1[1] exists only if arg > 1. If arg is 1 (you did not give any arguments to the program), there would be no arg1[1] and dereferencing it would be an error. Therefore one should check that value exists before you try to use it, and not the other way around.

if ( 1 < argc ) {
  if ( argv[1] == ... 
}
// Shorter, with logical AND:
if ( 1 < argc && argv[1] == ... )

[TheIdeasMan]

@keskiverto

Yes that is a more detailed explanation, but I have a problem with this (might just be a typo):

if ( 1 < argc ) {

Why wouldn't one do this:

if ( argc < 1 ) {

Not trying to be pedantic, I only mentioned it because it looks like it is a lvalue rvalue issue.

[keskiverto]

You mean

if ( 1 < argc ) ..
// or
if ( argc > 1 ) ..

Both are ok, because neither is an assignment. Lets look at the other usual comparison:

if ( 1 == argc ) ..
// or
if ( argc == 1 ) ..

Still fine. Now, I make a typo writing them:

if ( 1 = argc ) ..
// or
if ( argc = 1 ) ..

The compiler will bark about the first, but accept the second and the code probably does unexpected things. Therefore, it is safer to systematically have the const expression on the left side.

The second thing is that for more complex types one probably defines operator< and operator== and then relies templates in <utility> to get the other relational operators.
http://www.cplusplus.com/reference/utility/rel_ops/
That makes < teeny weeny bit nicer than >.


@ostar2:
"-c" is a string value. Converting it into std::string enables the use of std::string::operator== (const char *).

[booradley60]

keskiverto wrote:

The compiler will bark about the first, but accept the second and the code probably does unexpected things. Therefore, it is safer to systematically have the const expression on the left side.

Ha, aka "Yoda Conditions." http://www.codinghorror.com/blog/2012/07/new-programming-jargon.html

[ostar2]

I fixed it:

#include <stdio.h>
#include <stdlib.h>
int main(const int arg, const char *arg1[])
{
    if (arg == 3)
    {
        /*  convert both to int to avoid
        compilation error and  make command match */
        if (atoi(arg1[1]) == atoi("-c"))
        {
            for (int i = 0; i < atoi(arg1[2]); ++i)
            {
                printf("%i\n",i);
            }
        }
    }
    else
    {
        printf("Usage: %s -c 10\n", arg1[0]);
    }
    return 0;
}

[TheIdeasMan]

I fixed it:

Not at all.

http://www.cplusplus.com/reference/cstdlib/atoi/

Always read the reference material about the functions you use.

The atoi (ascii to int) function converts a string literal representing an integer into an int. if you send it something that is not an integer then it returns 0. So the condition is always true when you send both atoi functions alpha chars. So this would be true:

arg1[1] = "Wrong Argument";

if ( atoi(arg1[1]) == atoi("-c") ) //true 

Use the strcmp function to avoid the undefined behaviour.

With functions like atoi, scanf etc that return values which show their success, always test this return value before doing anything with the result.

When you write code, test it with all kinds of good & bad values.

And will you change line 3 to :

int main( int argc, char *argv[])

to reduce my irritation?

I have never seen anyone put const in front of command line arguments before, but I suppose it won't hurt.

[ostar2]

I changed line 3 and also used strcomp. Now when an invalid option is given the usage is not displayed. It also segfaults if no arguments are given.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
int main(const int argc, const char *argv[])
{
    int value = strcmp(argv[1],"-c");
    if (argc == 3)
    {
        while (value == 0)
        {
            for (int i = 0; i < atoi(argv[2]); ++i)
            {
                printf("%i\n",i);
            }
            value += 1;
        }
    }
    else
    {
        printf("Usage: %s -c 10\n", argv[0]);
    }
    return 0;
}

[keskiverto]

Of course.

We did already tell you that you cannot use argv[1] before you know that it does exist. Hence the crash.

You print usage only if the argc is not 3. Now you expressed desire to print usage also when argc is 3, but arguments are invalid.

[ostar2]

This time I fixed it:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
int main(const int argc, const char *argv[])
{
    if (argc == 3)
    {
        if (atoi(argv[1]) == atoi("-c"))
        {
            for (int i = 0; i < atoi(argv[2]); ++i)
            {
                printf("%i\n",i);
            }
        }
    }
    else
    {
        printf("Usage: %s -c 10\n", argv[0]);
    }
    return 0;
}

Invalid command test: Loop, Loop -c and
Loop -b
Output:
Usage: Loop -c 10
Valid command test: Loop -c 10
Output:

0
1
2
3
4
5
6
7
8
9

See I fixed it.

[booradley60]

What does Loop -b 10 do?

[booradley60]

ostar2 wrote:

This program is meant as practice.

Then practice using the string and stream facilities provided by the standard library. Make your life simpler!

#include <iostream>
#include <string>

int main(int argc, char *argv[])
{
    const unsigned int NUM_EXPECTED_ARGS = 3;

    if (argc == NUM_EXPECTED_ARGS)
    {
        std::string arg1(argv[1]);
        std::string arg2(argv[2]);

        if (arg1 == "-c")
        {
            for (int i = 0; i < atoi(arg2.c_str()); ++i)
            {
                std::cout << i << "\n";
            }
        }
        else
        {
            std::cout << "Unrecognized flag: " << arg1 << "\n";
            std::cout << "Usage: " << argv[0] << " -c <number>\n";
        }
    }
    else
    {
        std::cout << "Incorrect number of arguments. Expected " << NUM_EXPECTED_ARGS << " args, but got " << argc;
        (argc == 1) ? std::cout << " arg.\n" : std::cout << " args.\n";
        std::cout << "Usage: " << argv[0] << " -c <number>\n";
    }
    return 0;
}

EDIT: fixed a bug

[keskiverto]

String and stream? Then that last 'atoi' should go too.

int count = 0;
std::istringstream iss( arg2 );
iss >> count;

[booradley60]

        //...
        std::istringstream arg1(argv[1]);
        std::istringstream arg2(argv[2]);
        int arg2Num;

        if (arg1.str() == "-c" && arg2 >> arg2Num)
        {
            for (int i = 0; i < arg2Num; ++i)
            {
                std::cout << i << "\n";
            }
        }
        //...