trafodion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qifan Chen <qifan.c...@esgyn.com>
Subject Re: Set_SqlParser_Flags
Date Thu, 19 May 2016 15:36:11 GMT
We may also consider the use of constructor/destructor to hold the current
flag bits, set new bits and restore the old bits, to make the process even
more robust and easy to program.

class parser_flags {

protected:
     ULng32 flagbits_;

public:
    parser_flags (ULng32 flagbits) { TurnOn_SqlParser_Flags(flagbits) /*
store the old bits in flagbits_ */  };
   ~parser_flags () { TurnOff_SqlParser_Flags(flagbits_); };
};

Usage:

 Class1::method1()
{
   {
      parser_flags bits(newBitsToUse);

      ... do some stuff with the parser ...
   }

}

--Qifan

On Thu, May 19, 2016 at 10:13 AM, Sandhya Sundaresan <
sandhya.sundaresan@esgyn.com> wrote:

> Agree with this proposal. If not, it may not be a very easy task for one
> person and also may introduce new bugs/behavior which may be very hard to
> track down if all usages are replaced.
> And to encourage the usage of new methods,  we need to document  very
> clearly that the current  methods are obsolete and not to use them anymore
> and that old occurrences will need to be replaced gradually.
>
> Sandhya
>
> -----Original Message-----
> From: Hans Zeller [mailto:hans.zeller@esgyn.com]
> Sent: Thursday, May 19, 2016 8:08 AM
> To: dev <dev@trafodion.incubator.apache.org>
> Subject: Re: Set_SqlParser_Flags
>
> +1 it would be good to move (more or less gradually) to the new methods
> that are easier to understand.
>
> Hans
>
> On Wed, May 18, 2016 at 3:55 PM, Dave Birdsall <dave.birdsall@esgyn.com>
> wrote:
>
> > Hi,
> >
> > I'm concerned that these bugs propagate to some extent by code
> > cloning. For that, documentation doesn't help, and a review of all
> > calls is still necessary to make sure there are no bugs to clone. The
> > example I debugged today had a copy with exactly the same variable
> > name and comment so I assume it was cloned.
> >
> > By the way, the earlier example I debugged some months ago is here:
> > https://issues.apache.org/jira/browse/TRAFODION-1678.
> >
> > Perhaps I can make a slightly different proposal?
> >
> > Perhaps we provide replacements for Set_SqlParser_Flags and
> > Reset_SqlParser_Flags that have cleaner semantics. But leave the old
> > ones there. And we can encourage folks to change their usage over time.
> >
> > What I have in mind is something like:
> >
> > inline static ULng32 TurnOn_SqlParser_Flags(ULng32 flagbits) {
> >    ULng32 newlyOn = flagbits & ~SqlParser_Flags; // remember the bits
> > that weren't already turned on
> >    SqlParser_Flags |= flagbits;
> >    return newlyOn;
> > }
> >
> > inline static void TurnOff_SqlParser_Flags(ULng32 flagbits) {
> >    SqlParser_Flags &= ~flagbits;
> > }
> >
> > The usage model is like this:
> >
> > Class1::method1()
> > {
> >     ULng32 turnOffOnExit = TurnOn_SqlParser_Flags(<whatever bits I
> > want to turn on>);
> >
> >     ... do some stuff ...
> >
> >     TurnOff_SqlParser_Flags(turnOffOnExit);
> > }
> >
> > This is in comparison to current examples:
> >
> > Class1::method1()
> > {
> >   ULng32 savedParserFlags = Get_SqlParser_Flags(0xFFFFFFFF);  // have
> > to remember to use 0xFFFFFFFF here
> >
> >    Set_SqlParser_Flags(<whatever bits I want to turn on but it needs
> > to be non-zero to work right>);
> >
> >    … do a bunch of stuff …
> >
> >    // Restore parser flags to prior settings.
> >
> >    Assign_SqlParser_Flags(savedParserFlags);  // note Assign, not Set
> > }
> >
> > Or, the following, which is incorrect since it doesn't take into
> > account that some of the bits might have already been on when the
> > method is
> > entered:
> >
> > Class1::method1()
> > {
> >     Set_SqlParser_Flags(<whatever bits I want to turn on but it needs
> > to be non-zero to work right>);
> >
> >     ... do some stuff ...
> >
> >     Reset_SqlParserFlags(<whatever bits I turned on above>); }
> >
> > What do you think?
> >
> > Dave
> >
> > -----Original Message-----
> > From: Anoop Sharma [mailto:anoop.sharma@esgyn.com]
> > Sent: Wednesday, May 18, 2016 2:36 PM
> > To: dev@trafodion.incubator.apache.org
> > Subject: RE: Set_SqlParser_Flags
> >
> > Set and Reset parserflags have been in use for a long time, probably
> > for multiple years.
> >
> > Initially, use of parserflags was very limited and restricted to
> > parser component only. And the current Set and Reset functionality was
> > correct for that usage.
> >
> > But over time, parserflags started to get used in many other
> > components and code.
> >
> > Some of the places did use Set incorrectly as has been mentioned
> > Dave's email.
> > The method AssignParserFlags method was added later to correctly save
> > and restore parserflags, if that is what was needed.
> >
> > We need to be bit careful if all of traf code is to be fixed for
> > Set/Reset usage. One would need to make sure that they understand it
> > well in terms of its impact on the immediate code as well as surrounding
> > code.
> > It may be good to do that though at the same time I don’t think the
> > code is broken because of incorrect usage of parserflags.
> >
> > It maybe good enough to document the Set/Reset/Assign functionality so
> > folks who are adding new code do the right thing.
> >
> > anoop
> >
> > -----Original Message-----
> > From: Dave Birdsall [mailto:dave.birdsall@esgyn.com]
> > Sent: Wednesday, May 18, 2016 2:02 PM
> > To: dev@trafodion.incubator.apache.org
> > Subject: Set_SqlParser_Flags
> >
> > Hi,
> >
> >
> >
> > In parser/SqlParserGlobalsCmn.h are three functions:
> >
> >
> >
> >   inline static void Set_SqlParser_Flags(ULng32 flagbits)
> >
> >   {
> >
> >     if (flagbits)
> >
> >       SqlParser_Flags |= flagbits;
> >
> >     else
> >
> >       SqlParser_Flags = 0;
> >
> >   }
> >
> >
> >
> >   inline static void Assign_SqlParser_Flags(ULng32 flagbits)
> >
> >   {
> >
> >     SqlParser_Flags = flagbits;
> >
> >   }
> >
> >
> >
> > inline static void Reset_SqlParser_Flags(ULng32 flagbits)
> >
> >   {
> >
> >     if (flagbits)
> >
> >       SqlParser_Flags &= ~flagbits;
> >
> >     else
> >
> >       SqlParser_Flags = 0;
> >
> >   }
> >
> >
> >
> > These functions have some error-prone semantics. First problem is that
> > they modify a global variable, but I’ll leave that discussion for another
> > day.
> > The second problem is some odd semantics. Set_SqlParser_Flags turns on
> > bits, unless you pass it a zero value. Then it turns everything off.
> > Reset_SqlParser_Flags turns off some bits, unless you pass it a zero
> > value.
> > Then it turns off EVERY bit. The third problem is that the name
> > “Set_SqlParser_Flags” is misleading. I have debugged more than one set
> > of code where the author appears to have thought that this function
> > set ALL the bits to his particular input value.
> >
> >
> >
> > Here’s an example of a bug that I debugged today:
> >
> >
> >
> > Class1::method1()
> >
> > {
> >
> >   ULng32 savedParserFlags = Get_SqlParser_Flags(0xFFFFFFFF);
> >
> >    Set_SqlParser_Flags(0x100000); // ORs this bit into the flags
> >
> >
> >
> >    … do a bunch of stuff …
> >
> >
> >
> >    // Restore parser flags to prior settings.
> >
> >    Set_SqlParser_Flags(savedParserFlags);
> >
> > }
> >
> >
> >
> > Here’s what this code does. If the parser flags happen to be zero on
> > entry, the savedParserFlags variable gets a value of zero. The first
> > Set_SqlParser_Flags call turns on bit 0x100000. The last
> > Set_SqlParser_Flags call resets the parser flags to zero *because of
> > the weird semantic concerning a zero value.*
> >
> >
> >
> > If the parser flags happen to be non-zero on entry, the
> > savedParserFlags variable gets a non-zero value. The first
> > Set_SqlParser_Flags call turns on the 0x100000 bit. The last
> > Set_SqlParser_Flags call *does nothing!* Why?
> > Because we pass a non-zero value, it just turns on those bits. But
> > those bits were already on! Notice that the 0x100000 bit remains on at
> > exit, in contrast to the first case above where it is reset to zero.
> >
> >
> >
> > It’s clear from the comment that the author intended to restore the
> > parser flags to their previous state. And most of the time, the parser
> > flags are zero, so the code works as intended. But if there is another
> > bug somewhere else that leaves a bit set, then this code will leave
> > another bit set unintentionally, and we snowball.
> >
> >
> >
> > The bug is, the author should have used Assign_SqlParser_Flags for the
> > last call, instead of Set_SqlParser_Flags.
> >
> >
> >
> > Debugging mis-set global variables is hard in general, and this is the
> > second instance of this kind of bug that I’ve run into.
> >
> >
> >
> > I think the misleading function names confuse developers as to the
> > right call to use. And the weird zero semantics tends to hide the bugs
> > so they occur only in weird situations.
> >
> >
> >
> > So I would like to propose the following refactoring:
> >
> >
> >
> > 1.       Rename the functions. Set_SqlParser_Flags à
> > TurnOn_SqlParser_FlagBits, and similarly for Reset. Leave
> > Assign_SqlParser_Flags as-is.
> >
> > 2.       Remove the weird zero semantics. In TurnOn_SqlParser_FlagBits,
> if
> > a zero value is passed in, it does nothing. This is more
> > mathematically consistent; it is the behavior of the Boolean OR
> > operator, and the Set UNION operator. Similarly, in
> > TurnOff_SqlParser_FlagBits, if a zero value is passed in, it does
> > nothing. That’s more like a Set DIFFERENCE.
> >
> > 3.       Go through the Trafodion code, looking for places that call
> > Set_SqlParser_Flags that seem to intend to assign rather than OR, and
> > replace those calls with Assign_SqlParser_Flags.
> >
> >
> >
> > If the community does not object, I will create a JIRA for this work
> > and assign it to myself.
> >
> >
> >
> > In the meantime, please be careful when coding Set_SqlParser_Flags calls.
> >
> >
> >
> > Thanks,
> >
> >
> >
> > Dave
> >
>



-- 
Regards, --Qifan

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message