Uploaded image for project: 'Core Server'
  1. Core Server
  2. SERVER-43909

util/hex.h is non-portable, inefficient, arbitrary, untested, nonorthogonal, and redundant

    • Type: Icon: Bug Bug
    • Resolution: Fixed
    • Priority: Icon: Minor - P4 Minor - P4
    • 4.8.0
    • Affects Version/s: None
    • Component/s: None
    • Labels:
      None
    • Fully Compatible
    • ALL
    • Service arch 2020-09-21

      Numerous issues here.

      Need a decomposed library, probably just a set of fundamental templates and some sugar to bind them together into a reasonable user-level offering. Currently we can only parse to std::string and StatusWith<...>, which are too high-level and can't be fast.

      False advertising: header is missing definition for unconstrained template integerToHex, which is defined in the .cpp file for only a few types. `template <typename T> std::string integerToHex<T>(T);` Calling with an unsupported integer type (like unsigned char or unsigned short) is a link-time error, which is user-unfriendly and undocumented. A header should declare what it provides and only what it provides. After doing some refactoring, I don't see a reason to have a .cpp file here at all.

      integerToHex formats ALL the bits of the input value and then truncates the leading zeros. This is wasteful.

      integerToHex performs right-shifts on potentially negative integers. This is implementation-defined behavior and not portable.

      Arbitrary: A few of the functions here are doing the same basic thing but with slightly different behavior. For example, hexdump and toHexLower both take a ptr+len and hexdump the memory, but hexdump uses uppercase and spaces between the hexpairs, while toHexLower uses lowercase and no spaces. hexdump is verify()-limited to 1M bytes, but toHexLower is unlimited.

      isValidHex insists on even digit counts. This is user specific. An unpadded hexdump of an integer value can easily have an odd number of digits (e.g. "0"). Function is unnecessary and called only twice.

      Inefficient: uses various mechanisms internally, inconsistently, of various levels of inefficiency. These are local char[] + snprintf, StringBuilder, etc.

      There are no unit tests for some of the functions here.
      I introduced some errors and didn't see any failures in util_test

            Assignee:
            billy.donahue@mongodb.com Billy Donahue
            Reporter:
            billy.donahue@mongodb.com Billy Donahue
            Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

              Created:
              Updated:
              Resolved: