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

[Philip Craig]

On Mon, Aug 20, 2012 at 10:31 AM, Victor Vicente de Carvalho
<victor.v.carvalho at gmail.com> wrote:
> But why the source location for the first initializer is completely messed
> up?

Because that's the implicit initializer. It can't have a real source
location, so it points to the constructor name instead. (Maybe this is
a clang bug and it should use an invalid source location instead, I
don't know.) So the suggestion is to skip over that one by testing
isWritten().

> I've just tried to dump the construct xml using clang -dump-ast-xml, and
> that's what I've got:

That uses DumpXML.cpp, which only has a stub for CXXCtorInitializer,
so it won't show them. I'm using a tool based on RecursiveASTVisitor
to dump it instead.

> <CXXConstructor ptr="0x4c664c0" name="Plane" previous="0x4c65000"
> prototype="true">
>   <FunctionProtoType ptr="0x4c64ae0" canonical="0x4c64ae0">
>    <BuiltinType ptr="0x4c254a0" canonical="0x4c254a0"/>
>    <parameters>
>     <LValueReferenceType ptr="0x4c5df40" canonical="0x4c5df40">
>      <QualType const="true">
>       <RecordType ptr="0x4c5d820" canonical="0x4c5d820">
>        <CXXRecord ref="0x4c5d790"/>
>       </RecordType>
>      </QualType>
>     </LValueReferenceType>
>     <BuiltinType ptr="0x4c25640" canonical="0x4c25640"/>
>    </parameters>
>   </FunctionProtoType>
>   <ParmVar ptr="0x4c66270" name="normal" initstyle="c">
>    <LValueReferenceType ptr="0x4c5df40" canonical="0x4c5df40">
>     <QualType const="true">
>      <RecordType ptr="0x4c5d820" canonical="0x4c5d820">
>       <CXXRecord ref="0x4c5d790"/>
>      </RecordType>
>     </QualType>
>    </LValueReferenceType>
>   </ParmVar>
>   <ParmVar ptr="0x4c662e0" name="d" initstyle="c">
>    <BuiltinType ptr="0x4c25640" canonical="0x4c25640"/>
>   </ParmVar>
>   <Stmt>
> (CompoundStmt 0x4c666d8 <Plane.cpp:27:55, line:29:1>)
>
>   </Stmt>
>  </CXXConstructor>
>
> for the folowing constructor:
>
> Plane::Plane (const Vector3& normal, float d) : d(d)  {
>   this->normal.set(normal).nor();
> }
>
> Somehow I'm not being able to recover the initializer list, nor the correct
> SourceLocation for it.
>
>
> Victor
>
> 2012/8/19 Philip Craig <philipjcraig at gmail.com>
>>
>> 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?
>
>