[PATCH] D75545: [dsymutil] Fix template stripping in getDIENames(...) to account for overloaded operators

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:144
   if (StripTemplate && Info.Name && Info.MangledName != Info.Name) {
-    // FIXME: dsymutil compatibility. This is wrong for operator<
-    auto Split = Info.Name.getString().split('<');
-    if (!Split.second.empty())
-      Info.NameWithoutTemplate = StringPool.getEntry(Split.first);
+    auto name = Info.Name.getString();
+
----------------
`s/auto/StringRef/`


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:148
+    if (name.endswith(">")) {
+      // If we have balanced < and > then we just have to find the first < to
+      // know where the template parameters start.
----------------
This comment adds more confusion than it clarifies. The angle brackets being balanced doesn't really matter as this point. I'd say something like:

 ```To strip the template we need to find at least one left angle bracket```


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:150
+      // know where the template parameters start.
+      size_t num_left_angles_to_count = 1;
+
----------------
Please follow the LLVM coding style. This applies to all variables. Also I'd rename this to `LeftAngleBracketsToSkip`.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:154
+      // as well.
+      num_left_angles_to_count += name.count("<=>");
+
----------------
Can this operator occur more than once?


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:161
+      // or shift operators overloaded.
+      if (left_angle_count > right_angle_count)
+        num_left_angles_to_count += left_angle_count - right_angle_count;
----------------
The comment says not balanced, but you use `>` instead of `!=`. Does that matter? 


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:169
+      Info.NameWithoutTemplate =
+          StringPool.getEntry(name.substr(0, start_of_template - 1));
+    }
----------------
Given that we always strip the end, can we simplify the algorithm to something like

```
size_t RightAngleCount = name.count('>');
size_t LeftAngleCount = name.count('<');
if (RightAngleCount != LeftAngleCount) {
	size_t AngleDifference = std::abs(RightAngleCount - LeftAngleCount);
	while (AngleDifference) {
		name = name.take_front(name.rfind('<'));
	}
} 
```

That way we don't have to hardcode the `<=>` stuff. 


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:170
+          StringPool.getEntry(name.substr(0, start_of_template - 1));
+    }
   }
----------------
We should extract this logic in a static function, something like `StringRef stripTemplateFromName(StringRef Str);`. 


================
Comment at: llvm/test/tools/dsymutil/X86/template_operators.test:23
+}
+
+struct B{};
----------------
Please test `operator<=>` as well. 


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

https://reviews.llvm.org/D75545