Skip to content

Conversation

pavelkryukov
Copy link
Contributor

Hallo

In our project, we use some wrappers around boost_program_options and have tests for them, like that:

TEST( config_parse,  Pass_Valid_Args_2)
{
    const uint64 mandatory_int_value = 356;
    const std::string mandatory_string_value{ "run_test.elf"};

    const char* argv[] =
    {
        "mipt-mips",
        "-b", "run_test.elf",
        "-n", "356",
        "-d"
    };
    const int argc = countof(argv);

    // should not throw any exceptions
    ASSERT_NO_THROW( config::handleArgs( argc, argv));

    ASSERT_EQ( config::uint64_config, mandatory_int_value);
    ASSERT_FALSE( mandatory_string_value.compare( config::string_config));
    ASSERT_EQ( config::bool_config_1, true);
    ASSERT_EQ( config::bool_config_2, false);
}

I decided to switch to Popl, as it is more portable than Boost, has no RTTI requirements, has less weight etc. In order to continue to use these tests, I had to add const-correctness to Popl, as I removing it from array initialization is forbidden by C++ standard:

unit_test.cpp:85:5: error: ISO C++ forbids converting a string constant to ‘char*’ [-Werror=write-strings]

I find that change quite useful, so I open a pull request.

@badaix
Copy link
Owner

badaix commented May 12, 2018

I tried it and compilation fails with

popl_example.cpp:38:21: error: invalid conversion from ‘char**’ to ‘const char**’

Since the main function must have the following (non-const) signature

int main()               // (1)
int main(int, char*[])   // (2)

The argv must be casted to const char**
I checked how getopt solves this, and they are using

int getopt(int argc, char * const argv[], const char *optstring);

When changing the parse function to

void parse(int argc, char* const argv[]);

it compiles and works fine. Can you please test this in your unit test framework?

@pavelkryukov
Copy link
Contributor Author

Yes, I found that independently, pushing a fixed version

@badaix
Copy link
Owner

badaix commented May 12, 2018

I'm confused of all these consts :)

your PR is using

const char * const * argv

which should be equivalent to

const char * const argv[] // constant array of constant pointer to char

which is different (i.e. more constant) to the getopt version

char * const argv[]   // array of constant pointer to char

is there an advantage in your version compared to the getopt version (which is part of the glibc)?
Both are compiling without warning on clang and gcc and I don't fully understand why there is no cast to const necessary, but because the arguments from main are not const, my stomach is pointing me to the getopt char * const argv[] version.

@pavelkryukov
Copy link
Contributor Author

pavelkryukov commented May 12, 2018

There are several advantages of const char * const argv[] over char * const argv[]:

  1. The first explicitly says to user of the parse method: "your data will not be modified by any mean, so after calling these your array remains untouched". Without the 1st const qualifier, you allow parse to modify values (**argv = 'Ж'). It is not that function does at the moment, but such harmful behavior may be introduced intentionally or unintentionally, and the const would protect us from that.
  2. As there is a guarantee of constantness, compiler may do more agressive optimizations.
  3. It solves my problems I described in PR message above: array of C-string literals cannot be converted to char* argv[], it is always const char* argv[].
  4. Finally, char * const argv[] is implicitly convertable to const char * const argv[], so you do no harm by this.

@pavelkryukov
Copy link
Contributor Author

pavelkryukov commented May 12, 2018

because the arguments from main are not const

It allows users handle argv in-place. For example, one may modify them before pushing into Popl.

@badaix badaix changed the base branch from master to develop May 12, 2018 19:36
@badaix badaix merged commit 86afe9c into badaix:develop May 12, 2018
@badaix
Copy link
Owner

badaix commented May 12, 2018

cool, convinced, thanks :)

@pavelkryukov pavelkryukov deleted the master branch May 12, 2018 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants