[Lldb-commits] [PATCH] D27780: Make OptionDefinition structure store a StringRef

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

There seems to be a bunch of places that we are passing a string literal to functions that take a StringRef. Can we pass L("--") instead of "--" to functions that take a string ref and have it be more efficient so a StringRef isn't implicitly constructed for us each time? See inlined comments for places.



================
Comment at: lldb/source/Interpreter/Args.cpp:954
+  llvm::SmallString<3> short_buffer =
+      llvm::formatv("-{0}", (char)long_options[long_options_index].val);
+  llvm::SmallString<255> long_buffer = llvm::formatv(
----------------
Will this line cause a StringRef constructor to be called on "-{0}"? If so, is there a way to avoid this with some constexpr magic?


================
Comment at: lldb/source/Interpreter/Args.cpp:956
+  llvm::SmallString<255> long_buffer = llvm::formatv(
+      "--{0}", long_options[long_options_index].definition->long_option);
 
----------------
Ditto


================
Comment at: lldb/source/Interpreter/Options.cpp:337
+bool Options::SupportsLongOption(llvm::StringRef long_option) {
+  long_option.consume_front("--");
+  if (long_option.empty())
----------------
Will this line cause a StringRef constructor to be called on "--"?


================
Comment at: lldb/source/Interpreter/Options.cpp:701
+        // level code will know this is a full match and add the " ".
+        if (cur_opt_str.startswith("--") &&
+            cur_opt_str.drop_front(2) != opt_defs[opt_defs_index].long_option) {
----------------
Will this line cause a StringRef constructor to be called on "--"?


================
Comment at: lldb/source/Interpreter/Options.cpp:719
 
-            if (strstr(def.long_option, cur_opt_str + 2) == def.long_option) {
-              std::string full_name("--");
-              full_name.append(def.long_option);
-              // The options definitions table has duplicates because of the
-              // way the grouping information is stored, so only add once.
-              bool duplicate = false;
-              for (size_t k = 0; k < matches.GetSize(); k++) {
-                if (matches.GetStringAtIndex(k) == full_name) {
-                  duplicate = true;
-                  break;
-                }
-              }
-              if (!duplicate)
-                matches.AppendString(full_name.c_str());
+        if (!cur_opt_str.consume_front("--"))
+          return true;
----------------
Will this line cause a StringRef constructor to be called on "--"?


================
Comment at: lldb/source/Interpreter/Options.cpp:728
+        for (auto &def : range) {
+          std::string full_name("--");
+          full_name.append(def.long_option);
----------------
Do we still need std::string here for full_name? We might be able to do smarter things with StringRef for all uses of full_name below, including the matches.GetStringAtIndex() by seeing if the string at index starts with "--", and then just comparing the remainder to "def.long_option"?


================
Comment at: lldb/source/Interpreter/Options.cpp:834
       // restrict it to that shared library.
-      if (cur_opt_name && strcmp(cur_opt_name, "shlib") == 0 &&
-          cur_arg_pos != -1) {
-        const char *module_name = input.GetArgumentAtIndex(cur_arg_pos);
-        if (module_name) {
+      if (cur_opt_name == "shlib" && cur_arg_pos != -1) {
+        auto module_name = input[cur_arg_pos].ref;
----------------
Will this line cause a StringRef constructor to be called on "shlib"?


https://reviews.llvm.org/D27780