fix unused for loop var checker for macro expansion scenario

[ivafanas]

Quite the same problem with macro expansion locations is applicable to UFLV plugin.

Proposal allows to find unused variables in a code like:

 24 #define GENERATE_UNUSED_RANGE_FOR_LOOP_VAR(arr, inc) \
 25     do {\
 26         for (auto x : arr)\
 27             ++inc;\
 28     }\
 29     while (0)
 30
 31 int foo4(int var_a) {
 32     int arr[3] = {1, 2, 3};
 33     GENERATE_UNUSED_RANGE_FOR_LOOP_VAR(arr, var_a);
 34     return var_a;
 35 }
 36
 37 #define GENERATE_UNUSED_REGULAR_FOR_LOOP_VAR(size, inc) \
 38     do {\
 39         for (int x = 0; x < size; ++x)\
 40             ++inc;\
 41     }\
 42     while (0)
 43
 44 int foo5(int var_a) {
 45     GENERATE_UNUSED_REGULAR_FOR_LOOP_VAR(3, var_a);
 46     return var_a;
 47 }

Report message is:

input_unusedforloopvar.cpp:33:5: warning: (AST Matcher) range for-loop variable not used
    GENERATE_UNUSED_RANGE_FOR_LOOP_VAR(arr, var_a);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
input_unusedforloopvar.cpp:26:19: note: expanded from macro 'GENERATE_UNUSED_RANGE_FOR_LOOP_VAR'
        for (auto x : arr)\
             ~~~~~^~
input_unusedforloopvar.cpp:45:5: warning: (Recursive AST Visitor) regular for-loop variable not used
    GENERATE_UNUSED_REGULAR_FOR_LOOP_VAR(3, var_a);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
input_unusedforloopvar.cpp:39:18: note: expanded from macro 'GENERATE_UNUSED_REGULAR_FOR_LOOP_VAR'
        for (int x = 0; x < size; ++x)\
             ~~~~^~~~

[ivafanas]

It would be nice if there is a way to rerun checks.

Failure is a CI problem:

E: Failed to fetch https://packages.microsoft.com/ubuntu/18.04/prod/dists/bionic/main/binary-amd64/Packages.bz2  File has unexpected size (154407 != 153334). Mirror sync in progress? [IP: 13.90.56.68 443]

[ivafanas]

Hmm, force-push of the same commit retriggered checks but ms servers are still in sync. Need to wait.

[banach-space]

Thank you for submitting this!

I'm a bit confused - your tests pass for me even without your modifications. Is that not the case for you?

Could you try playing with -ast-dump for the tests that you provided and e.g. CodeStyleCheckerMacro.cpp? In the first case, all location are resolved to the source file. For CodeStyleCheckerMacro.cpp some locations are reported as <scratch space>, which is why a fix was required.

Either way, these are very good tests!

[ivafanas]

Hi!

They shouldn't. I will have a look on it.

[ivafanas]

Hm, for me:

• UnusedForLoopVar_range_loop_macro.cpp failed without my changes
• UnusedForLoopVar_regular_loop_macro.cpp passed without my changes

I'm going to investigate why regular loop works but not today. It is a deep night now.

[banach-space]

Thanks for checking, I confirm that UnusedForLoopVar_range_loop_macro.cpp fails without your change. I haven't tested with your change just yet (but I suspect that it will pass). There are no <scratch space>s in the AST dump though - weird.

Also, no rush with this!

[ivafanas]

Well, the answer was easy.

UnusedForLoopVarASTConsumer::HandleTranslationUnit enumerates declarations in main translation unit to traverse them recursively. In our case it checks function declaration against the main translation unit and search for loops with unused variables inside function.

In order to reproduce the bug for RecursiveASTVisitor function declaration should be generated by preprocessor.

UnusedForLoopVar_regular_loop_function_macro.cpp test is added. It should fail without fix and pass when patch is applied.

[banach-space]

The answer is always easy once you know it :) Great catch, thank you!

So, UnusedForLoopVar_regular_loop_macro.cpp will pass even without this patch. This is fine, but IMO deserves a comment in the commit message. Otherwise I know that my future self will assume that all 3 tests would fail before and pass after merging this patch.

Would you mind expanding the commit message? I'll merge it then!

[ivafanas]

Commit message is extended.

Done