[Lldb-commits] [PATCH] D27223: Expression evaluation for overloaded C functions (redux)

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:449
+/// Given a mangled function `mangled`, replace all the primitive function type
+/// arguments of `mangled` with type `replace`.
+static ConstString SubsPrimitiveParmItanium(const char *mangled,
----------------
Don't you mean `search` instead of `mangled` above?


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:450-452
+static ConstString SubsPrimitiveParmItanium(const char *mangled,
+                                            const char *search,
+                                            const char *replace) {
----------------
Use llvm::StringRef instead of "const char *" and then use the llvm::StringRef functions instead of libc functions for string searching.


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:456-458
+  const size_t search_len = ::strlen(search);
+  const size_t replace_len = ::strlen(replace);
+  const size_t mangled_len = ::strlen(mangled);
----------------
Remove since using llvm::StringRef will have lengths already.


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:460
+  const size_t max_len =
+      mangled_len + llvm::StringRef(mangled).count(search) * replace_len + 1;
+
----------------
remove llvm::StringRef temp as no longer needed since "mangled" will be a llvm::StringRef already.


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:464
+  // and copy the original there
+  std::unique_ptr<char[]> output_buf(new char[max_len]);
+  ::memset(output_buf.get(), '\0', max_len);
----------------
Why not use a std::string?


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:465-466
+  std::unique_ptr<char[]> output_buf(new char[max_len]);
+  ::memset(output_buf.get(), '\0', max_len);
+  ::strncpy(output_buf.get(), mangled, max_len - 1);
+
----------------
Remove both lines above if you use a std::string. Also, why zero it first and then copy the string over it?


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:471
+
+  auto swap_parms_hook = [&](const char *s) {
+    if (!s || !*s)
----------------
Does this only get called with parameters? Or does it get called for everything?


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:476
+    // Check whether we've found a substitutee
+    if (::strncmp(s, search, search_len) == 0) {
+      // account for the case where a replacement is of a different length
----------------
Use starts_with StringRef function.

You also need to verify that there is a word boundary at the end of this. If you are searching for "F", and replacing it with "E" and you get "Foo, Bar) const" in `s` then this will match you will change it to "Eoo, Bar) const" right? So you need to check `s[search.size]` and make sure it is a non identifier character. Unless this function only gets called with identifier boundaries. From my quick look at the FastDemangle changes, it seems that this will get called with the entire remaining part of the string.


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:481-486
+      ptrdiff_t replace_idx = (s - mangled) + replaced_offset;
+      for (size_t i = 0; avail_sz && replace[i]; ++i, ++s) {
+        output_buf[replace_idx++] = replace[i];
+        avail_sz--;
+      }
+
----------------
If you use a std::string just call .erase() and .insert()


https://reviews.llvm.org/D27223