Stop mkString from producing the extra space in int * x[10].

The problem already affected several regression tests for more complex
cases (which expected the extra space), and we didn't seem to notice or
care. But now it is affecting every unchanged variable of that form in a
multi-decl that gets rewritten, and this broke several other regression
tests.

Author: mattmccutchen-cci

clang/lib/3C/ConstraintVariables.cpp

@@ -813,10 +813,31 @@ PointerVariableConstraint::mkString(Constraints &CS,
     if (!ForItype && BaseType == "void")
       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;
 
@@ -906,6 +927,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);
   }
 
@@ -944,8 +967,14 @@ PointerVariableConstraint::mkString(Constraints &CS,
   }
 
   // No space after itype.
-  if (!EmittedName && !UseName.empty())
-    Ss << " " << UseName;
+  if (!EmittedName && !UseName.empty()) {
+    // REVIEW: Is there a better way? This might actually be less of a
+    // maintenance problem than trying to have all code paths correctly set an
+    // explicit flag for whether to add a space.
+    if (!StringRef(Ss.str()).endswith("*"))
+      Ss << " ";
+    Ss << UseName;
+  }
 
   // Final array dropping.
   if (!ConstArrs.empty()) {

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/macro_end_of_decl.c

@@ -38,7 +38,7 @@ int *f EQ 0;
 
 int(*g0) ARGS, g1 SIZE, *g2 EQ 0;
 //CHECK: _Ptr<int (void)> g0 = ((void *)0);
-//CHECK_NOALL: int g1[1];
+//CHECK_NOALL: int g1 SIZE;
 //CHECK_ALL: int g1 _Checked SIZE;
 //CHECK: _Ptr<int> g2 = 0;
 

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++) {