Combining auto_ptr with references in C++

Question

Some time ago, I decided strictly following the rule to check each pointer before dereferencing it the first time in a scope, I also changed pointers to references where appropriate: in some cases statically in the code base, in some cases dynamically (after asserting the pointer isn't null of course). This finally led to code like this:

std::string LoadText0(const std::string& fileName)
{
    TStrings& lines = *new TStringList;
    lines.LoadFromFile(fileName.c_str());
    std::string result = lines.Text.c_str();
    delete &lines;
    return result;
}

...which I don't really "like", but it's consistent with the above rule, because I don't care failing news at all: I assure it to throw an exception based on compiler settings.

But: this code is far from being exception-safe. Since the static analysis tool Cppcheck has an issue in the current version 1.65, stating that I try to delete an auto-variable which causes undefined behaviour, I found the lack of exception-safety worth rewriting the code, now I'm using std::auto_ptr.

But now, I dislike all the -> operators: they give the false feeling that the pointers are optionally assigned. This look is especially a problem when the scope gets larger, and it's also a lot of work to rewrite all dots to arrows.

std::string LoadText1(const std::string& fileName)
{
    std::auto_ptr<TStrings> lines(new TStringList);
    lines->LoadFromFile(fileName.c_str());
    std::string result = lines->Text.c_str();
    return result;
}

So I tried to figure out a solution that combines the best from the two worlds and this is what I found (with the downside of two "access points" to the same object)

std::string LoadText2(const std::string& fileName)
{
    std::auto_ptr<TStrings> plines(new TStringList);
    TStrings& lines = *plines;
    lines.LoadFromFile(fileName.c_str());
    std::string result = lines.Text.c_str();
    return result;
}

Is this overkill in your eyes? Would you consider it to be idio(ma)tic?

Answer 1

Well, this might just be opinion, but I will try to justify the opinion.

I consider the use of the -> in your second example to be more idiomatic. It is immediately clear to a skilled c++ reader what you are doing.

By turning the pointer into a reference in snippet three you are inserting (imho) a useless line of code whose only benefit is to soothe anyone who is disturbed by the use of the -> operator.

If the pointer is not assigned, you will get the same error when you attempt to turn into a reference as you will when you try to use it. So it accomplished nothing. So in my opinion the code neither accomplishes anything, nor clarifies the code -- this makes is noise, not signal, and in a code review I would suggest removing it.

Given your constraint that you have to allocate on the heap, and only have auto_ptr available, I find your second solution to be the correct one. To assure the reader of the code that the the pointer is assumed to be assigned, I recommend to follow guideline 68 from "C++ Coding Standards" (Sutter & Alexandrescu -> and excellent read), which states "Assert liberally to document internal assumptions and invariants.".

Thus immediately after initializing your (auto) pointer, include the line:

assert(lines);

This acts as documentation to anyone reading your code that you are reasonably certain no uninitialized pointers will be reaching that point of the code, and it serves as a debug (not release) time test that your assumptions are valid. Win win.