[PATCH] D82300: [unittest, ADT] Add unit tests for itostr & utostr
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 23 01:32:48 PDT 2020
jhenderson added inline comments.
================
Comment at: llvm/unittests/ADT/StringExtrasTest.cpp:195-196
+ EXPECT_EQ("1", utostr(1, /*isNeg=*/false));
+ EXPECT_EQ("18446744073709551615", utostr(MaxUint64));
+ EXPECT_EQ("18446744073709551615", utostr(MaxUint64, /*isNeg=*/false));
+
----------------
Here, and in the below cases using the max/min values, I wonder whether it would be more robust to write something like:
```
EXPECT_EQ(std::to_string(MaxUint64), utostr(MaxUint64));
EXPECT_EQ("-" + std::to_string(MaxUint64), utostr(MaxUint64, /*isNeg=*/true);
```
etc.
I'd expect them to produce identical output, but it also is easier to confirm the expected number. It also removes any concerns about numeric representation (2's complement etc).
================
Comment at: llvm/unittests/ADT/StringExtrasTest.cpp:198
+
+ EXPECT_EQ("-0", utostr(0, /*isNeg=*/true));
+ EXPECT_EQ("-1", utostr(1, /*isNeg=*/true));
----------------
thopre wrote:
> Since there's no comment, I'm not sure whether it is supported by the API or not. I'd be tempted to add an assert and a comment not to support it.
I'm inclined to leave it as-is. It's possible it was a simple oversight by the original developer, but I think it's okay to permit it for consistency. People could be using it in the sense of "subtract 0" rather than "negative 0" in theory, apart from anything else.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82300/new/
https://reviews.llvm.org/D82300
More information about the llvm-commits
mailing list