std::string and contiguous memory?

[laserlight]

It has been guaranteed in the present by the fact that all known implementations of std::string do guarantee these things, and the latest C++ standard that was approved last year guarantee that all future implementations will be compliant as well, so it is overly paranoid to assume otherwise.

I think he's right here. You're not writing code to be that may be compiled with an older standard library implementation that may be unknown to you, so just check that what you have satisfies the assumption (it probably does). If so, all is well: go ahead and use &s[0] even if the standard library implementation was written at a time when the current guarantees did not exist.

[Paul McKenzie]

Reviewer: I'm not claiming that what you did won't work, only that it's unnecessarily convoluted. I am concerned about the impact on maintenance and what junior engineers are learning.

Why does the reviewer think the other programmers are ignorant or can't simply look up this information? What the reviewer is stating sounds like projection -- he/she has a problem with it, so it must mean that others will have a problem with it. Unless you're using convoluted and obfuscated C coding in a C++ program, a reviewer should (or would) never say this, since IMO this is an indictment of the other programmers.

If these programmers don't know that vector is contiguous, then what other aspect of the standard library do they not know? Are you allowed to use vectors as a replacement for new[]/delete[]? How about usage of algorithms? Function objects? Might as well have the reviewer list them, so you don't waste your time writing code that uses these idioms.

Regards,

Paul McKenzie

[monarch_dodra]

I think he's right here. You're not writing code to be that may be compiled with an older standard library implementation that may be unknown to you, so just check that what you have satisfies the assumption (it probably does). If so, all is well: go ahead and use &s[0] even if the standard library implementation was written at a time when the current guarantees did not exist.

Unfortunately, my real world experience kind of makes me feel the same way. Yes, it is wrong, but let's face it, it works, and it costs less.

What I suggest you do though, rather than doing an ugly const_cast, or an undefined &str[0]/&*str.begin(), is to use a non-member "data" function. C++11 defined non-member functions such as begin/end that could be used on either containers or with static arrays. Why not just do the same thing with a data function? Here is what I have in mind:

#include <vector>
#include <iostream>
#include <string>
#include <array>
#include "data.h"

int main() {
        std::vector<int> vec1(10);
  const std::vector<int> vec2(10);
        char* p1 = "blablabla";
  const char* p2 = "blablabla";
        char a1[10] = "blablabla";
  const char a2[10] = "blablabla";
        std::string s1 = "blablabla";
  const std::string s2 = "blablabla";
        std::array<int, 3> ar1 = {1, 2, 3};
  const std::array<int, 3> ar2 = {1, 2, 3};

  std::cout << *data(vec1) << std::endl;
  std::cout << *data(vec2) << std::endl;
  std::cout << *data(p1) << std::endl;
  std::cout << *data(p2) << std::endl;
  std::cout << *data(a1) << std::endl;
  std::cout << *data(a2) << std::endl;
  std::cout << *data(s1) << std::endl;
  std::cout << *data(s2) << std::endl;
  std::cout << *data(ar1) << std::endl;
  std::cout << *data(ar2) << std::endl;
}

With this approach, you have access to a generic/portable "data" method, that works for any compatible container-like object. The advantage is the developper doesn't have to worry since the public interface of "data" is: "I guarantee I work. Don't worry about it".

To implement this, you have two options:

The first one, the easy one:

//Generic container
template<typename T>
typename T::pointer data(T& input)
{
  return const_cast<typename T::pointer>(input.data());
}

//Generic const_container
template<typename T>
typename T::const_pointer data(const T& input)
{
  return input.data();
}

//Generic (const) pointer/array
template<typename T>
T* data(T* input)
{return input;}

By adding that const_cast, it makes sure the call works for either generic containers (vector/array), as well as strings classes. Yay!

I'd recommend this actually doing this though:

//Generic container
template<typename T>
typename T::pointer data(T& input)
{
  return input.data();
}

//Generic const_container
template<typename T>
typename T::const_pointer data(const T& input)
{
  return input.data();
}

//basic_string forward
namespace std{
template<typename C, typename T, typename A>
class basic_string;
}

//data for basic_string
template<typename C, typename T, typename A>
typename std::basic_string<C, T, A>::pointer data(std::basic_string<C, T, A>& input)
{
  return const_cast<typename std::basic_string<C, T, A>::pointer>(input.data());
}

