[cfe-dev] Fix PR13444 - wrong mangling of "const char * const *" and friends

[Timur Iskhodzhanov]

On Fri, Aug 31, 2012 at 8:11 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Fri, Aug 31, 2012 at 7:55 AM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>> Can you please review this patch?
>> This fixes http://llvm.org/PR13444 as well as a large part of
>> http://llvm.org/PR13182 plus adds quite a few tests.
>>
>>
>> +void MicrosoftCXXNameMangler::manglePointerQualifiers(Qualifiers Quals) {
>> +  // <pointer-cvr-qualifiers> ::= P  # no qualifiers
>> +  //                          ::= Q  # const
>> +  //                          ::= R  # volatile
>> +  //                          ::= S  # const volatile
>> +  if (!Quals.hasVolatile()) {
>> +    if (!Quals.hasConst()) {
>> +      Out << 'P';
>> +    } else {
>> +      Out << 'Q';
>> +    }
>> +  } else {
>> +    if (!Quals.hasConst()) {
>> +      Out << 'R';
>> +    } else {
>> +      Out << 'S';
>> +    }
>> +  }
>> +}
>
> I wonder if this could be made a bit more clear, a la something like this:
>
> bool HasConst = Quals.hasConst();
> bool HasVolatile = Quals.hasVolatile();
>
> if (hasVolatile && hasConst)
>   Out << 'S';
> else if (hasConst)
>   Out << 'Q';
> else if (hasVolatile)
>   Out << 'R';
> else
>   Out << 'P';
>
> As it stands, I had to think about the logic due to the nesting.
Actually what you're suggesting is exactly how I initially wrote it :)
But then I saw MicrosoftCXXNameMangler::mangleQualifiers() and decided
to take that approach.
Let me fix both functions then...

> Other than that, it LGTM.  Thanks!
I suppose your LGTM is enough to commit.

> ~Aaron