[PATCH] D59262: [scudo][standalone] Add string utility functions

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: lib/scudo/standalone/string_utils.cc:33
+  while (true) {
+    const unsigned C1 = *S1;
+    const unsigned C2 = *S2;
----------------
Why `unsigned`? Shouldn't this just be `char`?


================
Comment at: lib/scudo/standalone/string_utils.cc:56
+
+s64 convertStringToS64(const char *NPtr, const char **EndPtr, int Base) {
+  CHECK_EQ(Base, 10);
----------------
Personally, input params before output params, `EndPtr` should be after `Base`.


================
Comment at: lib/scudo/standalone/string_utils.cc:63
+  bool HaveDigits = false;
+  char *OldNPtr = const_cast<char *>(NPtr);
+  if (*NPtr == '+') {
----------------
`const char *OldNPtr = NPtr`


================
Comment at: lib/scudo/standalone/string_utils.cc:65
+  if (*NPtr == '+') {
+    Sign = 1;
+    NPtr++;
----------------
`Sign` is initialized to 1 in declaration, can remove this.


================
Comment at: lib/scudo/standalone/string_utils.cc:73
+    Res = (Res <= UINT64_MAX / 10) ? Res * 10 : UINT64_MAX;
+    const uptr Digit = ((*NPtr) - '0');
+    Res = (Res <= UINT64_MAX - Digit) ? Res + Digit : UINT64_MAX;
----------------
Why `uptr`? `char` or `int` should be sufficient. Also `const` not needed here.


================
Comment at: lib/scudo/standalone/string_utils.cc:78
+  }
+  if (EndPtr) {
+    *EndPtr = (HaveDigits) ? const_cast<char *>(NPtr) : OldNPtr;
----------------
Can remove brackets


================
Comment at: lib/scudo/standalone/string_utils.cc:79
+  if (EndPtr) {
+    *EndPtr = (HaveDigits) ? const_cast<char *>(NPtr) : OldNPtr;
+  }
----------------
*EndPtr = (HaveDigits) ? NPtr : OldNPtr;


================
Comment at: lib/scudo/standalone/string_utils.cc:81
+  }
+  if (Sign > 0) {
+    return static_cast<s64>(Min(static_cast<u64>(INT64_MAX), Res));
----------------
Can remove brackets. Same with the `else` of this clause.


================
Comment at: lib/scudo/standalone/string_utils.cc:82
+  if (Sign > 0) {
+    return static_cast<s64>(Min(static_cast<u64>(INT64_MAX), Res));
+  } else {
----------------
Is `return Min(static_cast<u64>(INT64_MAX), Res);` sufficient here?


================
Comment at: lib/scudo/standalone/string_utils.cc:96
+
+// appends number in a given Base to buffer. If its length is less than
+// |MinNumberLength|, it is padded with leading zeroes or spaces, depending
----------------
Nit: Capitalize `appends` at start of sentence.


================
Comment at: lib/scudo/standalone/string_utils.h:9
+
+#ifndef SCUDO_STRING_H_
+#define SCUDO_STRING_H_
----------------
`SCUDO_STRING_UTILS_H_`


================
Comment at: lib/scudo/standalone/tests/strings_test.cc:79
+
+TEST(ScudoStringsTest, StringToNumber) {
+  EXPECT_EQ(scudo::convertStringToS64("1234", nullptr, 10), 1234);
----------------
Could you add some `UINT_MAX` and `INT_MAX` tests here?

Also `"+0", "-0", "-", "+"`, however I'm not sure how likely these are to be used in the reports/flags.


Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59262/new/

https://reviews.llvm.org/D59262