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

Sat Aug 18 20:52:11 PDT 2012

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.

Cheers,

Victor

2012/8/17 Manuel Klimek <klimek at google.com>

> On Sat, Aug 18, 2012 at 12:16 AM, Victor Vicente de Carvalho
> <victor.v.carvalho at gmail.com> wrote:
> > 2012/8/17 Manuel Klimek <klimek at google.com>
> >>
> >> On Fri, Aug 17, 2012 at 9:34 PM, Victor Vicente de Carvalho
> >> <victor.v.carvalho at gmail.com> wrote:
> >> > Hello Manuel,
> >> >
> >> > Sadly, no. I'm struggling with some problems now:
> >> >
> >> > 1 - How do I detect and generate the class name for nested classes?
>  I'm
> >> > using  CXXConstructorDecl getNameAsString() but it only returns to me
> >> > the
> >> > name of the enclosing class.
> >>
> >> As Jends said, getQualifiedNameAsString.
> >>
> >
> > Thanks! It worked
> >
> >
> >>
> >> > 2 - I would like to not replace constructor initializers that were set
> >> > up
> >> > with macros with their values:
> >> >
> >> > #define MACRO 10
> >> >
> >> > Class::Class() : myval(MACRO)
> >> >
> >> > after I rewrite the outputted code is:
> >> >
> >> > Class::Class() : mvyal(10)
> >> >
> >> > Is there a way to avoid this? I've tried to use
> >> > Compiler.getPreprocessor().SetMacroExpansionOnlyInDirectives() but
> this
> >> > simply removes the myval definition from the generated AST
> >>
> >> Source locations have the notion of an expansion and a spelling
> >> location. To get the code that was originally written, you'll need to
> >> fiddle around with those. In your case, MACRO is the expansion
> >> location of the literal 10.
> >>
> >> > 3 - Variables that are default initialized on the header are stored
> with
> >> > it.
> >> > This is a problem as the compiler complains when I rewrite the source
> >> > file
> >> > with the initialization on the source but it was previously defined on
> >> > the
> >> > header. Is there to detect if the default initializer is on the method
> >> > definition or in the method declaration?
> >>
> >> I would assume so - I don't where to look of the top of my head though.
> >>
> >> >
> >> > 4 - I've found a bug on the DeclPrinter class, and fixed it. The
> >> > attachment
> >> > is the patch.
> >>
> >> I suggest you send an email to cfe-commits with the subject [PATCH]
> >> Fix problem with DeclPrinter, containing the patch and what was wrong
> >> (and ideally an automated regression test ;)
> >>
> >> >
> >> > 5 - The current way i'm using to replace the old constructor
> declaration
> >> > with the new declaration is with a new range:
> >> >
> >> > SourceRange rng(d->getSourceRange().getBegin(),
> >> > d->getBody()->getSourceRange().getBegin().getLocWithOffset(-1));
> >> >
> >> > This was the only way I've managed to get the correct end of the end
> of
> >> > the
> >> > initializer list. This is an inconvenience as I would like to not
> >> > process
> >> > the body and hence make the tool faster, but if I disable body
> parsing I
> >> > can't get its SourceRange.
> >>
> >> As far as I understand (and I might well be wrong there) skipping body
> >> parsing has some very concrete use cases, and source-to-source
> >> transformations is not on of them...
> >>
> >> Cheers,
> >> /Manuel
> >>
> >
> > Manuel, I think I've been thinking the wrong way. As the only thing I
> need
> > is to reorder the initialization, I do not need to mess with the original
> > source like I was doing and just concatenate the reordered, original
> > CXXCtorInitializer string data. So I've been wondering how I'm supposed
> to
> > recover the source string based on a SourceLocation. Should I use
> > SourceManager's getCharacterData? There is a way to recover the entire
> > string based on a sourceRange?
>
> RefactoringTool in lib/Tooling has some examples of how to deal with
> that. They're not perfect yet, as source locations can be a little
> fiddly to handle. In general, SourceManager's getCharacterData sounds
> like a good way to go as long as it works for your use case.
>
> Cheers,
> /Manuel
>
> >
> > Cheers,
> >
> > Victor
> >
> >
> >
> >>
> >> >
> >> >
> >> > 2012/8/17 Manuel Klimek <klimek at google.com>
> >> >>
> >> >> Hi Victor,
> >> >>
> >> >> if I remember correctly we've been talking in IRC - have all your
> >> >> issues been resolved?
> >> >>
> >> >> Cheers,
> >> >> /Manuel
> >> >>
> >> >> On Thu, Aug 9, 2012 at 5:22 AM, Victor Vicente de Carvalho
> >> >> <victor.v.carvalho at gmail.com> wrote:
> >> >> > Hello,
> >> >> >
> >> >> > In the past month I've been working in a clang tool to refactor the
> >> >> > fields
> >> >> > which are declared on a class constructor to match the order
> declared
> >> >> > on
> >> >> > the
> >> >> > header file, hence resolving the annoying Wreorder warning. After a
> >> >> > series
> >> >> > of difficulties finally I have something that is close to the
> ideal,
> >> >> > and
> >> >> > would like to someone more acquainted with clang internals to check
> >> >> > if
> >> >> > it's
> >> >> > the best form to implement this.
> >> >> >
> >> >> > Also, I've got some problems that are still remaining:
> >> >> >
> >> >> > 1 - I've been following the samples I've managed to find through
> the
> >> >> > internet, but every time I try to run the tool using the following
> >> >> > command
> >> >> > line (I'm intentionally not providing any include paths as I don't
> >> >> > want
> >> >> > to
> >> >> > std to be parsed):
> >> >> >
> >> >> > ../cpp-fixya <SOURCE.CPP>  --
> >> >> >
> >> >> > I get the following errros:
> >> >> >
> >> >> > <premain>: CommandLine Error: Argument 'version' defined more than
> >> >> > once!
> >> >> > <premain>: CommandLine Error: Argument 'print-all-options' defined
> >> >> > more
> >> >> > than
> >> >> > once!
> >> >> > <premain>: CommandLine Error: Argument 'print-options' defined more
> >> >> > than
> >> >> > once!
> >> >> > <premain>: CommandLine Error: Argument 'help-hidden' defined more
> >> >> > than
> >> >> > once!
> >> >> > <premain>: CommandLine Error: Argument 'help' defined more than
> once!
> >> >> > <premain>: CommandLine Error: Argument 'debug-only' defined more
> than
> >> >> > once!
> >> >> > <premain>: CommandLine Error: Argument 'debug-buffer-size' defined
> >> >> > more
> >> >> > than
> >> >> > once!
> >> >> > <premain>: CommandLine Error: Argument 'debug' defined more than
> >> >> > once!
> >> >> > cpp-fixya: CommandLine Error: Argument 'version' defined more than
> >> >> > once!
> >> >> > cpp-fixya: CommandLine Error: Argument 'print-all-options' defined
> >> >> > more
> >> >> > than
> >> >> > once!
> >> >> > cpp-fixya: CommandLine Error: Argument 'print-options' defined more
> >> >> > than
> >> >> > once!
> >> >> > cpp-fixya: CommandLine Error: Argument 'help-hidden' defined more
> >> >> > than
> >> >> > once!
> >> >> > cpp-fixya: CommandLine Error: Argument 'help' defined more than
> once!
> >> >> > cpp-fixya: CommandLine Error: Argument 'debug-only' defined more
> than
> >> >> > once!
> >> >> > cpp-fixya: CommandLine Error: Argument 'debug-buffer-size' defined
> >> >> > more
> >> >> > than
> >> >> > once!
> >> >> > cpp-fixya: CommandLine Error: Argument 'debug' defined more than
> >> >> > once!
> >> >> >
> >> >> >
> >> >> > 2 - I haven't found a way to determine the class name for nested
> >> >> > classes.
> >> >> > How would be the best way to detect and generate the correct naming
> >> >> > to a
> >> >> > nested class? I've been using:
> >> >> >
> >> >> > d->getNameAsString()
> >> >> >
> >> >> > but it only returns the enclosing class
> >> >> >
> >> >> >
> >> >> > 3 - How do I determine if a class declaration is on a header or a
> >> >> > source
> >> >> > file? I mean that because if it's a source-only declaration (i.e.
> >> >> > private
> >> >> > classes) the c++ compiler wont accept constructors in the form:
> >> >> >
> >> >> > Constructor::Constructor()  : <init_declrs> {
> >> >> > }
> >> >> >
> >> >> > only in the form:
> >> >> >
> >> >> > Constructor()  : <init_declrs> {
> >> >> > }
> >> >> >
> >> >> >
> >> >> > Here is the code:
> >> >> >
> >> >> > https://gist.github.com/3300494
> >> >> >
> >> >> >
> >> >> > Thanks in advance,
> >> >> >
> >> >> >
> >> >> > Victor
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >> >> > _______________________________________________
> >> >> > cfe-dev mailing list
> >> >> > cfe-dev at cs.uiuc.edu
> >> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
> >> >> >
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20120819/aabbb534/attachment.html>