[llvm] r306096 - [ADT] Add llvm::to_float
Pavel Labath via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 6 01:54:14 PDT 2017
For this commit, I was just trying to follow the existing style, which
is more important than the Optional vs. bool debate. (And I wasn't
aware that there is such a debate going on in the first place.)
That said, I think I also (mildly) prefer the Optional syntax, but I
can't say I really care about that. Looking at the existing handful of
uses, it looks like a couple of them would be shorter with the
optional syntax, while a couple would be more complicated (mainly
because the caller already uses a by-ref return style).
We could also have both, and use whichever is more convenient....
On 5 July 2017 at 23:46, Justin Bogner <mail at justinbogner.com> wrote:
> Pavel Labath via llvm-commits <llvm-commits at lists.llvm.org> writes:
>> Author: labath
>> Date: Fri Jun 23 07:55:02 2017
>> New Revision: 306096
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=306096&view=rev
>> Log:
>> [ADT] Add llvm::to_float
>>
>> Summary:
>> The function matches the interface of llvm::to_integer, but as we are
>> calling out to a C library function, I let it take a Twine argument, so
>> we can avoid a string copy at least in some cases.
>
> Just like in my (unaddressed) review feedback about to_integer[1], I
> feel pretty strongly that this isn't the right interface to implement.
> It would be more useable and less error prone to provide an interface
> returning optional instead:
>
> template <typename FloatTy> Optional<FloatTy> to_float(const Twine &T)
>
> [1]: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170612/461927.html
>>
>> I add a test and replace a couple of existing uses of strtod with this
>> function.
>>
>> Reviewers: zturner
>>
>> Subscribers: llvm-commits
>>
>> Differential Revision: https://reviews.llvm.org/D34518
>>
>> Modified:
>> llvm/trunk/include/llvm/ADT/StringExtras.h
>> llvm/trunk/lib/Support/CommandLine.cpp
>> llvm/trunk/lib/Support/YAMLTraits.cpp
>> llvm/trunk/unittests/ADT/StringExtrasTest.cpp
>>
>> Modified: llvm/trunk/include/llvm/ADT/StringExtras.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringExtras.h?rev=306096&r1=306095&r2=306096&view=diff
>> ==============================================================================
>>
>> --- llvm/trunk/include/llvm/ADT/StringExtras.h (original)
>> +++ llvm/trunk/include/llvm/ADT/StringExtras.h Fri Jun 23 07:55:02 2017
>> @@ -15,10 +15,12 @@
>> #define LLVM_ADT_STRINGEXTRAS_H
>>
>> #include "llvm/ADT/ArrayRef.h"
>> +#include "llvm/ADT/SmallString.h"
>> #include "llvm/ADT/StringRef.h"
>> #include <cassert>
>> #include <cstddef>
>> #include <cstdint>
>> +#include <cstdlib>
>> #include <cstring>
>> #include <iterator>
>> #include <string>
>> @@ -129,6 +131,32 @@ template <typename N> bool to_integer(St
>> return !S.getAsInteger(Base, Num);
>> }
>>
>> +namespace detail {
>> +template <typename N>
>> +inline bool to_float(const Twine &T, N &Num, N (*StrTo)(const char *, char **)) {
>> + SmallString<32> Storage;
>> + StringRef S = T.toNullTerminatedStringRef(Storage);
>> + char *End;
>> + N Temp = StrTo(S.data(), &End);
>> + if (*End != '\0')
>> + return false;
>> + Num = Temp;
>> + return true;
>> +}
>> +}
>> +
>> +inline bool to_float(const Twine &T, float &Num) {
>> + return detail::to_float(T, Num, std::strtof);
>> +}
>> +
>> +inline bool to_float(const Twine &T, double &Num) {
>> + return detail::to_float(T, Num, std::strtod);
>> +}
>> +
>> +inline bool to_float(const Twine &T, long double &Num) {
>> + return detail::to_float(T, Num, std::strtold);
>> +}
>> +
>> static inline std::string utostr(uint64_t X, bool isNeg = false) {
>> char Buffer[21];
>> char *BufPtr = std::end(Buffer);
>>
>> Modified: llvm/trunk/lib/Support/CommandLine.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CommandLine.cpp?rev=306096&r1=306095&r2=306096&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Support/CommandLine.cpp (original)
>> +++ llvm/trunk/lib/Support/CommandLine.cpp Fri Jun 23 07:55:02 2017
>> @@ -24,6 +24,7 @@
>> #include "llvm/ADT/STLExtras.h"
>> #include "llvm/ADT/SmallPtrSet.h"
>> #include "llvm/ADT/SmallString.h"
>> +#include "llvm/ADT/StringExtras.h"
>> #include "llvm/ADT/StringMap.h"
>> #include "llvm/ADT/Twine.h"
>> #include "llvm/Config/config.h"
>> @@ -1522,13 +1523,9 @@ bool parser<unsigned long long>::parse(O
>> // parser<double>/parser<float> implementation
>> //
>> static bool parseDouble(Option &O, StringRef Arg, double &Value) {
>> - SmallString<32> TmpStr(Arg.begin(), Arg.end());
>> - const char *ArgStart = TmpStr.c_str();
>> - char *End;
>> - Value = strtod(ArgStart, &End);
>> - if (*End != 0)
>> - return O.error("'" + Arg + "' value invalid for floating point argument!");
>> - return false;
>> + if (to_float(Arg, Value))
>> + return false;
>> + return O.error("'" + Arg + "' value invalid for floating point argument!");
>> }
>>
>> bool parser<double>::parse(Option &O, StringRef ArgName, StringRef Arg,
>>
>> Modified: llvm/trunk/lib/Support/YAMLTraits.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/YAMLTraits.cpp?rev=306096&r1=306095&r2=306096&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Support/YAMLTraits.cpp (original)
>> +++ llvm/trunk/lib/Support/YAMLTraits.cpp Fri Jun 23 07:55:02 2017
>> @@ -10,6 +10,7 @@
>> #include "llvm/Support/YAMLTraits.h"
>> #include "llvm/ADT/STLExtras.h"
>> #include "llvm/ADT/SmallString.h"
>> +#include "llvm/ADT/StringExtras.h"
>> #include "llvm/ADT/StringRef.h"
>> #include "llvm/ADT/Twine.h"
>> #include "llvm/Support/Casting.h"
>> @@ -912,12 +913,9 @@ void ScalarTraits<double>::output(const
>> }
>>
>> StringRef ScalarTraits<double>::input(StringRef Scalar, void *, double &Val) {
>> - SmallString<32> buff(Scalar.begin(), Scalar.end());
>> - char *end;
>> - Val = strtod(buff.c_str(), &end);
>> - if (*end != '\0')
>> - return "invalid floating point number";
>> - return StringRef();
>> + if (to_float(Scalar, Val))
>> + return StringRef();
>> + return "invalid floating point number";
>> }
>>
>> void ScalarTraits<float>::output(const float &Val, void *, raw_ostream &Out) {
>> @@ -925,12 +923,9 @@ void ScalarTraits<float>::output(const f
>> }
>>
>> StringRef ScalarTraits<float>::input(StringRef Scalar, void *, float &Val) {
>> - SmallString<32> buff(Scalar.begin(), Scalar.end());
>> - char *end;
>> - Val = strtod(buff.c_str(), &end);
>> - if (*end != '\0')
>> - return "invalid floating point number";
>> - return StringRef();
>> + if (to_float(Scalar, Val))
>> + return StringRef();
>> + return "invalid floating point number";
>> }
>>
>> void ScalarTraits<Hex8>::output(const Hex8 &Val, void *, raw_ostream &Out) {
>>
>> Modified: llvm/trunk/unittests/ADT/StringExtrasTest.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/StringExtrasTest.cpp?rev=306096&r1=306095&r2=306096&view=diff
>> ==============================================================================
>> --- llvm/trunk/unittests/ADT/StringExtrasTest.cpp (original)
>> +++ llvm/trunk/unittests/ADT/StringExtrasTest.cpp Fri Jun 23 07:55:02 2017
>> @@ -65,4 +65,22 @@ TEST(StringExtrasTest, ToAndFromHex) {
>> EvenBytes.size());
>> EXPECT_EQ(EvenStr, toHex(EvenData));
>> EXPECT_EQ(EvenData, fromHex(EvenStr));
>> -}
>> \ No newline at end of file
>> +}
>> +
>> +TEST(StringExtrasTest, to_float) {
>> + float F;
>> + EXPECT_TRUE(to_float("4.7", F));
>> + EXPECT_FLOAT_EQ(4.7, F);
>> +
>> + double D;
>> + EXPECT_TRUE(to_float("4.7", D));
>> + EXPECT_DOUBLE_EQ(4.7, D);
>> +
>> + long double LD;
>> + EXPECT_TRUE(to_float("4.7", LD));
>> + EXPECT_DOUBLE_EQ(4.7, LD);
>> +
>> + EXPECT_FALSE(to_float("foo", F));
>> + EXPECT_FALSE(to_float("7.4 foo", F));
>> + EXPECT_FLOAT_EQ(4.7, F); // F should be unchanged
>> +}
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list