Derby
  1. Derby
  2. DERBY-5011

[patch] Client driver lexer to determine statement type: fix bad attempt at incrementing a variable

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: 10.4.2.0, 10.5.1.1, 10.5.2.0, 10.5.3.0, 10.6.1.0, 10.6.2.1, 10.7.1.1
    • Fix Version/s: 10.8.1.2
    • Component/s: Network Client
    • Labels:
      None
    • Urgency:
      Normal
    • Bug behavior facts:
      Deviation from standard

      Description

      In the client driver lexer to determine statement type, we look for token "select", "update", "values" etc. The lexer used to has a bug in its
      handling of end-of-line comments "--".

      code does

      idx = idx++;

      but this does absolutely nothing

      1. fix_bad_increment_dhw.diff
        0.8 kB
        Dag H. Wanvik
      2. fix_bad_increment.diff
        0.6 kB
        Dave Brosius

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Dave. The fix looks correct to me. I don't think this bug will ever cause any problems seen by the users. The only thing that happens is that the state machine considers the second '' in '-' as part of the comment and ignores it, which is fine. But it's still good to have it fixed.

          Another issue with this code is that it sets tokenFound = "/" if there are no more characters after the first ''. I think this is a copy/paste error from the case for "/*", and it should have set tokenFound to "" instead. But again, this doesn't really cause any problems, since the only tokens we care about when calling this method are "select", "values", "insert", "update", "delete" and "call".

          Dag, you probably know this code better. Does the above analysis sound about right?

          Show
          Knut Anders Hatlen added a comment - Thanks, Dave. The fix looks correct to me. I don't think this bug will ever cause any problems seen by the users. The only thing that happens is that the state machine considers the second ' ' in ' -' as part of the comment and ignores it, which is fine. But it's still good to have it fixed. Another issue with this code is that it sets tokenFound = "/" if there are no more characters after the first ' '. I think this is a copy/paste error from the case for "/*", and it should have set tokenFound to " " instead. But again, this doesn't really cause any problems, since the only tokens we care about when calling this method are "select", "values", "insert", "update", "delete" and "call". Dag, you probably know this code better. Does the above analysis sound about right?
          Hide
          Dag H. Wanvik added a comment -

          Yes, in existing code, the second '' in the '-' give rise to an
          extra '-' token, which is ignored with the rest of the comment, so
          no harm is done. Also, a single '-' (erroneously translated to '/') will
          not cause harm in this situation, but nice to get the code right.
          Copy/paste error, in deed.

          Show
          Dag H. Wanvik added a comment - Yes, in existing code, the second ' ' in the ' -' give rise to an extra '-' token, which is ignored with the rest of the comment, so no harm is done. Also, a single '-' (erroneously translated to '/') will not cause harm in this situation, but nice to get the code right. Copy/paste error, in deed.
          Hide
          Dag H. Wanvik added a comment -

          Uploading a revision of the fix to handle Knut's observation as well.

          Show
          Dag H. Wanvik added a comment - Uploading a revision of the fix to handle Knut's observation as well.
          Hide
          Dag H. Wanvik added a comment -

          Regressions ran ok, committed as svn 1068702., resolving. Please close, Dave, if you are OK with the fix.

          Show
          Dag H. Wanvik added a comment - Regressions ran ok, committed as svn 1068702., resolving. Please close, Dave, if you are OK with the fix.
          Hide
          Dave Brosius added a comment -

          +1

          Show
          Dave Brosius added a comment - +1

            People

            • Assignee:
              Dave Brosius
              Reporter:
              Dave Brosius
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 1h
                1h
                Remaining:
                Remaining Estimate - 1h
                1h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development