[cfe-commits] Source code fidelity problem after commit 168895.

[Douglas Gregor]

On Jan 3, 2013, at 1:03 AM, Enea Zaffanella <zaffanella at cs.unipr.it> wrote:

> On 01/03/2013 04:16 AM, Rafael Espíndola wrote:
>>> After this commit, for the following code
>>> 
>>> $ cat merge.c
>>> typedef int A;
>>> A f();
>>> typedef int B;
>>> B f();
>>> 
>>> we obtain the following AST dump, where typedef B gets replaced by A in the
>>> second declaration of f():
>>> 
>>> $ clang -cc1 -ast-dump merge.c
>>> [...]
>>>   (TypedefDecl 0x4e1f220 <merge.c:1:1, col:13> A 'int')
>>>   (FunctionDecl 0x4e1f310 <line:2:1, col:5> f 'A ()')
>>>   (TypedefDecl 0x4e1f3c0 <line:3:1, col:13> B 'int')
>>>   (FunctionDecl 0x4e1f480 <line:4:1, col:5> f 'A ()'))
>>> 
>>> 
>>> The chunk responsible for this problem seems to be the following:
>>> 
>>>> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=168895&r1=168894&r2=168895&view=diff
>>>> 
>>>> ==============================================================================
>>>> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
>>>> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Thu Nov 29 10:09:03 2012
>>>> @@ -2402,6 +2402,12 @@
>>>>     if (getLangOpts().CPlusPlus)
>>>>       return MergeCXXFunctionDecl(New, Old, S);
>>>> 
>>>> +  // Merge the function types so the we get the composite types for the
>>>> return
>>>> +  // and argument types.
>>>> +  QualType Merged = Context.mergeTypes(Old->getType(), New->getType());
>>>> +  if (!Merged.isNull())
>>>> +    New->setType(Merged);
>>>> +
>>>>     return false;
>>>>   }
>>> 
>>> 
>>> As a bare minimum, I'd suggest to exchange the order of arguments in the
>>> call to mergeTypes():
>>> 
>>> -  QualType Merged = Context.mergeTypes(Old->getType(), New->getType());
>>> +  QualType Merged = Context.mergeTypes(New->getType(), Old->getType());
>>> 
>>> This will fix (just) the test case above ...
>>> however, in general, what happens to the *syntactic* type after merging?
>>> Is it going to be simply discarded (i.e., replaced by the merged one)?
>>> If so, what about source code fidelity?
>> 
>> That is a good question. I noticed that clang was merging some of the
>> types, but not all. Subsequent passes of clang expect the merged type,
>> so merging the missing cases seemed the right answer.
>> 
>> So in general clang doesn't keep the "type as written". It should be
>> possible to always keep that, and have getType() do the merging, but
>> that is probably really expensive, which I assume is why we don't keep
>> it. Douglas, is that the case?
> 
> 
> mmmhhh ... I now think I may have been overlooking something.
> 
> When clang merges types, will it touch the TypeSourceInfo at all?
> If that info is fully preserved, then source fidelity will be preserved too (as far as I remember the two methods getType and getTypeSourceInfo return different data for function declarations).

TypeSourceInfo is preserved: use that if you want to know what the user wrote. 

getType() is the semantic type, which may have been adjusted.