//data for const basic_string
template<typename C, typename T, typename A>
typename std::basic_string<C, T, A>::const_pointer data(const std::basic_string<C, T, A>& input)
{
  return input.data();
}

//Generic (const) pointer/array
template<typename T>
T* data(T* input)
{return input;}

This is a bit more convoluted, but it separates the implementation for string_types. This can be useful if you want to track down all data calls to [|s|s|u13|u32]string's

EDIT: I just remembered you aren't using C++11, so you can replace the container's ".data" with ".empty? null, &[0];"

[laserlight]

With this approach, you have access to a generic/portable "data" method, that works for any compatible container-like object. The advantage is the developper doesn't have to worry since the public interface of "data" is: "I guarantee I work. Don't worry about it".

Nice, but...

EDIT: I just remembered you aren't using C++11, so you can replace the container's ".data" with ".empty? null, &[0];"

There are containers for which &x[0] is valid, but which do not store the elements of x contiguously, e.g., std:eque (which does not have a data() member function in C++11). In such a case, this workaround would not result in a compile error, i.e., this non-member data() would not actually guarantee that it works.

[JohnW@Wessex]

Why does he think using std::vector is liable to be a maintenance problem?
It's hardly a lot of extra code to write.

// Unsafe way
std::string text("Some text");
...
Function(const_cast<char*>(text.c_str()));

// Safe way
// Safe C string generator.
class SafeCString
{
public:

    SafeCString(const std::string& text)
    {
        data.assign(text.c_str(), text.c_str() + text.size() + 1);
    }

    char* operator()()
    {
        return &data[0];
    }

private:

    std::vector<char> data;
};

std::string text("Some text");
...
Function(SafeCString(text)());

[nuzzle]

That’s why I used a vector. My co-worker is very persistent though, so I just needed some proof.

I think you're both wrong. There's no reason to assume anything about the internals of vector or string really. Use them at the level of abstraction they were designed for. Don't break encapsulation in ordinary code just because you can.

So create an ordinary char array, copy the string content to it and pass it to the function? That's the proper C solution to a C problem and it's guaranteed to be correct.

[laserlight]

I think you're both wrong. There's no reason to assume anything about the internals of vector or string really. Use them at the level of abstraction they were designed for. Don't break encapsulation in ordinary code just because you can.

I linked to Herb Sutter's blog post on this earlier, admittedly for a different reason, but the blog post itself was about whether using &v[0] for a std::vector v violates the abstraction: Cringe not: Vectors are guaranteed to be contiguous. He concludes that by design, it doesn't.

EDIT:
Consequently, the same idea applies for the guarantee of contiguous storage of the contents of basic_string in C++11.

So create an ordinary char array, copy the string content to it and pass it to the function? That's the proper C solution to a C problem and it's guaranteed to be correct.

You will then have to do manual memory management. Not the end of the world, but why do that when there's std::vector, or post-C++11 (or with suitable knowledge that it is safe) std::string?

[Philip Nicoletti]

One question though, is data() guaranteed to return a null terminated string, or is that also implementation dependent

Not in the old standard ... in C+11 data() is guaranteed to return a null terminated string.
c_str() is guaranteed in both versions.

[monarch_dodra]

Nice, but...


There are containers for which &x[0] is valid, but which do not store the elements of x contiguously, e.g., std:eque (which does not have a data() member function in C++11). In such a case, this workaround would not result in a compile error, i.e., this non-member data() would not actually guarantee that it works.

Crap

There goes my beautiful design

[monarch_dodra]

[doublepost]

[Codeplug]

>> I would like to say I have no intention on changing the code that I have written based on our conversation, unless he goes to management and tries to force me to see things his way
Be mindful of the "office politics" in play here. You don't want to set an unfavorable perception of yourself in other's eyes.

>> The interface is non-const. There is no guarantee that the interface won't modify the non-const buffer. Therefore there is no argument for using const_cast that could possibly hold water.
There is no debate outside of that. Using const_cast<> in this case is just garbage. Once that fact is accepted, then move on to "how to do it right". If that fact won't be accepted, move on. Not much you can do besides laying down that facts (what the standard says), and the opinions of yourself and other experts in the field like Scott Meyers and Herb Sutter.

