[clang-tidy] Some possible contributions

Well, taking the assumption that all variable declarations are single declarations would simplify my localizing variables checker a great deal and make issue #1 obsolete.

On the other hand, I worry about the decreased usability from the user's point of view.
He would have to first run the checker that splits multiple declarations, applying its fixes everywhere.
Only then, as a second step, he would be able to run the localizing variables checker, and use its fixes.

I'm not sure if we want to go in that direction. As far as I know, so far there aren't any checkers that make such assumptions, generating such implicit dependencies.

Best regards,
Piotr Dziwinski

I just uploaded the code to pastebin:

DeclarationTransformer: http://pastebin.com/GpzpJGiz
VariableDeclaration: http://pastebin.com/SPMNJvkY

DeclarationTransformer is a Matcher/MatcherCallback and VariableDeclaration a helper struct.

DeclarationTransformer declarationTransformer(context, rewriter);
declarationTransformer.doTransformation();

I am using this in a bigger clang-tool that I am writing right now and thus there are still TODOs and work to do. But I think this should help for now. Furthermore, it is only used for C-code. So auto and co. won’t work (I think) since it is not been tested for C++ at all.

Here an example before (http://pastebin.com/M6Qkwbgi) and after (http://pastebin.com/9jAQGd5Q) transformation. And of course, I will appreciate any improvements.

Best,
Firat

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <56709B81.2020403@gmail.com>,
    Piotr Dziwinski via cfe-dev <cfe-dev@lists.llvm.org> writes:

> I remember now that I was looking into this for a check that would
> split a multiple variable declaration into multiple single variable
> declarations. [...]

Well, taking the assumption that all variable declarations are single
declarations would simplify my localizing variables checker a great deal
and make issue #1 obsolete.

OK, good we're on the same thought track.

On the other hand, I worry about the decreased usability from the user's
point of view.

Agreed.

He would have to first run the checker that splits multiple
declarations, applying its fixes everywhere.
Only then, as a second step, he would be able to run the localizing
variables checker, and use its fixes.

How would a user feel if the localizing check broke down multiple
delcarations into single declarations as a by-product?

In other words:

int i, j, k;
// many lines of code
i = 10;

becomes

int j;
int k;
// many lines of code
int i = 10;

Would this be unacceptable?

I just improved and simplified the code. Now auto, decltype and initializer_list work too. As you can see in the example, auto is automatically transformed into its deduced type (there is a way to keep the auto keyword) and long int will get long. An additional if-statement would change both behavior if you like.

Now, you only need the DeclarationTransformer: http://pastebin.com/Luu1i9s3
Example before and after: http://pastebin.com/KnrzNWnMhttp://pastebin.com/rLmPZRxP

There is always a simple solution! :slight_smile:

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <CAC8rT1_WHgg4wE0Tn2s-GqWSZ_tA5UJYF=sSpBh0c+QoGu93zw@mail.gmail.com>,
    Firat Kasmis via cfe-dev <cfe-dev@lists.llvm.org> writes:

I just improved and simplified the code. Now auto, decltype
and initializer_list work too. As you can see in the example, auto is
automatically transformed into its deduced type (there is a way to keep the
auto keyword) and long int will get long. An additional if-statement would
change both behavior if you like.

Now, you only need the DeclarationTransformer: http://pastebin.com/Luu1i9s3
Example before and after: http://pastebin.com/KnrzNWnM ->
http://pastebin.com/rLmPZRxP

There is always a simple solution! :slight_smile:

Awesome!

A couple other things to try:

- C++11 brace initialization

    std::vector<std::string> s{"foo"s, "bar"s}, t{"foo"s}, u;
=>
    std::vector<std::string> s{"foo"s, "bar"s};
    std::vector<std::string> t{"foo"s};
    std::vector<std::string> u;

- function declarations

    void f(int), g(int, float);
=>
    void f(int);
    void g(int, float);

- pointers to functions

    void gg(int, float);
    void (*f)(int), (*g)(int, float) = gg;
=>
    void gg(int, float);
    void (*f)(int);
    void (*g)(int, float) = gg;

- pointers to member data

    struct S { int a; const int b; };
  int S::*p = &S::a, S::* const q = &S::a;
  const int S::*r = &S::b, S::*t;
=>
    struct S { int a; const int b; };
  int S::*p = &S::a;
  int S::* const q = &S::a;
  const int S::*r = &S::b;
  const int S::*t;

- pointers to member functions

    struct S { int f(); };
  int (S::*p)() = &S::f, (S::*q)();
=>
    struct S { int f(); };
  int (S::*p)() = &S::f;
  int (S::*q)();

I haven't looked at your implementation to specifically see if any of
these are a problem, but in my experience these more "exotic" types
are often a weak spot in refactoring tools. clang tooling
infrastructure tends to do a little better here because it's a real
parser.

Great! Thanks for the test cases. I am going to implement the rest and keep you guys updated. Btw, where is your repository so I can contribute directly into your repository instead of doing this here in the mailing list.

[Please reply only to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <CAC8rT1_WHgg4wE0Tn2s-GqWSZ_tA5UJYF=sSpBh0c+QoGu93zw@mail.gmail.com>,
Firat Kasmis via cfe-dev <cfe-dev@lists.llvm.org> writes:

I just improved and simplified the code. Now auto, decltype
and initializer_list work too. As you can see in the example, auto is
automatically transformed into its deduced type (there is a way to keep the
auto keyword) and long int will get long. An additional if-statement would
change both behavior if you like.

Now, you only need the DeclarationTransformer: http://pastebin.com/Luu1i9s3
Example before and after: http://pastebin.com/KnrzNWnM
http://pastebin.com/rLmPZRxP

There is always a simple solution! :slight_smile:

Awesome!

A couple other things to try:

  • C++11 brace initialization

std::vectorstd::string s{"foo"s, "bar"s}, t{"foo"s}, u;
=>
std::vectorstd::string s{"foo"s, "bar"s};
std::vectorstd::string t{"foo"s};
std::vectorstd::string u;

  • function declarations

void f(int), g(int, float);
=>
void f(int);
void g(int, float);

  • pointers to functions

void gg(int, float);
void (*f)(int), (*g)(int, float) = gg;
=>
void gg(int, float);
void (*f)(int);
void (*g)(int, float) = gg;

  • pointers to member data

struct S { int a; const int b; };
int S::p = &S::a, S:: const q = &S::a;
const int S::*r = &S::b, S::*t;
=>
struct S { int a; const int b; };
int S::p = &S::a;
int S::
const q = &S::a;
const int S::*r = &S::b;
const int S::*t;

  • pointers to member functions

struct S { int f(); };
int (S::*p)() = &S::f, (S::*q)();
=>
struct S { int f(); };
int (S::*p)() = &S::f;
int (S::*q)();

I haven’t looked at your implementation to specifically see if any of
these are a problem, but in my experience these more “exotic” types
are often a weak spot in refactoring tools. clang tooling
infrastructure tends to do a little better here because it’s a real
parser.

[Please reply *only* to the list and do not include my email directly
in the To: or Cc: of your reply; otherwise I will not see your reply.
Thanks.]

In article <CAC8rT1-xjLDoVD2rWRY2=FGJGoQwFD7iw-H_cv1d54dzx+00_g@mail.gmail.com>,
    Firat Kasmis via cfe-dev <cfe-dev@lists.llvm.org> writes:

Great! Thanks for the test cases. I am going to implement the rest and keep
you guys updated. Btw, where is your repository so I can contribute
directly into your repository instead of doing this here in the mailing
list.

I make contributions through Phabricator, it's pretty easy:
<http://llvm.org/docs/Phabricator.html>

I don't have write access to the repository so when my change is
accepted, I ask someone else to commit it and that's worked great for
me so far.

Well, my checker doesn't have anything against multiple declarations per se. There are cases where they are not considered a problem.
For example:

   int a, b;
   useByReference(a, b);

Here, both variables are already declared just before they are used so there is nothing to do, and no diagnostic is generated.
I would like to keep it like that, as any change suggested here, even if's just splitting the declarations, would be confusing to the user.

The only problem is when variables from multiple declaration statement must be moved, possibly to different locations.
In such case, since we are already changing the line with declaration, we might as well do as you suggest and split the declaration.
Actually, it would be quite simple to implement as well: remove the whole declaration and then insert individual variables as single declarations,
either somewhere else if they are to be moved, or in the place of old declaration if they are to be left behind as they were.

However, if we go this way, there is a problem with generating the diagnostics in a sensible way.

So far, the idea was to have:
   1 diagnostic = 1 variable = 1 FixIt hint removal from old location = 1 FixIt hint insertion at new location.

If we decide to do the splitting of multiple declarations the way I described, it would have to be:
  1 diagnostic = 1 (multiple) declaration statement = 1 FixIt hint removal from old location = 1 or more FixIt hint insertions for the individual variables.

I'm not sure if it is better or worse from the user's POV.

Perhaps you are right and the easiest way out would be to assume that all declarations are single declarations,
and let the user deal with multiple declarations by running another checker to split them.

Best regards,
Piotr Dziwinski