mkString fixes (mostly moved from the multi-decl overhaul)

clang/lib/3C/ConstraintVariables.cpp

@@ -833,10 +833,31 @@ PointerVariableConstraint::mkString(Constraints &CS,
     if (!ForItype && InferredGenericIndex == -1 && isVoidPtr())
       K = Atom::A_Wild;
 
+    // In a case like `_Ptr<T> g[1]`, we need to push the ` g` onto EndStrs
+    // before we can push the `>` and start drilling into T.
+    //
+    // Exception: In a case like `int *g[1]`, we don't want an extra space after
+    // the `*` but we don't yet know whether we have a `*`, so we skip emitting
+    // the name. We have to also skip the addArrayAnnotations because it would
+    // cause the array annotations to be emitted before the name.
+    //
+    // Exception to the exception: In a case like `void (*fs[2])()`, we still
+    // want to avoid emitting the name (which would add an extra space after the
+    // `*`), but we _do_ need to call addArrayAnnotations so the array
+    // annotations end up with the variable name rather than after the function
+    // parameter list. The function pointer code knows to emit the name before
+    // EndStrs.
+    //
+    // This passes the current regression tests but feels very ad-hoc.
+    // REVIEW: Help me redesign this code!
     if (PrevArr && ArrSizes.at(TypeIdx).first != O_SizedArray && !EmittedName) {
-      EmittedName = true;
-      addArrayAnnotations(ConstArrs, EndStrs);
-      EndStrs.push_front(" " + UseName);
+      if (K != Atom::A_Wild || FV != nullptr) {
+        addArrayAnnotations(ConstArrs, EndStrs);
+      }
+      if (K != Atom::A_Wild) {
+        EmittedName = true;
+        EndStrs.push_front(" " + UseName);
+      }
     }
     PrevArr = ArrSizes.at(TypeIdx).first == O_SizedArray;
 
@@ -926,6 +947,8 @@ PointerVariableConstraint::mkString(Constraints &CS,
   // the the stack array type.
   if (PrevArr && !EmittedName && AllArrays) {
     EmittedName = true;
+    // Note: If the whole type is an array, we can't have a "*", so we don't
+    // need to worry about an extra space.
     EndStrs.push_front(" " + UseName);
   }
 
@@ -978,7 +1001,7 @@ PointerVariableConstraint::mkString(Constraints &CS,
       Ss << Str;
   }
 
-  if (IsReturn && !ForItype)
+  if (IsReturn && !ForItype && !StringRef(Ss.str()).endswith("*"))
     Ss << " ";
 
   return Ss.str();

clang/test/3C/compound_literal.c

@@ -67,7 +67,7 @@ void lists() {
 
   int *d[2] = (int *[2]){&x, (int *)1};
   //CHECK_NOALL: int *d[2] = (int *[2]){&x, (int *)1};
-  //CHECK_ALL: int * d _Checked[2] = (int * _Checked[2]){&x, (int *)1};
+  //CHECK_ALL: int *d _Checked[2] = (int * _Checked[2]){&x, (int *)1};
   int *d0 = d[0];
   //CHECK: int *d0 = d[0];
 

clang/test/3C/fptr_array.c

@@ -9,15 +9,15 @@
 
 void (*fs[2])(int *);
 void (*f)(int *);
-//CHECK_NOALL: void (* fs[2])(_Ptr<int>) = {((void *)0)};
+//CHECK_NOALL: void (*fs[2])(_Ptr<int>) = {((void *)0)};
 //CHECK_NOALL: void (*f)(_Ptr<int>) = ((void *)0);
 //CHECK_ALL: _Ptr<void (_Ptr<int>)> fs _Checked[2] = {((void *)0)};
 //CHECK_ALL: _Ptr<void (_Ptr<int>)> f = ((void *)0);
 
 void (*gs[2])(int *);
 void g_impl(int *x) { x = 1; }
 void (*g)(int *) = g_impl;
-//CHECK_NOALL: void (* gs[2])(int * : itype(_Ptr<int>)) = {((void *)0)};
+//CHECK_NOALL: void (*gs[2])(int * : itype(_Ptr<int>)) = {((void *)0)};
 //CHECK_NOALL: void g_impl(int *x : itype(_Ptr<int>)) { x = 1; }
 //CHECK_NOALL: void (*g)(int * : itype(_Ptr<int>)) = g_impl;
 
@@ -30,21 +30,21 @@ void (*h)(void *);
 
 int *(*is[2])(void);
 int *(*i)(void);
-//CHECK_NOALL: _Ptr<int> (* is[2])(void) = {((void *)0)};
+//CHECK_NOALL: _Ptr<int> (*is[2])(void) = {((void *)0)};
 //CHECK_NOALL: _Ptr<int> (*i)(void) = ((void *)0);
 //CHECK_ALL: _Ptr<_Ptr<int> (void)> is _Checked[2] = {((void *)0)};
 //CHECK_ALL: _Ptr<_Ptr<int> (void)> i = ((void *)0);
 
 int *(**js[2])(void);
 int *(**j)(void);
-//CHECK_NOALL: _Ptr<int> (** js[2])(void) = {((void *)0)};
+//CHECK_NOALL: _Ptr<int> (**js[2])(void) = {((void *)0)};
 //CHECK_NOALL: _Ptr<int> (**j)(void) = ((void *)0);
 //CHECK_ALL: _Ptr<_Ptr<_Ptr<int> (void)>> js _Checked[2] = {((void *)0)};
 //CHECK_ALL: _Ptr<_Ptr<_Ptr<int> (void)>> j = ((void *)0);
 
 int *(*ks[2][2])(void);
 int *(*k)(void);
-//CHECK_NOALL: _Ptr<int> (* ks[2][2])(void) = {((void *)0)};
+//CHECK_NOALL: _Ptr<int> (*ks[2][2])(void) = {((void *)0)};
 //CHECK_NOALL: _Ptr<int> (*k)(void) = ((void *)0);
 //CHECK_ALL: _Ptr<_Ptr<int> (void)> ks _Checked[2] _Checked[2] = {((void *)0)};
 //CHECK_ALL: _Ptr<_Ptr<int> (void)> k = ((void *)0);

clang/test/3C/itype_typedef.c

@@ -34,7 +34,7 @@ void test2(td2 a) {
 typedef int *td3;
 td3 test3() {
 //CHECK: typedef _Ptr<int> td3;
-//CHECK: int * test3(void) : itype(td3) {
+//CHECK: int *test3(void) : itype(td3) {
   return (int*) 1;
 }
 

clang/test/3C/itypes_for_extern.c

@@ -30,14 +30,19 @@ int *baz(int *a, int len, int *b) {
   return a;
 }
 
+// REVIEW: The problem described in the following comment has been fixed roughly
+// in the "first way": the canonical form of the type from mkString no longer
+// has the space, so the test will pass as long as the original code didn't have
+// the space either. If we still want to pursue the second fix, do we want to
+// note that somehow, e.g., by filing an issue?
 // FIXME: The space between after the first star is needed for the idempotence
 //        test to pass. If there isn't a space there, 3c will add one when
 //        re-run on its original output. This should be fixed ideally in two
 //        ways.  First, the space shouldn't be added when not present in the
 //        original source, and second, the second conversion should not rewrite
 //        the declaration.
-void buz(int * (*f)(int *, int *)) {}
-//CHECK: void buz(int * ((*f)(int *, int *)) : itype(_Ptr<_Ptr<int> (_Ptr<int>, _Ptr<int>)>)) _Checked {}
+void buz(int *(*f)(int *, int *)) {}
+//CHECK: void buz(int *((*f)(int *, int *)) : itype(_Ptr<_Ptr<int> (_Ptr<int>, _Ptr<int>)>)) _Checked {}
 
 typedef int * int_star;
 void typedef_test(int_star p) {}

clang/test/3C/ptr_array.c

@@ -28,7 +28,7 @@ void test1(int *a) {
 
   int *b[1] = {a};
   //CHECK_NOALL: int *b[1] = {a};
-  //CHECK_ALL: int * b _Checked[1] = {a};
+  //CHECK_ALL: int *b _Checked[1] = {a};
 }
 
 /* Example from from the issue */
@@ -40,7 +40,7 @@ int *foo() {
   int z = 3;
   int *ptrs[4] = {&x, &y, &z, (int *)5};
   //CHECK_NOALL: int *ptrs[4] = {&x, &y, &z, (int *)5};
-  //CHECK_ALL: int * ptrs _Checked[4] = {&x, &y, &z, (int *)5};
+  //CHECK_ALL: int *ptrs _Checked[4] = {&x, &y, &z, (int *)5};
   int *ret;
   //CHECK: int *ret;
   for (int i = 0; i < 4; i++) {