gg

[nuzzle]

I linked to Herb Sutter's blog post on this earlier, admittedly for a different reason, but the blog post itself was about whether using &v[0] for a std::vector v violates the abstraction:

Herb Sutter makes it a special point that he's in disagreement with Andy Koenig so I guess I side with the latter.

My point is that you have a choise. You don't have to use every aspect of an interface. Just because it's there doesn't mean you have to use it. It's better to stay away from things that are shaky, questionable or just downright ugly in general.

As of C++ 11 vector and string finally have certain firm guarantees for how memory is handled internally. But to me that's a big yawn because I'm never going to make use of this anyway. Not even if Herb Sutter claims it's a proper part of a vector or string abstraction. I, and possibly Andy Koenig, still don't think it is. Especially not if the only motivation is that it belongs there because it is there. And note that the OP is using pre-version 11 C++ where the situation isn't that clear-cut.

In my view the OP should suggest an adapter class to convert a string into a suitable char array in a way that doesn't make any assumptions on the internals of string or vector at all. That will undisputably work. And this should be presented as a first step towards a more thorough interface to that API library that's used.

That's the constructive thing to do. Getting out of this silly conflict where both sides are wrong and start improving the program together. I'm not against deep discussions about technical detail but sometimes they become all trees that prevent you from seeing the wood.

[monarch_dodra]

Why does he think using std::vector is liable to be a maintenance problem?
It's hardly a lot of extra code to write.

Truth. however, it lacks the "other" class, SafeMutableCString, if the point is to mutate the string:

struct SafeMutableCString
{
public:
    explicit SafeMutableCString(std::string& str) //NON CONST THIS TIME
      : m_str(str)
    {
        data.assign(m_str.c_str(), m_str.c_str() + str.size() + 1);
    }

    operator char*() //I changed this from char* operator()
    {
        return &data[0];
    }

    ~SafeMutableCString() //Destructor writes back to string
    {
      m_str = &data[0];
    }

private:
    std::string& m_str;
    std::vector<char> data;
};

BTW, I think the design is better with an explicit destructor, but implicit cast. The usage becomes more transparent, and I don't think there is any potential for wrong usage (since the constructor is explicit, and the ONLY point in creating the struct is to cast it anyways) :

void foo(char * p)
{
  strcpy(p, "hello world!");
}

int main()
{
  std::string text("Some text containing lots of letters");
  foo(SafeMutableCString(text));
  std::cout << text << std::endl;
}

Again, I think the strength in such a design is not the internals, but the public interface. In the long run, who cares how SafeMutableCString is written, as long as the coder can use it without worry. If your reviewer wants to replace the implementation with const_cast for efficiency reasons, then power to him. You, on the other hand, are using a method with an interface certified safe.

[laserlight]

You don't have to use every aspect of an interface. Just because it's there doesn't mean you have to use it. It's better to stay away from things that are shaky, questionable or just downright ugly in general.

I agree, but with respect to C++11, the &s[0] solution for std::string s is not shaky, questionable or downright ugly (a little ugly?). With respect to C++03, the &v[0] solution for an intermediate std::vector v is not shaky or questionable, but I concede that it is downright ugly compared to your suggestion of an adapter class... but then it could be used to implement the adapter class (like what JohnW@Wessex and monarch_dodra have been posting about recently).

Especially not if the only motivation is that it belongs there because it is there.

Sutter's assertion is that it is there because it belongs there and considering that the lack thereof was considered a defect in C++98, that makes sense to me. If it had been in C++98 rather than amended in C++03, then your argument might hold water.

In my view the OP should suggest an adapter class to convert a string into a suitable char array in a way that doesn't make any assumptions on the internals of string or vector at all. That will undisputably work.

If there is a guarantee, then there is no assumption. It will undisputably work.

Where the assumption comes in would be if sszd goes with the &s[0] solution for a std::string s, with the knowledge that the standard library implementation does not conform to C++11, but happens to use contiguous storage. It may be a practical solution, but it is still an assumption, not a guarantee.

[JohnW@Wessex]

BTW, I think the design is better with an explicit destructor,

I agree. That was a mistake on my part; I normally always make single parameter constructors explicit.