[Lldb-commits] [PATCH] Handle trailing spaces on "settings set" command more correctly

[Greg Clayton]

See inline code comments.

Overall good patch, we should not switch just one (StringToBoolean) over to use llvm::StringRef unless we plan to change all of them. I would be happy, possibly in another patch to see:

  OptionValue::SetValueFromCString(const char *s, ...)

be switched over to:

OptionValue::SetValueFromString(llvm::StringRef str, ...)


================
Comment at: include/lldb/Interpreter/Args.h:21
@@ -20,2 +20,3 @@
 // Other libraries and framework includes
+#include "llvm/ADT/StringRef.h"
 // Project includes
----------------
Remove this if you agree with my comments on line 385.

================
Comment at: include/lldb/Interpreter/Args.h:385
@@ -383,3 +384,3 @@
     static bool
-    StringToBoolean (const char *s, bool fail_value, bool *success_ptr);
+    StringToBoolean (llvm::StringRef s, bool fail_value, bool *success_ptr);
 
----------------
Unless we are going to change all StringTo calls with llvm::StringRef, I would leave this as a "const char *" and just make a local llvm::StringRef inside StringToBoolean.

================
Comment at: source/Interpreter/Args.cpp:871
@@ -870,3 +870,3 @@
 bool
-Args::StringToBoolean (const char *s, bool fail_value, bool *success_ptr)
+Args::StringToBoolean (llvm::StringRef s, bool fail_value, bool *success_ptr)
 {
----------------
We should use "const char *" for this arg unless we plan to switch all StringTo functions over to use llvm::StringRef and just make a local llvm::StringRef.

================
Comment at: source/Interpreter/Args.cpp:874-877
@@ -874,1 +873,6 @@
+    s = s.trim();
+    if (s.compare_lower("false") == 0 ||
+        s.compare_lower("off") == 0 ||
+        s.compare_lower("no") == 0 ||
+        s.compare_lower("0") == 0)
     {
----------------
We should use equals_lower(...) instead of compare_lower here.

================
Comment at: source/Interpreter/Args.cpp:884-887
@@ +883,6 @@
+    else
+    if (s.compare_lower("true") == 0 ||
+        s.compare_lower("on") == 0 ||
+        s.compare_lower("yes") == 0 ||
+        s.compare_lower("1") == 0)
+    {
----------------
We should use equals_lower(...) instead of compare_lower here.

http://reviews.llvm.org/D7592

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/