+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 >
