10

I like using sentry classes in c++, but I seem to have a mental affliction that results in repeatedly writing bugs like the following:

{
  MySentryClass(arg);
  // ... other code
}

Needless to say, this fails because the sentry dies immediately after creation, rather than at the end of the scope, as intended. Is there some way to prevent MySentryClass from being instantiated as a temporary, so that the above code either fails to compile, or at least aborts with an error message at runtime?

Dan
  • 6,008
  • 7
  • 40
  • 41
SuperElectric
  • 17,548
  • 10
  • 52
  • 69
  • 1
    I don't think the answerers so far understand - you *know* how to do it properly but find yourself making this mistake often, and you want to know if there's an automated way to detect it. Right? – Mark Ransom Jan 03 '11 at 22:31
  • @Mark, @DeadMG: yes, that's correct. – SuperElectric Jan 03 '11 at 22:39

6 Answers6

8

I can't think of an automatic way to detect if you make this mistake or not. You could always create a macro that expands to the correct thing and use that to declare the sentry instead if you keep using it wrong.

#define MY_SENTRY_CLASS(_X) MySentryClass _sentry(_X)

and then use

MY_SENTRY_CLASS(arg);

or put a post-it on your monitor to remind you.

Roger Perkins
  • 678
  • 5
  • 7
  • 1
    I've found that this works even better if you build a macro that also handles the block scoping for you. That way, you can write something like "guarded_block(myBlock) { ... }" and it works automatically. – templatetypedef Jan 03 '11 at 22:43
  • oh, no! not macros, not those stinky things! – Gene Bushuyev Jan 03 '11 at 22:46
  • 4
    Don't use underscore like that. One day the compiler god will slap you. – Martin York Jan 03 '11 at 22:48
  • @Gene: in some cases macros *are* appropriate, this one is a good example – Roman L Jan 03 '11 at 22:53
  • Low tech to the rescue! And you don't usually care what the sentry object is named, so you can eliminate the parameter and always use a fixed name. – Mark Ransom Jan 03 '11 at 22:58
  • I like the macro idea. I have no faith in the power of the post-it, though. More effective would be an emacs plugin that detects the above bug as soon as it happens, and activates my shock-training collar. – SuperElectric Jan 03 '11 at 22:59
  • @7vies, macros are always evil, but in some cases they are necessary, and this is not one of those cases. – Gene Bushuyev Jan 03 '11 at 23:01
  • @Gene: I don't like macros neither, but I don't see a point in claiming something is evil without any reasoning – Roman L Jan 03 '11 at 23:08
  • @Gene You should inform the authors of two of my favorite C++ libraries, google-gflags and google-glog. They give you macros for defining command-line flags, and logging, respectively. Do no evil indeed! (No I don't work there). – SuperElectric Jan 03 '11 at 23:10
  • 1
    +1, and if you add `__COUNTER__` to the macro it will also be possible to have multiple sentries in the same scope. – dalle Jan 04 '11 at 09:45
  • `__COUNTER__`? I know of `__FILE__`, `__LINE__`, `__DATE__`, and `__TIME__`, but I've never heard of that one. Sounds like an extension. – Mike DeSimone Jan 04 '11 at 13:05
  • It's apparently an extension provided by VC++ and GCC: http://stackoverflow.com/questions/652815/has-anyone-ever-had-a-use-for-the-counter-pre-processor-macro – SuperElectric Jan 16 '11 at 22:11
4

The only thing you could do is make the constructors private and force access through a helper function. This is far less similar than the initial construction syntax and less likely to be mistaken. You could also allocate on the heap (still a waste) but it's much easier to spot. However, if you want your class to be constructible, you can't stop people constructing rvalues of that type.

Edit: IF you know that MySentryClass always takes an argument, you could disallow construction AND and only allow operator=(arguments). This would force you to do

MySentryClass x;
x = arg;

You could do some kind of method chain for it.

MySentryClass x;
x.SetArg1(arg).SetArg2(arg2).construct();
Puppy
  • 144,682
  • 38
  • 256
  • 465
  • This isn't helpful, since this whole idea of this is to use scope to get 'ctor called on block entrance, dtor called on exit.' – bmargulies Jan 03 '11 at 22:33
  • But still, this doesn't prevent or prohibit anything, or does it? – dalle Jan 03 '11 at 22:34
  • All it does is force the use of less similar syntax, making it less likely to be mistaken. Fundamentally, the language has no forced stack allocation. – Puppy Jan 03 '11 at 22:36
3

No, there is no exit from this problem. To make objects on the stack, you have to have public constructors, and if you have public constructors, you can make the mistake you are reporting.

bmargulies
  • 97,814
  • 39
  • 186
  • 310
2

Not sure you'll like this solution, but the solution may well be grep:

find /path/to/project -type f -name \*.cpp -print0 | xargs grep -0 'MySentryClass('

Another thing you could do is use sed or perl to preprocess your source file, replacing MySentryClass( with \n#error MySentryClass used incorrectly\n, which hopefully will give you a line number that's close to where the error is. How to do this depends on your build system.

Mike DeSimone
  • 41,631
  • 10
  • 72
  • 96
1

I think the #define is the best method.
But just as an option for not using #define:

Main

int main()
{
    try
    {
        S arg1;
        // This will not compile
        // MySentry    x1   = MySentry::CreateSentry(arg1);

        S arg3;
        MySentry    x2(MySentry::CreateSentry(arg3));


        S arg2;
        // This will not compile
        // MySentry(arg2);

        S arg4;
        // This will generate a runtime exception
        // It will never call start() or end()
        //MySentry::CreateSentry(arg4);
    }
    catch(std::exception const& e)
    {
        std::cout << "Exception : " << e.what() << "\n";
    }
}

Edited. Now works better.

#include <stdexcept>
#include <iostream>

class S
{
    public:
        void start()    {std::cout << "Start\n";}
        void end()      {std::cout << "End\n";}
};

class MySentry
{
        struct Init
        {
            Init(S& s) : arg(s),bad(true) {}
           ~Init()  {if (bad) {throw std::runtime_error("Bad usage of MySentry");}}
            S&              arg;
            mutable bool    bad;
        };
    public:
        static Init CreateSentry(S& arg) { return Init(arg);}

        explicit MySentry(Init const& arg)
            : obj(arg.arg)
            , bad(false)
        {
            arg.bad = false;
            std::cout << "Created\n";
            obj.start();
        }
        MySentry(MySentry const& rhs)
            : obj(rhs.obj)
            , bad(false)
        {
            std::cout << "Copied (this may not appear)\n";
            std::cout << "If the optimizer kicks in then the copy may be elided.\n";

            // But if it did not optimize out then
            // We have to mark the temporaty as bad
            // And not call end() in its destructor.

            // Note: Never call start() here as it will always be called in the
            //       main private constrctor above
            rhs.bad = true;
        }
        ~MySentry()
        {
            if (!bad)
            {
                // Everything working
                obj.end();
            }
            std::cout << "Destroyed\n";
        }
    private:
        S&              obj;
        mutable bool    bad;
};
Martin York
  • 257,169
  • 86
  • 333
  • 562
0

What you are trying to do is perfectly legal in C++ and I don't think there is a way to disallow it.

Sonny Saluja
  • 7,193
  • 2
  • 25
  • 39