The Chromium browser is developing very fast. When we checked the solution for the first time in 2011, it included 473 projects. Now it includes 1169 projects. We were curious to know if Google developers had managed to keep the highest quality of their code with Chromium developing at such a fast rate. Well, they had.
Chromium is an open-source web browser developed by the Google company. It is used as a basis for the Google Chrome browser. Visit the "Get the Code" page for information on downloading the solution source codes.
We checked Chromium earlier and reported those checks in two articles: the first check (23.05.2011), the second check (13.10.2011). Each time we managed to find a number of errors - this is a subtle hint about the usefulness of code analyzers.
Currently (we downloaded the source codes in July 2013) Chromium consists of 1169 projects. The total size of the C/C++ source code is 260 Mbytes plus 450 Mbytes more of the third-party libraries.
If you study our first analysis-report for Chromium of 2011, you will notice that the size of the third-party libraries has not changed much since that. But the size of the project code itself has grown from 155 Mbytes to 260 Mbytes.
The PVS-Studio analyzer is capable of searching for functions with big cyclomatic complexity values. These functions are usually the first candidates to be refactored. Having checked 1160 projects, I was naturally curious to find out which of them held the record for having "the most complex function".
In Chromium, the function ValidateChunkAMD64() has the highest cyclomatic complexity (2782). However, we had to disqualify it from the contest because it is located in the validator_x86_64.c file which is an autogenerated one. That's a pity: it could have been an epic record-holder. I have never seen such a large cyclomatic complexity value in my life.
Thus, the first three places go to the following functions:
If you have never come across cyclomatic complexity of 1000, you'd better never have to for your psychic health's sake :). It's just too much, you know.
What can be said about the quality of the Chromium project's code? It is perfect as always. There are some bugs indeed, just as in any other large project; but if you calculate their density (by dividing their number by the code size) you'll see that it is very trifling. This is a very good code with pretty few bugs. So, we award a medal to the Chromium developers for their clear code. The previous medal was awarded to the Casablanca (C++ REST SDK) project by Microsoft.
Figure 1. A medal for the Chromium developers.
Along with the project code itself, I also checked the third-party libraries used by Chromium. However, describing errors found in them is not very interesting, especially considering that I just glanced through the report very quickly. You may think I'm a mean guy, but I'm not. I'd like to watch you studying carefully the analysis report for all the 1169 projects. The bugs I did notice were added into our bug database. This article describes only those errors that I found in Chromium itself (its plugins and so on).
The Chromium project being so perfect, what for should I describe its bugs at all? It's simple: I want to show you how powerful the PVS-Studio analyzer is. Since it has managed to catch some bugs in Chromium with its fine code, it is surely worth your attention.
The analyzer chewed up dozens of thousands of files with the total size 710 Mbytes and still survived. Although the Chromium project is being developed by highly skilled developers and checked by various verifying tools, PVS-Studio still managed to catch some defects. And that's an awesome achievement! And the last thing: it took it a reasonable time (about 5 hours) to complete the analysis, as the check ran in parallel (AMD FX-8320/3.50 GHz/eight-core processor, 16.0 GB RAM).
I invite you to study selected code samples that caught my glance when looking through the analysis report. I'm sure that a more thorough examination will have much more interesting results.
Vector3dF
Matrix3F::SolveEigenproblem(Matrix3F* eigenvectors) const
{
// The matrix must be symmetric.
const float epsilon = std::numeric_limits<float>::epsilon();
if (std::abs(data_[M01] - data_[M10]) > epsilon ||
std::abs(data_[M02] - data_[M02]) > epsilon ||
std::abs(data_[M12] - data_[M21]) > epsilon) {
NOTREACHED();
return Vector3dF();
}
....
}V501 There are identical sub-expressions to the left and to the right of the '-' operator: data_[M02] - data_[M02] matrix3_f.cc 128
We need to check that a 3x3 matrix is symmetric.
Figure 2. 3x3 matrix.
To do that we should compare the following items:
The code must have been written with the use of the Copy-Paste technology, which resulted in M02 cell being compared to itself. That's a funny matrix class.
Another plain misprint:
bool IsTextField(const FormFieldData& field) {
return
field.form_control_type == "text" ||
field.form_control_type == "search" ||
field.form_control_type == "tel" ||
field.form_control_type == "url" ||
field.form_control_type == "email" ||
field.form_control_type == "text";
}V501 There are identical sub-expressions 'field.form_control_type == "text"' to the left and to the right of the '||' operator. autocomplete_history_manager.cc 35
A comparison to the "text" string is executed twice, which is strange. One of these lines is not necessary or there must be some other comparison instead.
static void ParseRequestCookieLine(
const std::string& header_value,
ParsedRequestCookies* parsed_cookies)
{
std::string::const_iterator i = header_value.begin();
....
if (*i == '"') {
while (i != header_value.end() && *i != '"') ++i;
....
}V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 500, 501. web_request_api_helpers.cc 500
I guess this code was intended to skip a text framed by double quotes. But it actually does nothing, as the condition is false right away. I've written a small pseudo-code fragment to clarify the point:
if ( A == 'X' ) {
while ( .... && A != 'X' ) ....;The programmer must have forgotten to move the pointer by one character, so the fixed code should look like this:
if (*i == '"') {
++i;
while (i != header_value.end() && *i != '"') ++i;void ShortcutsProvider::DeleteMatchesWithURLs(
const std::set<GURL>& urls)
{
std::remove_if(matches_.begin(),
matches_.end(),
RemoveMatchPredicate(urls));
listener_->OnProviderUpdate(true);
}V530 The return value of function 'remove_if' is required to be utilized. shortcuts_provider.cc 136
To remove items from the container the function std::remove_if() is used, yet it is used incorrectly. The function remove_if() actually removes nothing; it only shifts items to the beginning and moves the iterator back to garbage which you need to remove manually by calling the erase() function of the container. See also the Wikipedia-article "Erase-remove idiom" for details.
The fixed code:
matches_.erase(std::remove_if(.....), matches_.end());SOCKET in the Linux world is an integer SIGNED data type.
SOCKET in the Windows world is an integer UNSIGNED data type.
In Visual C++ header files, the SOCKET type is declared in this way:
typedef UINT_PTR SOCKET;But programmers are constantly forgetting this and keep writing code like this:
class NET_EXPORT_PRIVATE TCPServerSocketWin {
....
SOCKET socket_;
....
};
int TCPServerSocketWin::Listen(....) {
....
socket_ = socket(address.GetSockAddrFamily(),
SOCK_STREAM, IPPROTO_TCP);
if (socket_ < 0) {
PLOG(ERROR) << "socket() returned an error";
return MapSystemError(WSAGetLastError());
}
....
}V547 Expression 'socket_ < 0' is always false. Unsigned type value is never < 0. tcp_server_socket_win.cc 48
An unsigned variable is always above or equal to zero. It means that the 'socket_ < 0' check is meaningless. If the socket fails to be opened while the program is running, this situation will be handled incorrectly.
enum FontStyle {
NORMAL = 0,
BOLD = 1,
ITALIC = 2,
UNDERLINE = 4,
};
void LabelButton::SetIsDefault(bool is_default) {
....
style = is_default ? style | gfx::Font::BOLD :
style & !gfx::Font::BOLD;
....
}V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. label_button.cc 131
I guess the code was intended to work in the following way:
The expression "style & !gfx::Font::BOLD", however, works quite differently than the programmer expects. The result of the "!gfx::Font::BOLD" operation will be 'false', i.e. 0. The code above is equivalent to this one:
style = is_default ? style | gfx::Font::BOLD : 0;For it to work correctly the '~' operation must be used:
style = is_default ? style | gfx::Font::BOLD :
style & ~gfx::Font::BOLD;base::win::ScopedComPtr<IDirect3DSurface9>
scaler_scratch_surfaces_[2];
bool AcceleratedSurfaceTransformer::ResizeBilinear(
IDirect3DSurface9* src_surface, ....)
{
....
IDirect3DSurface9* read_buffer = (i == 0) ?
src_surface : scaler_scratch_surfaces_[read_buffer_index];
....
}V623 Consider inspecting the '?:' operator. A temporary object of the 'ScopedComPtr' type is being created and subsequently destroyed. Check second operand. accelerated_surface_transformer_win.cc 391
This code will hardly cause any bugs, but it is worth discussing: I suppose some programmers will discover a new C++ trap they will find interesting.
It's all simple at first sight: depending on the condition, either the 'src_surface' pointer or one of the 'scaler_scratch_surfaces_' array's items is chosen. The array is comprised by objects of the base::win::ScopedComPtr<IDirect3DSurface9> type which can be automatically cast to the pointer to IDirect3DSurface9.
The devil is in the details.
The ternary operator '?:' cannot return different types depending on the conditions. Here is a simple example to explain the point.
int A = 1;
auto X = v ? A : 2.0;The ?: operator returns the 'double' type. Because of that, the 'X' variable will also be double. But it's not the point. The point is that the 'A' variable will be implicitly extended to the 'double' type!
The trouble occurs if you write a thing like this:
CString s1(L"1");
wchar_t s2[] = L"2";
bool a = false;
const wchar_t *s = a ? s1 : s2;Execution of this code fragment results in the 's' variable referring to data inside a temporary object of the CString type. The problem is that this object will be immediately destroyed.
Now let's go back to Chromium's source code.
IDirect3DSurface9* read_buffer = (i == 0) ?
src_surface : scaler_scratch_surfaces_[read_buffer_index];If the 'i == 0' condition is true, the next thing occurs:
I'm not familiar with the logic of the program and the ScopedComPtr class and I can't tell for sure if any negative consequences will occur. The most probable thing is that the counter of the reference number will be incremented in the constructor and decremented in the destructor. So, everything will be OK.
If not, you risk getting a non-valid pointer or broken reference counter.
So, even if there is no error in this particular case, I will be glad if anyone of the readers has learned something new. As you can see, ternary operators are much more dangerous than one may think.
Here is one more suspicious fragment like the previous one:
typedef
GenericScopedHandle<HandleTraits, VerifierTraits> ScopedHandle;
DWORD HandlePolicy::DuplicateHandleProxyAction(....)
{
....
base::win::ScopedHandle remote_target_process;
....
HANDLE target_process =
remote_target_process.IsValid() ?
remote_target_process : ::GetCurrentProcess();
....
}V623 Consider inspecting the '?:' operator. A temporary object of the 'GenericScopedHandle' type is being created and subsequently destroyed. Check third operand. handle_policy.cc 81
string16 GetAccessString(HandleType handle_type,
ACCESS_MASK access) {
....
if (access & FILE_WRITE_ATTRIBUTES)
output.append(ASCIIToUTF16("\tFILE_WRITE_ATTRIBUTES\n"));
if (access & FILE_WRITE_DATA)
output.append(ASCIIToUTF16("\tFILE_WRITE_DATA\n"));
if (access & FILE_WRITE_EA)
output.append(ASCIIToUTF16("\tFILE_WRITE_EA\n"));
if (access & FILE_WRITE_EA)
output.append(ASCIIToUTF16("\tFILE_WRITE_EA\n"));
....
}V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 176, 178. handle_enumerator_win.cc 178
If the flag FILE_WRITE_EA is enabled, the string "\tFILE_WRITE_EA\n" will be added twice. That's very strange.
A similar strange thing happens in the following fragment as well:
static bool PasswordFormComparator(const PasswordForm& pf1,
const PasswordForm& pf2) {
if (pf1.submit_element < pf2.submit_element)
return true;
if (pf1.username_element < pf2.username_element)
return true;
if (pf1.username_value < pf2.username_value)
return true;
if (pf1.username_value < pf2.username_value)
return true;
if (pf1.password_element < pf2.password_element)
return true;
if (pf1.password_value < pf2.password_value)
return true;
return false;
}V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 259, 261. profile_sync_service_password_unittest.cc 261
The check "pf1.username_value < pf2.username_value" is executed twice. Perhaps one string is just not needed, but it's also probable that the programmer wanted to check something else and some different condition is missing here.
ResourceProvider::ResourceId
PictureLayerImpl::ContentsResourceId() const
{
....
for (PictureLayerTilingSet::CoverageIterator iter(....);
iter;
++iter)
{
if (!*iter)
return 0;
const ManagedTileState::TileVersion& tile_version = ....;
if (....)
return 0;
if (iter.geometry_rect() != content_rect)
return 0;
return tile_version.get_resource_id();
}
return 0;
}V612 An unconditional 'return' within a loop. picture_layer_impl.cc 638
Something is not right with this loop: it iterates only once. There is the unconditional operator return at the end of the loop, which may be due to the following reasons:
There are some other strange loops iterating only once:
scoped_ptr<ActionInfo> ActionInfo::Load(....)
{
....
for (base::ListValue::const_iterator iter = icons->begin();
iter != icons->end(); ++iter)
{
std::string path;
if (....);
return scoped_ptr<ActionInfo>();
}
result->default_icon.Add(....);
break;
}
....
}V612 An unconditional 'break' within a loop. action_info.cc 76
const BluetoothServiceRecord* BluetoothDeviceWin::GetServiceRecord(
const std::string& uuid) const
{
for (ServiceRecordList::const_iterator iter =
service_record_list_.begin();
iter != service_record_list_.end();
++iter)
{
return *iter;
}
return NULL;
}V612 An unconditional 'return' within a loop. bluetooth_device_win.cc 224
HRESULT IEEventSink::Attach(IWebBrowser2* browser) {
DCHECK(browser);
HRESULT result;
if (browser) {
web_browser2_ = browser;
FindIEProcessId();
result = DispEventAdvise(web_browser2_, &DIID_DWebBrowserEvents2);
}
return result;
}V614 Potentially uninitialized variable 'result' used. ie_event_sink.cc 240
If the pointer 'browser' equals zero, the function will return an uninitialized variable.
Another code fragment:
void SavePackage::GetSaveInfo() {
....
bool skip_dir_check;
....
if (....) {
....->GetSaveDir(...., &skip_dir_check);
}
....
BrowserThread::PostTask(BrowserThread::FILE,
FROM_HERE,
base::Bind(..., skip_dir_check, ...));
}V614 Potentially uninitialized variable 'skip_dir_check' used. Consider checking the fifth actual argument of the 'Bind' function. save_package.cc 1326
The variable 'skip_dir_check' may remain uninitialized.
void OnTraceNotification(int notification) {
if (notification & TraceLog::EVENT_WATCH_NOTIFICATION)
++event_watch_notification_;
notifications_received_ |= notification;
}V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. trace_event_unittest.cc 57
When examining this code, you cannot understand whether or not curly brackets are missing here. Even if it is correct, it should be changed a bit so that it doesn't confuse other programmers.
Here is a couple of fragments with a TOO strange code alignment:
Many programs contain legacy code written in those old times when the 'new' operator did not throw an exception in case of memory shortage. It used to return a null pointer instead.
Chromium is no exception in that aspect - it also has such checks. The trouble is not that these checks are meaningless but that returning a null pointer implied performing some actions or returning certain values by functions. Now the program logic is different because of the practice of exception generation: the code that was given control in case of a memory allocation error now stays idle.
Have a look at this example:
static base::DictionaryValue* GetDictValueStats(
const webrtc::StatsReport& report)
{
....
DictionaryValue* dict = new base::DictionaryValue();
if (!dict)
return NULL;
dict->SetDouble("timestamp", report.timestamp);
base::ListValue* values = new base::ListValue();
if (!values) {
delete dict;
return NULL;
}
....
}V668 There is no sense in testing the 'dict' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 164
V668 There is no sense in testing the 'values' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. peer_connection_tracker.cc 169
The first check "if (!dict) return NULL;" doesn't seem harmful. But the second check is not safe. If memory fails to be allocated when the "new base::ListValue()" function creates an object, the exception 'std::bad_alloc' will be thrown and the GetDictValueStats() function will terminate.
As a result, this code:
if (!values) {
delete dict;
return NULL;
}will never destroy the object whose address is stored in the 'dict' variable.
To fix the code we need to refactor it and use smart pointers.
Examine another code fragment:
bool Target::Init() {
{
....
ctx_ = new uint8_t[abi_->GetContextSize()];
if (NULL == ctx_) {
Destroy();
return false;
}
....
}V668 There is no sense in testing the 'ctx_' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. target.cc 73
In case of a memory allocation error, the function Destroy() won't be called.
I find this type of bugs not very much interesting to write about any further. Instead, I give you a list of other fragments of that kind I've noticed:
Unit tests are a wonderful method of software quality enhancement. But tests themselves often have errors, which results in their failure. Making tests for tests is just too much; so, static code analysis will be of use in these cases. I discussed this idea in more detail in the article "How to complement TDD with static analysis".
Below are some examples of errors I have found in tests for Chromium:
std::string TestAudioConfig::TestValidConfigs() {
....
static const uint32_t kRequestFrameCounts[] = {
PP_AUDIOMINSAMPLEFRAMECOUNT,
PP_AUDIOMAXSAMPLEFRAMECOUNT,
1024,
2048,
4096
};
....
for (size_t j = 0;
j < sizeof(kRequestFrameCounts)/sizeof(kRequestFrameCounts);
j++) {
....
}V501 There are identical sub-expressions 'sizeof (kRequestFrameCounts)' to the left and to the right of the '/' operator. test_audio_config.cc 56
Only one test is executed in the loop. The error is this: "sizeof(kRequestFrameCounts)/sizeof(kRequestFrameCounts)" equals one. The correct expression is "sizeof(kRequestFrameCounts)/sizeof(kRequestFrameCounts[0])".
Another incorrect test:
void DiskCacheEntryTest::ExternalSyncIOBackground(....) {
....
scoped_refptr<net::IOBuffer> buffer1(new net::IOBuffer(kSize1));
scoped_refptr<net::IOBuffer> buffer2(new net::IOBuffer(kSize2));
....
EXPECT_EQ(0, memcmp(buffer2->data(), buffer2->data(), 10000));
....
}V549 The first argument of 'memcmp' function is equal to the second argument. entry_unittest.cc 393
The function "memcmp()" compares the buffer to itself. This results in the test failing to perform the necessary check. I guess the code should look like this:
EXPECT_EQ(0, memcmp(buffer1->data(), buffer2->data(), 10000));And the next test is the one that may unexpectedly spoil the other tests:
static const int kNumPainters = 3;
static const struct {
const char* name;
GPUPainter* painter;
} painters[] = {
{ "CPU CSC + GPU Render", new CPUColorPainter() },
{ "GPU CSC/Render", new GPUColorWithLuminancePainter() },
};
int main(int argc, char** argv) {
....
// Run GPU painter tests.
for (int i = 0; i < kNumPainters; i++) {
scoped_ptr<GPUPainter> painter(painters[i].painter);
....
}V557 Array overrun is possible. The value of 'i' index could reach 2. shader_bench.cc 152
The 'painters' array perhaps used to consist of three items earlier. Now it has only two, but the value of the 'kNumPainters' constant is still 3.
Here is a list of some other incorrect code fragments in tests which I find worth considering:
V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1790
V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1800
V579 The string function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. syncable_unittest.cc 1810
V595 The 'browser' pointer was utilized before it was verified against nullptr. Check lines: 5489, 5493. testing_automation_provider.cc 5489
V595 The 'waiting_for_.get()' pointer was utilized before it was verified against nullptr. Check lines: 205, 222. downloads_api_unittest.cc 205
V595 The 'pNPWindow' pointer was utilized before it was verified against nullptr. Check lines: 34, 35. plugin_windowed_test.cc 34
V595 The 'pNPWindow' pointer was utilized before it was verified against nullptr. Check lines: 16, 20. plugin_window_size_test.cc 16
V595 The 'textfield_view_' pointer was utilized before it was verified against nullptr. Check lines: 182, 191. native_textfield_views_unittest.cc 182
V595 The 'message_loop_' pointer was utilized before it was verified against nullptr. Check lines: 53, 55. test_flash_message_loop.cc 53
In any program many defects are found in code fragments responsible for handling errors and reacting to incorrect input data. This is due to the fact that these fragments are difficult to test, so they are usually not. Because of this, programs start behaving in a very intricate way, which was not planned by the programmer.
For example:
DWORD GetLastError(VOID);
void TryOpenFile(wchar_t *path, FILE *output) {
wchar_t path_expanded[MAX_PATH] = {0};
DWORD size = ::ExpandEnvironmentStrings(
path, path_expanded, MAX_PATH - 1);
if (!size) {
fprintf(output,
"[ERROR] Cannot expand \"%S\". Error %S.\r\n",
path, ::GetLastError());
}
....
}V576 Incorrect format. Consider checking the fourth actual argument of the 'fprintf' function. The pointer to string of wchar_t type symbols is expected. fs.cc 17
If the variable 'size' equals zero, the program tries to write a text message into a file. But this message will most probably contain some abracadabra at the end. Moreover, this code may cause an access violation.
Writing is performed by the function fprintf() which cannot control the types of its arguments. It expects that the last argument should be a pointer to a string. But the actual argument is a number (error code) which will be converted into an address, and no one can predict how the program will behave after that.
I remind you once again that I only looked through the list of warning messages and described in this article only what caught my attention. What's more, I've shown just a part of what I've found. If I described all those bugs, the article would become too big. And it's already big enough.
I decided not to mention many code fragments which I found of no interest to the readers. Here you are a couple of examples to explain what I mean.
bool ManagedUserService::UserMayLoad(
const extensions::Extension* extension,
string16* error) const
{
if (extension_service &&
extension_service->GetInstalledExtension(extension->id()))
return true;
if (extension) {
bool was_installed_by_default =
extension->was_installed_by_default();
.....
}
}V595 The 'extension' pointer was utilized before it was verified against nullptr. Check lines: 277, 280. managed_user_service.cc 277
The pointer 'extension' gets dereferenced in the "extension->id()" expression in the beginning. After that it is being checked for being a null pointer.
Such code fragments are usually harmless, for the pointer simply cannot be equal to zero, so the check is meaningless. That's why I find it unreasonable to mention these fragments because I may be mistaken and confuse a correct code for an incorrect one.
This is one more example of a diagnostic I preferred not to notice:
bool WebMClusterParser::ParseBlock(....)
{
int timecode = buf[1] << 8 | buf[2];
....
if (timecode & 0x8000)
timecode |= (-1 << 16);
....
}V610 Undefined behavior. Check the shift operator '<<. The left operand '-1' is negative. webm_cluster_parser.cc 217
Formally, a shift of a negative value leads to undefined behavior. But many compilers work stably and behave the way the programmer expects them to. It results in the code working well and long, though it shouldn't. I don't feel like fighting these troubles now, so I'll better skip such messages. Those of you who want to study the issue in detail, see the article "Wade not in unknown waters - part three".
I am often asked the question:
You do it very cleverly citing samples of detected bugs in your articles without telling the total number of warning messages produced by the tool. It's a usual thing with static analyzers to generate too many false positives so that one has hard time trying to pick out real errors among them. How many false positives does PVS-Studio generate?
I always hesitate to answer. You see, I have two opposite answers: the first is "many", the second is "few". It all depends on how you approach to viewing the list of warning messages. I will try to explain this duality by the example of Chromium.
The PVS-Studio analyzer has generated 3582 first-level warnings (the set of General Analysis rules) for this project. This number is very big. Most of them are false positives. If you attack them frontally and start examining each item of the list, you'll get bored very soon. Your impression of the tool will be awful: almost all the warnings are false positives looking very alike. Nothing interesting. The tool is bad.
The typical mistake of a user who thinks like that is that he has not performed even minimum customization of the tool. Yes, we try to make PVS-Studio such a tool that can be efficiently run immediately after the setup. We try to make it in such a way that you don't have to customize anything; you just check your project and study the list of warnings.
But sometimes it can't be done. That was the case with Chromium. It was the macro 'DVLOG' that triggered so many false positives. This macro is responsible for logging something and is written in such a tricky way that PVS-Studio believes it to have a bug. Since the macro is used very often in the program, I got quite many false positives. In fact, the number of false warnings in the analysis report coincides with the number of times the DVLOG macro is used; namely, it triggered about 2300 false positives under the "V501 There are identical sub-expressions....." diagnostic.
You can suppress these warnings by adding the comment //-V:DVLOG:501 in the header file opposite to the macro declaration.
This simple operation lets us subtract 2300 false positives from the total number of messages (3528). We have at one instant eliminated 65% of messages. Now we don't have to waste time examining them.
It won't take you much time and effort to perform some more subtle customizations like that. As a result, most of the false positives will be removed from the report. Customizing some diagnostics requires analysis relaunch, while others don't. All this is described in detail in the documentation section "Suppression of false alarms". In particular, analysis relaunch is required for diagnostics detecting errors in macros.
I hope you now understand why I have two opposite answers. It all depends on whether or not the programmer is ready to spend just a bit of his time studying the product and ways to get rid of irrelevant messages.
Taking the opportunity, I want to send my best regards to my parents... Oh, sorry, I meant: taking the opportunity, I want to send my best regards to programmers and remind them a few things: