Various bugs in mkString

[mattmccutchen-cci]

PointerVariableConstraint::mkString has bugs in how it prints certain types. I guess they aren't affecting important use cases yet, but they might in the future, and in principle, we'd like mkString to be able to correctly format arbitrary types. We can start with this as an umbrella issue and file more specific issues if it's helpful.

1. int *x gets printed as int * x. Fixed by Insert itypes correctly for constant sized arrays #702.
2. _Ptr<int> *x gets printed as int *_Ptr<int> x. This is hard to trigger in the current 3C but might become more common if the "outer wild -> inner wild" constraints were removed, as described in Remove double-pointer "outer wild -> inner wild" constraints? #656.
3. Some cases involving arrays will be fixed in mkString fixes (mostly moved from the multi-decl overhaul) #714.
4. One way to find more bugs is to temporarily change the generation of the unchecked side in DeclRewriter::buildItypeDecl to always use mkString rather than getting the type from Clang (qtyToStr, etc.) and then run the regression tests. One common bug is an extra space in a function return type like int * foo(void) : itype(_Ptr<int>), apparently coming from this code; this will be fixed in mkString fixes (mostly moved from the multi-decl overhaul) #714. I didn't look through all the test failures to see if there are other bugs.
5. A pointer to a fixed-size array like int (*x)[10] gets printed as int *[10] x. I haven't been able to find an example that triggers mkString on this case without modifying the code as in (4). However, there's a similar example without a name, where the only problem is missing parentheses around the * in the output:

   int (*q)[10] = 1;
   int (**p)[10] = &q;

   The output:

   int (*q)[10] = 1;
   _Ptr<int *[10]> p = &q;

6. (mkString writes wild pointer levels in reverse order (mixes up type qualifiers) #161) mkString traverses type levels from outer to inner and prints wild pointer symbols * directly to Ss in the order they are seen, so the outermost * ends up on the left and the innermost * on the right. It should be the other way around. One notable case in which this makes a difference is when the qualifiers on the different levels differ. Example:

   void test(void) {
     int *const *volatile q = 1;
     int *const *volatile *p = &q;
   }

   The output:

   void test(void) {
     int *const *volatile q = 1;
     _Ptr<int *volatile *const > p = &q;
   }

More generally, the logic in PointerVariableConstraint::mkString is very ad-hoc and could use a thorough re-think.

[john-h-kastner]

Item 6 sounds like #161

[mattmccutchen-cci]

Item 6 sounds like #161

Yes indeed! Thanks. I think I'll keep the duplicate copy of the description here so we can have it at hand as we consider any larger improvements to mkString.

I think I now know the basic redesign that would be needed to fix problems (5) and (6) and maybe other problems we haven't yet specifically identified. In essence, since the declarator syntax for wild pointers and both wild and checked arrays is inside-out (in contrast to checked pointers, which are right-side-out), so as we scan the type levels from outer to inner, we need to be able to add strings for inner type levels onto both the left and the right sides of the declarator. Currently, ConstArrs accumulates arrays on the right side of the declarator, but we need to extend it to accommodate right parentheses for pointers to arrays (as seen in item (5)) and add an analogous list for the left side. I could probably implement the basic idea in an hour or two, but I don't know how long it would take to get it working in all cases and cleaned up and get it through code review, so let's finish #714 and decide later about this further improvement. I've already ended up spending much more time on #714 than Mike may have foreseen when he expressed an opinion that we "may as well" get those fixes finished and merged. 😕