Yes, that addresses problem one that I mentioned; that this thing is a global variable. If I had my druthers, it would be a member of the Parser object instead. But I'll leave that for another day.
-----Original Message----- From: Qifan Chen [mailto:[email protected]] Sent: Thursday, May 19, 2016 8:36 AM To: dev <[email protected]> Subject: Re: Set_SqlParser_Flags 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 < [email protected]> 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:[email protected]] > Sent: Thursday, May 19, 2016 8:08 AM > To: dev <[email protected]> > 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 > <[email protected]> > 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:[email protected]] > > Sent: Wednesday, May 18, 2016 2:36 PM > > To: [email protected] > > 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:[email protected]] > > Sent: Wednesday, May 18, 2016 2:02 PM > > To: [email protected] > > 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
