[libc-commits] [PATCH] D108330: [libc] add atoi, atol, and atoll

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/test/src/stdlib/atol_test.cpp:42
+  ASSERT_EQ(__llvm_libc::atol(all_together), 110l);
+}
+
----------------
sivachandra wrote:
> No `LONG_MAX` tests?
> 
> Also, test for overflow values in all tests? We don't need to check the `errno` value but may be check that the return value is `*_MAX`/`*_MIN`?
I've been avoiding `LONG_MAX` in the tests, since the size of `LONG_MAX` changes based on the operating system, specifically Windows uses 32 bit `long` while linux uses 64 bit `long`. This is difficult when I'm hardcoding the strings.

There aren't any overflow value tests since overflowing is undefined behavior for `atoi`. I can add them, but it means we're testing things that aren't in the specification.


================
Comment at: libc/test/src/stdlib/atol_test.cpp:48
+
+  const char *octal = "010";
+  ASSERT_EQ(__llvm_libc::atol(hexadecimal), 10l);
----------------
sivachandra wrote:
> Is this really an invalid number?
I've changed the name of the tests, `010` isn't an invalid number, but it is interpreted as an octal number if the base is set to 0.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108330