Uploaded image for project: 'IMPALA'
  1. IMPALA
  2. IMPALA-4757

Macros in testutil/gtest-util.h evaluate their arguments twice

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: Impala 2.8.0
    • Fix Version/s: Impala 2.9.0
    • Component/s: Infrastructure
    • Labels:
      None

      Description

      I just discovered that the macros are written such that they evaluate their arguments twice. This can have confusing consequences if the argument is a function with side-effects.

        Activity

        Hide
        tarmstrong Tim Armstrong added a comment -

        IMPALA-4757: addendum: avoid double underscore in name

        Names containing double underscore are implementation-reserved.
        E.g. a global macro called __status could in principle be defined
        in a C++ system header.

        Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05
        Reviewed-on: http://gerrit.cloudera.org:8080/5702
        Reviewed-by: Jim Apple <jbapple-impala@apache.org>
        Tested-by: Impala Public Jenkins

        Show
        tarmstrong Tim Armstrong added a comment - IMPALA-4757 : addendum: avoid double underscore in name Names containing double underscore are implementation-reserved. E.g. a global macro called __status could in principle be defined in a C++ system header. Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05 Reviewed-on: http://gerrit.cloudera.org:8080/5702 Reviewed-by: Jim Apple <jbapple-impala@apache.org> Tested-by: Impala Public Jenkins
        Hide
        tarmstrong Tim Armstrong added a comment -

        IMPALA-4747: macros should only evaluate their arguments once

        The way the macros were previously written, they expanded
        so that the 'status' expression was duplicated. This means
        if the expression has side-effects, they happen twice.
        The fix is to introduce a temporary variable.

        Change-Id: I1229849150d3f747e6780cc072ba063152ade1a9
        Reviewed-on: http://gerrit.cloudera.org:8080/5686
        Reviewed-by: Matthew Jacobs <mj@cloudera.com>
        Reviewed-by: Henry Robinson <henry@cloudera.com>
        Tested-by: Impala Public Jenkins

        M be/src/exec/hash-table-test.cc
        M be/src/runtime/buffered-tuple-stream-test.cc
        M be/src/runtime/tmp-file-mgr-test.cc
        M be/src/testutil/gtest-util.h
        4 files changed, 20 insertions, 6 deletions

        Approvals:
        Impala Public Jenkins: Verified
        Henry Robinson: Looks good to me, approved
        Matthew Jacobs: Looks good to me, but someo

        Show
        tarmstrong Tim Armstrong added a comment - IMPALA-4747 : macros should only evaluate their arguments once The way the macros were previously written, they expanded so that the 'status' expression was duplicated. This means if the expression has side-effects, they happen twice. The fix is to introduce a temporary variable. Change-Id: I1229849150d3f747e6780cc072ba063152ade1a9 Reviewed-on: http://gerrit.cloudera.org:8080/5686 Reviewed-by: Matthew Jacobs <mj@cloudera.com> Reviewed-by: Henry Robinson <henry@cloudera.com> Tested-by: Impala Public Jenkins — M be/src/exec/hash-table-test.cc M be/src/runtime/buffered-tuple-stream-test.cc M be/src/runtime/tmp-file-mgr-test.cc M be/src/testutil/gtest-util.h 4 files changed, 20 insertions , 6 deletions Approvals: Impala Public Jenkins: Verified Henry Robinson: Looks good to me, approved Matthew Jacobs: Looks good to me, but someo

          People

          • Assignee:
            tarmstrong Tim Armstrong
            Reporter:
            tarmstrong Tim Armstrong
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development