trafodion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sandhya Sundaresan <sandhya.sundare...@esgyn.com>
Subject RE: Set_SqlParser_Flags
Date Thu, 19 May 2016 15:13:59 GMT
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
>

Mime
View raw message