[PATCH] Fix for bug 17427 - Assertion failed: "Computed __func__ length differs from type!"

[Alexey Bataev]

Richard, thanks for the review!

================
Comment at: include/clang/AST/Expr.h:1164
@@ -1163,3 +1163,1 @@
 
-/// PredefinedExpr - [C99 6.4.2.2] - A predefined identifier such as __func__.
-class PredefinedExpr : public Expr {
----------------
rsmith wrote:
> Please don't move this class to a different place in the file (and likewise in Expr.cpp); it creates history churn and makes the patch hard to review. Instead, you can move the `getStringLiteral` method definitions out of the class (either to after the definition of `StringLiteral` or into the .cpp file).
Ok

================
Comment at: include/clang/AST/Expr.h:1577
@@ +1576,3 @@
+public:
+  PredefinedExpr(SourceLocation L, QualType T, IdentType IT, StringLiteral *SL)
+    : Expr(PredefinedExprClass, T, VK_LValue, OK_Ordinary,
----------------
rsmith wrote:
> I don't think you need to pass in `T` here; it can be obtained from the `StringLiteral`.
Yes, I thought about it too. But decided to make the constructor similar to all others.

================
Comment at: include/clang/AST/Expr.h:1598
@@ +1597,3 @@
+  StringLiteral *getStringLiteral() { return cast<StringLiteral>(StringLit); }
+  void setStringLiteral(StringLiteral *SL) { StringLit = SL; }
+
----------------
rsmith wrote:
> Generally, we prefer to make the `ASTReader` a friend of the class being deserialized rather than adding setters. Please do that here (you should also be able to remove the other setters in the process).
Agree, thanks.

================
Comment at: lib/AST/StmtProfile.cpp:472
@@ -471,2 +471,3 @@
   ID.AddInteger(S->getIdentType());
+  VisitStringLiteral(S->getStringLiteral());
 }
----------------
rsmith wrote:
> This doesn't seem necessary.
Ok

================
Comment at: lib/CodeGen/CGExpr.cpp:2057
@@ -2134,1 +2056,3 @@
+         "No string literal is associated with PredefinedExpr");
+  return EmitStringLiteralLValue(E->getStringLiteral());
 }
----------------
rsmith wrote:
> I think we need to emit this with a specific mangled name in order to merge the __func__ objects across translation units if it's used in an inline function.
Agree, I'll try to improve it.

================
Comment at: lib/Sema/TreeTransform.h:6967
@@ -6966,3 +6966,3 @@
 TreeTransform<Derived>::TransformPredefinedExpr(PredefinedExpr *E) {
-  return E;
+  return getDerived().TransformPredefinedExpr(E);
 }
----------------
rsmith wrote:
> This is wrong: unless this is overridden by the derived class, it would just call itself. Instead you should add a RebuildPredefinedExpr function, call it from here, and make it call into Sema to rebuild the PredefinedExpr.
Ok, I'll fix it

================
Comment at: test/CodeGenCXX/funcsig.cpp:11
@@ -10,3 +10,3 @@
 }
-// CHECK: private unnamed_addr constant [{{.*}} x i8] c"void __cdecl freeFunc(int *, char)\00"
+// CHECK: linkonce_odr unnamed_addr constant [{{.*}} x i8] c"void __cdecl freeFunc(int *, char)\00"
 
----------------
rsmith wrote:
> If this is supposed to be linkonce_odr, you should also check its mangled name.
Ok

http://reviews.llvm.org/D5365