Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I made an adapter function to convert a const char * to char ** splitting the words in the initial string at each white space (leading, consecutive and trailing white spaces should be ignored).

#include <iostream>
#include <vector>
#include <cctype>

void str2arg(const char * str, std::vector<char*> & arg, std::string & memory) {
  arg.clear();
  memory.clear();
  bool copied_first_char = false;
  for ( size_t i = 0; str[i]!='\0'; ++i) {
    if (isspace(str[i])) {
      if (copied_first_char) {
        if (!isspace(str[i+1])) {
          memory += '\0';
        }
      }
    } else {
      copied_first_char = true;
      memory += str[i];
    }
  }
  if (memory.back()!='\0') memory += '\0'; 
  //from now on, memory should't be touched!
  for ( size_t i = 0; i < memory.length(); ++i ) {
    if (i==0 or memory[i-1]=='\0') arg.push_back(&memory[i]);
  }
}

int main() {
  std::vector<char*> av;
  std::string m;
  str2arg("   this is  a test   string   ", av, m);

  std::cout << "argc=" << av.size() << "  argv=" << av.data() << std::endl;
  for (auto i: av) std::cout << i << ".\n";
  return 0;
}

Basically the function takes the original C-string, a std::vector that will hold the pointers to the words, and a std::string for automated memory management.

In the first part of str2arg() I copy the chars from the original string substituting white spaces with the termination char '\0'. Then I push the char* into the vector looking at the positions of '\0'.

Of course both the vector and the string passed to str2arg() must then be used as read-only variables.

This looks fair, but I'm not totally proud of it...

share|improve this question
    
Could you tell us your motivation for needing a char** rather than, say, a std::vector <std::string>? Are you just trying to split the string into words (delimited by space)? Is there any reason you can't just do something like this? –  jliv902 Aug 29 at 14:45
    
@jliv902 Is very simple: I have other functions that requires the char** and I do not want to put my hands in those. –  DarioP Aug 29 at 15:00
    
Loosing the const in that conversion is a scary. –  Loki Astari Aug 29 at 17:27
    
Just read the code. Loosing that const is devastatingly bad put it back. Put it back now ahhhhhhhhhhhh. –  Loki Astari Aug 29 at 17:29
    
This is also a built in feature of streams. So you can use the standard algorithms and some stream iterator adapters to achieve the same affect in about three lines of code. –  Loki Astari Aug 29 at 17:58

3 Answers 3

I basically reject your code for admittance into the master branch.
This is not C++ code but primarily C and duplicates functionality that exists in the standard library.

Let's look at your interface first:

void str2arg(const char * str, std::vector<char*> & arg, std::string & memory) {

The parameter memory is only used internally to the function. It is not used externally and provides no utility. So get rid of it as a parameter. Make it an internal member of the function.

The parameter arg is used as an out parameter. You don't need to do this. Return the vector by value. This makes it very clear what you are doing. The optimizer will elide the copy and with move semantics in C++11 it will use move if it can not elide the copy. Summary: it is not an issue to return big values from functions the compiler will optimize away any issues (if for some reason it can't and you notice during benchmarking then you can have a look at using out parameters).

std::vector<char*> str2arg(const char * str);

Next issues is you are using pointers (and dropping the constness). Pointers are horrible and should only be used at the lowest level of your code for creating containers. Normally you can use normal objects to represent stuff. Here use std::string.

std::vector<std::string> str2arg(std::string const& str);
                                             ^^^^^^  Allows you to pass a literal.
                                                     That can be converted to a string.

Why are pointers horrible:
They do not provide any semantic definition of ownership. Ownership is a very important concept in C++ as it defines who is responsible for deleting an object. Normally we use containers and smart pointers to define ownership of dynamic objects and we refer to other objects via references when we don't own them.

Const correctness is a big deal in C++ this is something you should take to heart very quickly. You drop the constness on your pointers thus leading to a possibility where people incorrectly use the pointer you return (by modifying the content of the string). This is a real problem if the string is actually const and will cause the program to crash.

Algorithm:

Much improvement can be done here.
Let us examine the utilities provided by the standard.

Note 1:

The stream operator>> when applied to a std::string will read one space separated word from the stream.

  std::string word;
  stream >> word;  // drops leading space and reads a single space delimited word.

Note 2:

The std::istream_iterator<X> are iterators that treat streams as forward containers. They read X from the stream using operator>> and you can then access the object of type X with the standard iterator methods. Use this in combination with std::string and you can read words from a stream.

 std::istream_iterator<std::string> loop(stream);
 std::istream_iterator<std::string> end;

 for(;loop != end;++loop)
 {    std::cout << *loop << ".\n";
 }

Since we now have an iterator to scan through a stream of words we can also apply the standard algorithms.

 // Assuming the loop above had not drained the stream.
 // The following alternative copies a stream into the vector one word at a time.
 std::vector<std::string>  data;
 std::copy(loop, end, std::back_inserter(data));

Also note that std::vector has a constructor that takes iterators so you can build it directly, rather than using an algorithm.

 std::vector<std::string>  data(loop, end);

So now that we have gone through all that we can re-code your function like this:

