2

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?

Wolf
  • 9,679
  • 7
  • 62
  • 108
  • 1
    To me the last solution looks fine, but I don't understand why you want `TStringList plines` on heap. Most of the time, you keep variables on heap and stay happy. – Mohit Jain May 15 '14 at 11:58
  • _"But now, I dislike all the -> operators: they give the false feeling that the pointers are optionally assigned"_ that rings alarm bells for me. I feel it is bad practise to try and disguide what a variable is. – Rook May 15 '14 at 11:59
  • `std::auto_ptr` is deprecated, use `std::unique_ptr` instead. Also, I think this might belong on codereview. – Eric Finn May 15 '14 at 12:02
  • you can always do #define . -> – sp2danny May 15 '14 at 12:10
  • @MohitJain I have to use a lot of utility classes of a framework (VCL) that restricts to objects on heap. – Wolf May 15 '14 at 12:11
  • I have the impression you are posting examples to illustrate something, but I have difficulty understanding what you are trying to accomplish. In the first example, "lines" should be instantiated on the stack, not on the heap with a pointer. If you really need heap allocation (because of limitations of the stack for e.g.), that should be encapsulated in the TStrings class. Oops apologies -- I just saw your response to mohit jain. – Spacemoose May 15 '14 at 12:12
  • @EricFinn my framework has only auto_ptr available – Wolf May 15 '14 at 12:13
  • @Spacemoose I always prefer stack-based variables, but here it's forbidden by the framework I use. – Wolf May 15 '14 at 12:14
  • @Rook do you really handle failing news? I'm dealing with objects that **exist** so using references seem to be right. – Wolf May 15 '14 at 12:17
  • 1
    @wolf you are disguising a heap-allocated object because you don't like the way the operators look. Your future maintainance programmers may not thank you for this – Rook May 15 '14 at 12:22
  • @MohitJain sorry I read your comment not carefully enough: now I'm a little confused: what are you trying to say? – Wolf May 15 '14 at 12:23
  • @Rock well, future maintainability may be an important point, thst's why I ask. Thanks for making that even clearer. – Wolf May 15 '14 at 12:30
  • Sorry for the typo. Please read later `heap` as `stack`. The problem why you are suffering is because it not possible to overload `.` operator. On possible way would be [overload some other operator](http://ideone.com/reAHmB). Although I won't suggest this. Spacemoose has given a quite convincing answer. – Mohit Jain May 15 '14 at 12:31
  • @MohitJain seems that my first answer matched ;) – Wolf May 15 '14 at 12:34

1 Answers1

4

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.

Spacemoose
  • 3,856
  • 1
  • 27
  • 48
  • +1 Your argument is quite convincing, I've to confess I'm relatively new to `auto_ptr` - in a few days the picture might be much clearer to me. – Wolf May 15 '14 at 12:27
  • I accept your answer so far. Your `assert(lines);` is irresistible :) – Wolf May 15 '14 at 14:40
  • Thanks, I'm glad you like it. That tip, among several others in the Sutter and Alexandrescu book had a big impact on my coding style. – Spacemoose May 15 '14 at 15:07