[PATCH] D21011: [codeview] Add complex record type translation

Amjad Aboud via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 8 07:29:18 PDT 2016


aaboud marked 6 inline comments as done.
aaboud added a comment.

Thanks for the feedback, I will apply fixes in next patch.
Please see my response on some comments below.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:876-878
@@ -871,1 +875,5 @@
 
+  // While processing the type being pointed to it is possible we already
+  // created this pointer type.  If so, we check here and return the existing
+  // pointer type.
+  //
----------------
rnk wrote:
> We should assert if this happens. I'd be interested in a test case that triggered this scenario.
No need to add  assertion, here is an example:

```
struct A {
  A* next;
};

void foo() {
 A *p;
 A a;
}
```

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:964-966
@@ +963,5 @@
+
+  // While processing the type being pointed to, it is possible we already
+  // created this modifier type.  If so, we check here and return the existing
+  // modifier type.
+  //
----------------
rnk wrote:
> Ditto, this is bad.
And once again, an example :)

```
struct A {
  const A* next;
  A();
};

void foo() {
 const A a;
}
```


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1301
@@ +1300,3 @@
+
+static StringRef stripScopesFromName(StringRef name) {
+  StringRef::size_type pos;
----------------
rnk wrote:
> Please don't commit this code if we can just fix it in clang. It's really easy to change our name printing code, we have a printing option that detects if codeview is enabled.
Actually, I think we can change the way clang generate DISubprogram names: http://reviews.llvm.org/rL255744
This commit was not the right thing to do, because when generating function inside FieldList we need to give it the naked name (without scoping), however in all other places we will need the full name with the scope.

If you ask me, I prefer that Clang do not add the scope name prefix to the function name, and lit this to the be done by the CodeViewDebug component in the Backend.



http://reviews.llvm.org/D21011





More information about the llvm-commits mailing list