| Author |
 Topic  |
|
|
jakeslade
Junior Member
United Kingdom
14 Posts |
 Posted - Oct 01 2019 : 07:54:53 AM
|
Hello,
I've been considering a purhase of VAX license. I read about Clang integration and this made me excited to try out Code Inspection features.
The result is disappointing. It doesn't pick up anything other than the most basic of Clang's suggestions.
For example, it doesn't even suggest range based for loop improvements in the most basic of circumstances.
The following example would be picked up by Clang, but it isn't picked up by VAX's Code Inspection:
https://pastebin.com/txbfNSGM
Is there something I need to configure for it to work properly? If not, is this feature likely to be improved?
Cheers |
Edited by - jakeslade on Oct 01 2019 07:58:01 AM |
|
|
feline
Whole Tomato Software
United Kingdom
19014 Posts |
Posted - Oct 01 2019 : 10:35:17 AM
|
Which IDE and version of VA are you using?
Can you please try adding the following code to one of your cpp files, and see if Code Inspection detects this loop, and suggests updating it? It does so for me, using VS2017 and VA 2341.2:
static int forLoopAcrossConst(const std::map<std::string, short> &mapNamesToNumbers)
{
int nTotal;
for (auto mapIt = mapNamesToNumbers.begin(); mapIt != mapNamesToNumbers.end(); ++mapIt)
{
nTotal += mapIt->second;
}
}
looking at your loop, you have a comma inside the for statement, along with 2 semi-colons. Is this deliberate?
From the names used I can see what the idea is here, but I don't recognise this syntax - *edit* you are declaring two variables here, of the same type, separated by a comma. I just don't remember seeing this done before in this context, but it makes sense once I recognise it. |
zen is the art of being at one with the two'ness |
Edited by - feline on Oct 01 2019 10:39:09 AM |
 |
|
|
jakeslade
Junior Member
United Kingdom
14 Posts |
Posted - Oct 01 2019 : 12:29:36 PM
|
Interesting, it did pick that up and suggest updating.
I changed to auto to explicit iterator type and it no longer picks it up:
static int forLoopAcrossConst(const std::map<std::string, short> &mapNamesToNumbers)
{
int nTotal;
for (std::map<std::string, short>::iterator mapIt = mapNamesToNumbers.begin(); mapIt != mapNamesToNumbers.end(); ++mapIt)
{
nTotal += mapIt->second;
}
}
I'm using VS2015 Pro and VA 2341.2
Appreciate the swift reply! |
Edited by - jakeslade on Oct 01 2019 12:30:02 PM |
|
|
|
feline
Whole Tomato Software
United Kingdom
19014 Posts |
Posted - Oct 01 2019 : 1:01:16 PM
|
| Does this version of the for loop compile for you? It is failing to compile for me, still in VS2017, even though on the surface it looks quite reasonable... if the code is invalid, its not a surprise that Code Inspection is not suggesting anything. |
zen is the art of being at one with the two'ness |
|
|
|
sean
Whole Tomato Software
USA
2817 Posts |
Posted - Oct 01 2019 : 1:34:56 PM
|
Odd -- the inspection doesn't light up until the body of the for loop uses the start variable:
int x = *start;
|
Edited by - sean on Oct 01 2019 1:37:25 PM |
|
|
|
feline
Whole Tomato Software
United Kingdom
19014 Posts |
Posted - Oct 04 2019 : 6:05:59 PM
|
I am now organised to test Clang directly, to compare what VA is doing to what Clang does. Your original code is:
#include <vector>
int main()
{
std::vector<int> v = {1, 2, 3};
for(std::vector<int>::const_iterator start = v.begin(), end = v.end(); start != end; ++start)
{
}
int *pInt = 0;
}
I have tested this code, using the command:
clang-tidy code_test.cpp -checks=-*,modernize-* --
on both Ubuntu Linux with clang-tidy version 6.0.0 and on Windows 10 with clang-tidy version 9.0.0, and neither test reports that the for loop can be changed. Have you actually tested this code with clang-tidy, and if so, which version, and what command line switches did you use? |
zen is the art of being at one with the two'ness |
|
|
|
feline
Whole Tomato Software
United Kingdom
19014 Posts |
Posted - Oct 05 2019 : 09:22:24 AM
|
For the other loop, VA is not picking up that the for loop can be converted with the code:
static int forLoopAcrossConst(const std::map<std::string, short> &mapNamesToNumbers)
{
int nTotal;
for (std::map<std::string, short>::iterator mapIt = mapNamesToNumbers.begin(); mapIt != mapNamesToNumbers.end(); ++mapIt)
{
nTotal += mapIt->second;
}
}
but this contains an error, it won't compile. You need to change the "iterator" into a "const_iterator", obvious when you spot it, since the map parameter is a const parameter. This gives you the loop:
static int forLoopAcrossConst(const std::map<std::string, short> &mapNamesToNumbers)
{
int nTotal;
for (std::map<std::string, short>::const_iterator mapIt = mapNamesToNumbers.begin(); mapIt != mapNamesToNumbers.end(); ++mapIt)
{
nTotal += mapIt->second;
}
}
and VA Code Inspection is picking up that this loop can be converted quite happily, and the fix works correctly when applied. |
zen is the art of being at one with the two'ness |
|
|
| |
Topic |
|
Code Inspection
