Re: commit (HEAD): xap_Strings.h/cpp

From: Dom Lachowicz (domlachowicz_at_yahoo.com)
Date: Fri Apr 09 2004 - 12:34:16 EDT

  • Next message: Christian Neumair: "Re: Preferences dialog: Not implementing options without hacks"

    --- Tomas Frydrych <tomasfrydrych_at_yahoo.co.uk> wrote:
    > Possibly, but not necessarily. In
    > ap_Menu_Functions.cpp, line 64, we
    > have this code:
    >
    > const char * c =
    > pss->getValueUTF8(AP_STRING_ID_...).utf8_str();
    >
    > Here the UT_UTF8String temporary variable gets
    > destroyed immediately
    > as this code is executed, so c is never valid (this
    > might be MSVC
    > specific, but is not unreasonable). A solution in
    > _this_ code would
    > be to have instead
    >
    > UT_UTF8String s =
    > pss->getValueUTF8(AP_STRING_ID_...);
    > c = s.utf8_str();
    >
    > This will fix this problem but in my view it is a
    > much poorer
    > solution than having static variable in
    > getValueUTF8(). As long as
    > getValueUTF8() returns a temporary variable, this
    > will happen again
    > and again -- the problem is not immediately obvious,
    > and because the
    > memory that was used by the temporary variable might
    > not be
    > immediately reused, the pointer might sometimes
    > appear to work.
    >
    > Returning a temporary variable from any function is
    > just asking for
    > trouble, creating bugs that are hard to reproduce
    > and therefore fix;
    > it is really the code in xap_Strings that is broken.
    > In the interest
    > of stability, functions should return pointers or
    > references to non-
    > local variables, not temporary objects.

    Again, the solution is to not use the .c_str() or
    .utf8_str() functions in these cases. Or if you do,
    you must really store the returned value.

    We are not returning a true temporary value from the
    function. The function returns an instance of a class,
    complete with copy constructors and assignment
    operators. It does not return a 'char *', and it is
    simply wrong to use it as such. We simply must store
    the returned value for the current 'char *'s scope.
    Not too hard...

    Do not make bad code worse by introducing static
    variables. Doing so is just begging for race
    conditions and other hard-to-reproduce, but
    pain-in-the-ass to debug situations. Much worse than
    anything you're claiming to fix here. Furhter, never
    let bad usage models push bad design upstream.

    Problems like these can be found easily enough using
    Valgrind, Purify, grep, or 5 seconds staring at the
    code. I suggest that you use them and properly fix the
    bug rather than grafting on a hack to cover up an
    outright abuse and misuse of the C++ language.

    Dom

    __________________________________
    Do you Yahoo!?
    Yahoo! Small Business $15K Web Design Giveaway
    http://promotions.yahoo.com/design_giveaway/



    This archive was generated by hypermail 2.1.4 : Fri Apr 09 2004 - 12:36:37 EDT