[cfe-dev] Criticism over a clang tool to reorder fields on constructor declarations

[Philip Craig]

On Mon, Aug 20, 2012 at 2:10 AM, James Dennett <james.dennett at gmail.com> wrote:
> On Sun, Aug 19, 2012 at 12:15 AM, Philip Craig <philipjcraig at gmail.com> wrote:
>> On Sun, Aug 19, 2012 at 1:52 PM, Victor Vicente de Carvalho
>> <victor.v.carvalho at gmail.com> wrote:
>>> Hi Manuel,
>>>
>>> my rewrite basing only in source replacements is complete, but now I'm
>>> facing some strange side effects on some files. Consider the example:
>>>
>>> header ----------
>>>
>>> #include <B.h> // B defines a simple class named B
>>>
>>> class A {
>>> public:
>>>   A();
>>>
>>>   float x;
>>>   B b;
>>> };
>>>
>>> source ---------------
>>>
>>> A::A() : x(0) {
>>> }
>>>
>>>
>>> the strange behavior that I'm having is that if in the header file "x" is
>>> declared before "b", when processing the source location for the initializer
>>> list on CXXConstructorDecl*, the source location for it get's completely
>>> wrong pointing to right before the constructor name: "A::". If i declare "b"
>>> before "x" then the source location points to "A::A() : " which was
>>> expected. Also, the method getNumCtorInitializers on this declaration is
>>> returning to me 2 ( should be 1 as only x is being initialized ), but if
>>> decide to initialize b I'm __still__ getting 2. Also, node that the class
>>> I'm parsing does not have a base class, and I'm providing the correct
>>> includes as the tool run fine and exits returning 0. I'm a bit lost here,
>>> any comments are welcome. I could upload on gist the current tool source,
>>> cmake and sample code I'm using to test it if someone is willing to try.
>>>
>>> I've also tried to dump the ast and verify it but the initalizer list is not
>>> showing up so I don't know what might be happening.
>>
>> The AST for the constructor looks like this:
>>
>>   CXXConstructorDecl <A.cc:2:1-3:1>
>>     NestedNameSpecifier <2:1-2> A::
>>       RecordType <2:1>
>>     DeclarationName <2:4> A
>>     FunctionProtoType <-6>
>>       BuiltinType void <>
>>     CXXCtorInitializer  <2:10-13>
>>       FieldDeclRef <2:10> A::x
>>       ImplicitCastExpr <2:12>
>>         IntegerLiteral <2:12> 0
>>     CXXCtorInitializer  <2:4>
>>       FieldDeclRef <2:4> A::b
>>     CompoundStmt <2:15-3:1>
>>
>> Reversing the order of x and b just reverses the order of the two
>> CXXCtorInitializer in the AST.
>>
>> So it looks like clang always creates implicit initializers for
>> members that aren't explicitly initialized, but it has no way of
>> flagging them as implicit. I don't know the expected way of
>> determining which are implicit, maybe checking for a NULL getInit().
>
> CXXCtorInitializer::isWritten "Returns true if this initializer is
> explicitly written in the source code."

Thanks, I missed that. I see RecursiveASTVisitor is already checking
that, but only to skip over getInit(). Would it make sense for RAV to
skip over the CXXCtorInitializer completely unless
shouldVisitImplicitCode() is true?