#include <vector>
#include <string>
#include <sstream>
#include <iterator>
std::vector<std::string> str2arg(std::string const& str)
{
    std::stringstream  stream(str);
    std::vector<std::string>  result((std::istream_iterator<std::string>(stream)),
                                     std::istream_iterator<std::string>());
    return result;
}

Side note.

Since we mention command line args in other answers.
I like to convert the command line args into string before proceeding.

int main(int argc, char* argv[])
{
    std::vector<std::string>  args(argv+1, argv + argc);
}

main

int main()
{
    std::vector<std::string>  data = str2arg("   this is  a test   string   ");
    std::vector<char*>        ptr;
    // Doing it like this because I don't feel inclined to go look
    // up the appropriate iterator adapter to convert a string into a pointer.
    std::for_each(args.begin(), args.end(),
                  [&ptr](std::string& a){ptr.push_back(&a[0]);});

    SomeCFunctionThatNeedsPointers(&ptr[0]);
}

Main with Boost

#include <boost/iterator/transform_iterator.hpp>

int main()
{
    typedef boost::transform_iterator<std::function<char*(std::string&)>,
                                      std::vector<std::string>::iterator
                                     >    ti;

    std::vector<std::string>  data = str2arg("   this is  a test   string   ");
    std::vector<char*>        ptr(ti(args.begin(), [](std::string& x){return &x[0];}), ti(args.end()));

    SomeCFunctionThatNeedsPointers(&ptr[0]);
}
share|improve this answer
2  
This is basically what I suggested in my comment. The OP responded that he has functions that take char ** that he for some reason cannot modify. Also, you cannot make memory an internal variable or else all of the char *'s in arg would point to garbage. This is another reason to avoid using pointers in C++. –  jliv902 Aug 29 at 19:47
    
The question is tagged C++ because I can use C++ features but the target is char** not std::vector<std::string>, the additional conversion is pretty easy. The initial suggestion will just lead to terrible bad segfaults: the purpose of std::string & memory is to own the memory pointed by char**. But it's ok, the interface was counter intuitive and I didn't like that either. –  DarioP Aug 29 at 21:07
1  
@DarioP: Those facts do not change any of my comments. It is trivial to create char** from std::vector<std::string> (it just requires one more declaration. –  Loki Astari Aug 29 at 21:15
    
The iterator adapter that you don't feel inclined to go look up, isn't something that I can find in the stl, right? (You shouldn't have triggered my curiosity with that!) Thanks however, your post touches many things that I didn't know/wouldn't have thought about! –  DarioP Aug 29 at 21:44
    
@DarioP: Probably more likely to find them in boost –  Loki Astari Aug 29 at 23:14
  • The code doesn't look like C++ at all, but rather plain old C. Should the length of the string be known in advance, std::replace_copy() would do most of the job.

  • First part of the function. Boolean flags, along with triple nested ifs point out that you are going a wrong direction.

    Consider instead:

    void copy_skipping_spaces(const char * str, std::string & memory)
    {
        str = skip_spaces(str);
        while(*str) {
            str = copy_non_spaces(str, memory);
            str = skip_spaces(str);
        }
    }
    
  • Single Responsibility Principle (and No Raw Loop rule) mandates factoring first and second parts into separate functions.

share|improve this answer

Changing the function into a class looks much much better and safer!

#include <iostream>
#include <vector>
#include <cctype>

class Str2Arg {
 private:
  std::vector<const char*> _arg;
  std::string _memory;
 public:
  Str2Arg(const char * str) { reset(str); }

  void reset(const char * str) {
    _arg.clear();
    _memory.clear();
    bool copied_first_char = false;
    for ( size_t i = 0; str[i]!='\0'; ++i) {
      if (isspace(str[i])) {
        if (copied_first_char) {
          if (!isspace(str[i+1])) {
            _memory += '\0';
          }
        }
      } else {
        copied_first_char = true;
        _memory += str[i];
      }
    }
    if (_memory.back()!='\0') _memory += '\0'; 
    //from now on, memory should't be touched!
    for ( size_t i = 0; i < _memory.length(); ++i ) {
      if (i==0 or _memory[i-1]=='\0') _arg.push_back(&_memory[i]);
    }  
  }

  int argc() const {return _arg.size();}
  const char* const* argv() const {return _arg.data();}
};

int main() {
  Str2Arg s("   this is  a test   string   ");

  std::cout << "argc=" << s.argc() << "  argv=" << s.argv() << std::endl;
  for (int i = 0; i<s.argc(); ++i) {
    std::cout << s.argv()[i] << '\n';
  }
  return 0;
}
share|improve this answer
    
Don't like your members having a leading ''. The rules about leading '' are complicated enought that most people don't actually know the exact rules. So best avoided. If you must try and identify your members (I think even that is a bad idea as it means your code is hard to read and should be re-factored to be easier to read) then maybe 'm_' would be nicer. –  Loki Astari Aug 29 at 18:54
    
I agree with @LokiAstari on the issue of leading underscores, but just to be clear, it is legal in this particular case. –  jliv902 Aug 29 at 19:37

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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