Is this dangerous or bad practice?

This is a discussion on Is this dangerous or bad practice? within the C++ Programming forums, part of the General Programming Boards category; Code: Level* LoadLevel(const string& fileName) { Level& level = *(new Level(GetFileModTime(fileName))); .... return &level; } instead of Code: Level* LoadLevel(const ...

  1. #1
    Registered User
    Join Date
    Apr 2007
    Location
    Sydney, Australia
    Posts
    217

    Is this dangerous or bad practice?

    Code:
    Level* LoadLevel(const string& fileName)
    {
      Level& level = *(new Level(GetFileModTime(fileName)));
      ....
      return &level;
    }
    instead of

    Code:
    Level* LoadLevel(const string& fileName)
    {
      Level* level = new Level(GetFileModTime(fileName));
      ....
      return level;
    }
    I just need to use operators on the level object, which is more annoying when its a pointer.

  2. #2
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    I don't see any point in doing the first variant - as to if it's dangerous or not - can't say for sure - I think not.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  3. #3
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,307
    That looks to me like simply an unusual combination of a number of not-so-unusual things. I would say it is absolutely not dangerous, but whether it is bad practice it probably a matter of opinion.
    I think it's probably okay.

    Here's food for thought:
    Code:
    template <typename T> T& new_ref()
    {
        return *(new T());
    }
    
    template <typename T, typename P1> T& new_ref(P1 p1)
    {
        return *(new T(p1));
    }
    
    template <typename T, typename P1, typename P2> T& new_ref(P1 p1, P2 p2)
    {
        return *(new T(p1, p2));
    }
    
    ... etc
    Of course this makes deletion a little unintuitive and easily forgotten, so I wouldn't suggest actually using code like the above.
    Last edited by iMalc; 07-18-2008 at 03:31 AM.
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  4. #4
    The larch
    Join Date
    May 2006
    Posts
    3,573
    You might also make a separate reference:
    Code:
    Level* LoadLevel(const string& fileName)
    {
      Level* level = new Level(GetFileModTime(fileName));
      Level& level_ref = *level;
      ....
      return level;
    }
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  5. #5
    Registered User
    Join Date
    Apr 2008
    Posts
    890
    You return a Level* in both cases. The first one won't compile, will it?
    Last edited by medievalelks; 07-18-2008 at 06:00 AM.

  6. #6
    Registered User
    Join Date
    Apr 2007
    Location
    Sydney, Australia
    Posts
    217
    ^ It compiles fine and it should. I just wanted to know if there was some hidden danger doing it like this. The function LoadLevel is alot bigger than what i posted, and it uses operators like [] and <<. I didnt want to have to convert it to a reference each time i need to call an operator (like (*level)[4] = blah)

  7. #7
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by 39ster View Post
    ^ It compiles fine and it should. I just wanted to know if there was some hidden danger doing it like this. The function LoadLevel is alot bigger than what i posted, and it uses operators like [] and <<. I didnt want to have to convert it to a reference each time i need to call an operator (like (*level)[4] = blah)
    So, use anon's method. Also, you may find that you want to actually make it into two functions, where you allocate the pointer, and then pass the pointer by reference to a "fill in" function.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  8. #8
    Deathray Engineer MacGyver's Avatar
    Join Date
    Mar 2007
    Posts
    3,211
    It might be best to return a pointer to dynamically allocated memory. It makes it a little more intuitive that the caller needs to deallocate the memory. If they receive a reference, I'm not sure if it's so obvious.

  9. #9
    Registered User
    Join Date
    May 2008
    Location
    Paris
    Posts
    248
    But, if you return a pointer to an object which has been created by 'new' in a function, that pointer won't point to the right object? I remember this problem I had before, I had to solve it with " ::new " (using anon's method):
    Code:
    Level* LoadLevel(const string& fileName)
    {
      Level* level = ::new Level(GetFileModTime(fileName));
      Level& level_ref = *level;
      ....
      return level;
    }
    ref: http://blogs.msdn.com/jaredpar/archi...ement-new.aspx
    Last edited by MarkZWEERS; 07-18-2008 at 08:09 AM. Reason: reference

  10. #10
    Deathray Engineer MacGyver's Avatar
    Join Date
    Mar 2007
    Posts
    3,211
    I think you're completely wrong on this, unless I'm misunderstanding you. That article has to do with placement new.

  11. #11
    Registered User
    Join Date
    May 2008
    Location
    Paris
    Posts
    248
    hmm.. ok, sorry for that. The article is the wrong one, can't find the right one anymore!

  12. #12
    Registered User
    Join Date
    Apr 2008
    Posts
    890
    Quote Originally Posted by 39ster View Post
    ^ It compiles fine and it should.
    Sorry - I completely missed that you were returning the address of level in the first one.

  13. #13
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,893
    Both variants are exception-unsafe and do not indicate clearly that the return value must be freed by the user. In addition, the second is unusual and thus a detriment to readability.

    You should:
    1) Capture the pointer in a smart pointer immediately.
    2) If you want reference access, create a separate reference.
    3) Return the smart pointer.
    E.g.
    Code:
    std::auto_ptr<Level> LoadLevel(const string& fileName)
    {
      std::auto_ptr<Level> plevel(new Level(GetFileModTime(fileName)));
      Level &level = *plevel;
      ....
      return plevel;
    }
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  14. #14
    Registered User
    Join Date
    Jun 2008
    Posts
    106
    IMalc, thanks for the site in your sig!

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Bad practice or acceptable?
    By Noose in forum C++ Programming
    Replies: 6
    Last Post: 06-09-2004, 01:43 AM
  2. Replies: 11
    Last Post: 11-13-2002, 12:29 PM
  3. Bad Practice??
    By Unregistered in forum C Programming
    Replies: 2
    Last Post: 11-25-2001, 07:37 AM
  4. good news and bad news
    By Garfield in forum A Brief History of Cprogramming.com
    Replies: 25
    Last Post: 10-27-2001, 07:31 AM
  5. Bad code or bad compiler?
    By musayume in forum C Programming
    Replies: 3
    Last Post: 10-22-2001, 09:08 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21