Our website uses cookies to enhance your browsing experience.
Accept
to the top
close form

Fill out the form in 2 simple steps below:

Your contact information:

Step 1
Congratulations! This is your promo code!

Desired license type:

Step 2
Team license
Enterprise license
** By clicking this button you agree to our Privacy Policy statement
close form
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
* By clicking this button you agree to our Privacy Policy statement

close form
Free PVS‑Studio license for Microsoft MVP specialists
* By clicking this button you agree to our Privacy Policy statement

close form
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

close form
I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

close form
check circle
Message submitted.

Your message has been sent. We will email you at


If you do not see the email in your inbox, please check if it is filtered to one of the following folders:

  • Promotion
  • Updates
  • Spam

>
>
>
MuditaOS: Will your alarm clock go off?…

MuditaOS: Will your alarm clock go off? Part II

Feb 21 2022

This is the second part in a series of articles checking the MuditaOS operating system. In this article, we cover the bottlenecks of the project that are worth refactoring. The PVS-Studio static analyzer and its optimization warnings will help us with this.

0925_MuditaOS_2/image1.png

Introduction

Recently, at one of the websites, where we posted the "Top 10 bugs found in C++ projects in 2021" article, someone asked the following questions in the comments:

How is PVS-Studio dealing with modern C++? Does the analyzer know how to detect incorrect use of move semantics and other insidious things of new C++ standards?

At that moment, I had an idea to write a separate article on micro-optimization diagnostics rules. Among these diagnostics, there are a lot of those that work with language constructs from the new language standards.

I note that the rules are called micro-optimizational for a reason. If you fix a couple of micro-optimization warnings, most often you won't get a noticeable performance gain. No one guarantees a crucial change in performance though. However, if you approach the issue comprehensively, you can often achieve significant improvements in project performance.

The most efficient way to increase performance is to use PVS-Studio together with some profiler. The static analyzer does not know how often the code fragment will be used, but simply says that some fragments should be rewritten in a more optimal way. It is the profiler that allows you to identify the most used code fragments. The method is as follows: to combine the output of both tools and, first of all, to fix the warnings of the static analyzer in the places pointed by the profiler.

In this article I will describe a lot of analyzer warnings. When evaluating them, I suggest taking a wider perspective and thinking about each of them as a small edit on the scale of a large code refactoring.

Since the project is regularly updated, to check it, I froze it in version 8cc1f77. So, without further delay, let's see what we managed to find!

Analyzer Warnings

Move semantics

V833 Passing the const-qualified object 'fileIndexerAudioPaths' to the 'std::move' function disables move semantics. BellHybridMain.cpp 77

int main()
{
  const std::vector<std::string> fileIndexerAudioPaths = ....
  ....
  std::vector<std::unique_ptr<sys::BaseServiceCreator>> systemServices;
  ....
  systemServices.emplace_back(sys::CreatorFor<
    service::ServiceFileIndexer>(std::move(fileIndexerAudioPaths)));
  ....
}

Let's start with the diagnostic rule that we implemented in PVS-Studio 7.16. This rule says that a developer is trying to apply std::move to a constant object.

The code does not work as the developer expects: a constant object is not moved, because the std::move function does not actually move the object and does not guarantee that the object will move. With the use of static_cast, the std::move function simply casts the passed argument to the T&& type. Roughly speaking, when you call std::move, you are requesting a move, not directly telling the compiler to move the object. If want to know more details, we invite you to check the corresponding section of the knowledge base on our website — "move semantics".

In this case, the move will not be performed because we cannot modify the constant object. To fix this code, you can remove the 'const' keyword from the local variable:

int main()
{
  std::vector<std::string> fileIndexerAudioPaths = ....
  ....
  std::vector<std::unique_ptr<sys::BaseServiceCreator>> systemServices;
  ....
  systemServices.push_back(sys::CreatorFor<
             service::ServiceFileIndexer>(std::move(fileIndexerAudioPaths)));
  ....
}

Or, if the 'const' keyword is important, it makes sense to remove the redundant std::move call:

int main()
{
  const std::vector<std::string> fileIndexerAudioPaths = ....
  ....
  std::vector<std::unique_ptr<sys::BaseServiceCreator>> systemServices;
  ....
  systemServices.push_back(sys::CreatorFor<
                      service::ServiceFileIndexer>(fileIndexerAudioPaths));
  ....
}

Also, as you may have noticed, we replaced the emplace_back function with push_back in the fixed code fragment. And we did it for reasons. The first reason is that emplace_back is a variadic function template in the std::vector class template. The compiler needs to additionally instantiate the function based on the arguments passed. More instantiations means to spend more time building the project. The second reason is that push_back is a function with two overloads in the std::vector class template.

But what about the special street magic of the emplace_back function, that allows you to create an object immediately in the vector? No magic here. In both cases, the container will request memory from the allocator to place the object. After that, the container will call the move constructor. You can find more information on this topic here.

The analyzer issued quite a few V833 warnings for the MuditaOS project. This is a new diagnostic rule, and I really like it, so let me show you a few more warnings:

V833 Passing the const-qualified object 'text' to the 'std::move' function disables move semantics. OptionBellMenu.hpp 30

class OptionBellMenu
{
public:
  OptionBellMenu(const UTF8 &text, ....)
    : text(std::move(text))
    , ....
  {
  
  }
  ....
private:
  UTF8 text;
  ....
}

V833 Passing the const-qualified object 'blocks' to the 'std::move' function disables move semantics. TextDocument.cpp 13

class TextDocument
{
  ....
  std::list<TextBlock> blocks;
  ....
}
....
TextDocument::TextDocument(const std::list<TextBlock> &blocks) 
  : blocks(std::move(blocks))
{
  
}

The approach to fix these errors is similar to the way we fixed the first error. So I don't see any point in repeating myself. In total, the analyzer found about 20 V833 warnings in the project.

Now let's look at another warning related to move semantics:

V820 The 'snoozed' variable is not used after copying. Copying can be replaced with move/swap for optimization. AlarmPresenter.cpp 27

void AlarmPopupContract::AlarmModel::setSnoozed(
  std::vector<SingleEventRecord> snoozed)
{
  this->snoozedRecord = snoozed;
}

The analyzer has detected code fragment where a variable is copied to another variable but is never used after that. Such code can be optimized by removing the unnecessary copy operation. For example, use the std::move function:

void AlarmPopupContract::AlarmModel::setSnoozed(
  std::vector<SingleEventRecord> snoozed)
{
  this->snoozedRecord = std::move(snoozed);
}

In total, the analyzer issued around 40 warnings of this diagnostic. Here are a few of them:

  • V833 Passing the const-qualified object 'result->snoozedAlarms' to the 'std::move' function disables move semantics. ActiveNotificationsModel.cpp 213
  • V833 Passing the const-qualified object 'scheme' to the 'std::move' function disables move semantics. ColorTestWindow.cpp 79
  • V833 Passing the const-qualified object 'text' to the 'std::move' function disables move semantics. OptionsWidgetMaker.cpp 17
  • ....
  • V820 The 'dayMonthText' variable is not used after copying. Copying can be replaced with move/swap for optimization. CalendarData.hpp 51
  • V820 The 'newRange' variable is not used after copying. Copying can be replaced with move/swap for optimization. SpinnerPolicies.hpp 83
  • V820 The 'newRange' variable is not used after copying. Copying can be replaced with move/swap for optimization. SpinnerPolicies.hpp 290
  • ....

Working with std::optional

V830 Decreased performance. Consider replacing the 'draft.value()' expression to '*draft'. SMSInputWidget.cpp 158

class SMSInputWidget : public ListItem
{
  ....
  std::optional<SMSRecord> draft;
  ....
}

....

void SMSInputWidget::updateDraftMessage(....)
{
  ....
  if (draft.has_value()) 
  {
    app->updateDraft(draft.value(), inputText);
  }
  ....
}

Here, while calling has_value, we see that the draft variable (whose type os std::optional)definitely contains a value inside. In this case, you don't need to call the value() method that will check again if there is a value before returning it. Use the * operator that will return the value that is obviously available here.

Here, one could argue that modern compilers optimize such code quite well. Yes, it would be correct to call this fix as code optimization that possibly reduces the overhead. If the compiler can't substitute function bodies (inlining) or such optimization is disabled, then the code version proposed below will work faster, and in other cases at least not slower:

void SMSInputWidget::updateDraftMessage(....)
{
  ....
  if (draft.has_value()) 
  {
    app->updateDraft(*draft, inputText);
  }
  ....
}

Here is another similar code example:

void ThreadItem::setContactName(std::optional<long int> numberImportance)
{
  ....
  if (numberImportance.has_value()) 
  {
    displayNumberImportance(numberImportance.value());
  }
  ....
}

You can refactor the code as follows:

void ThreadItem::setContactName(std::optional<long int> numberImportance)
{
  ....
  if (numberImportance.has_value()) 
  {
    displayNumberImportance(*numberImportance);
  }
  ....
}

However, such fixes have a downside: if you look at the use of the * overloaded operator and do not see the variable declaration, you might think that you are dealing with a pointer. Many people think that this is a rather strange semantics that should not be used. If you are one of these people, you can be easily disable this rule.

Just as with the V833 diagnostic, the analyzer issued a lot of similar V830 warnings (66 in total). If I decided to list them, it would take quite a few pages. So, let me show you just a few of them:

  • V830 Decreased performance. Consider replacing the 'lastSms.value()' expression to '*lastSms'. NewMessage.cpp 358
  • V830 Decreased performance. Consider replacing the 'currentFileToken.value()' expression to '*currentFileToken'. SongsPresenter.cpp 69
  • V830 Decreased performance. Consider replacing the 'returnedContact.value()' expression to '*returnedContact'. PhonebookNewContact.cpp 171
  • V830 Decreased performance. Consider replacing the 'activeDevice.value()' expression to '*activeDevice'. BluetoothSettingsModel.cpp 94
  • V830 Decreased performance. Consider replacing the 'selectedDevice.value()' expression to '*selectedDevice'. AllDevicesWindow.cpp 75
  • V830 Decreased performance. Consider replacing the 'blockSizeConstraint.value()' expression to '*blockSizeConstraint'. StreamFactory.cpp 72
  • ....

STL containers

V827 Maximum size of the 'actions' vector is known at compile time. Consider pre-allocating it by calling actions.reserve(3). BellAlarmHandler.cpp

auto BellAlarmClockHandler::getActions(sys::Service *service) -> Actions
{
  Actions actions;
  actions.emplace_back(....);
  actions.emplace_back(....);
  actions.emplace_back(....);
  return actions;
}

Here, we see the vector whose size is known at compile time. The analyzer suggests calling the reserve function before filling in the vector. If you don't call the reserve function, the emplace_back calls can lead to the internal buffer reallocation in the vector and the movement of elements to a new memory area. And if the move constructor of a class whose objects are stored in a vector is not marked as noexcept, the vector won't move, but copy the objects. You can reduce the overhead by allocating a buffer of the appropriate size. Here is the correct code:

auto BellAlarmClockHandler::getActions(sys::Service *service) -> Actions
{
  Actions actions;
  Actions.reserve(3);
  actions.emplace_back(....);
  actions.emplace_back(....);
  actions.emplace_back(....);
  return actions;
}

By the way, do you always make sure to mark your user-provided move constructors/operators as noexcept?

Traditionally, for MuditaOS, we have received many warnings of this diagnostic. Before we look at another V827 diagnostic warning, we need to explain some details of how this diagnostic works.

The diagnostic rule works based on the data flow analysis mechanism and suggests reserving the maximum possible number of elements. That is, if an element is added under a condition, the analyzer will take it into account and offer to reserve the maximum possible container size.

Let's look at a similar example:

V827 Maximum size of the 'ret' vector is known at compile time. Consider pre-allocating it by calling ret.reserve(8). Commands.cpp 11

std::vector<AT> getCommadsSet(commadsSet set)
{
  std::vector<AT> ret;

  switch (set) 
  {
    case commadsSet::modemInit:
      ret.push_back(AT::URC_NOTIF_CHANNEL);
      ret.push_back(AT::RI_PIN_AUTO_CALL);
      ret.push_back(AT::RI_PIN_PULSE_SMS);
      ret.push_back(AT::RI_PIN_PULSE_OTHER);
      ret.push_back(AT::URC_DELAY_ON);
      ret.push_back(AT::URC_UART1);
      ret.push_back(AT::AT_PIN_READY_LOGIC);
      ret.push_back(AT::CSQ_URC_ON);
      break;
    case commadsSet::simInit:
      ret.push_back(AT::CALLER_NUMBER_PRESENTATION);
      ret.push_back(AT::SMS_TEXT_FORMAT);
      ret.push_back(AT::SMS_GSM);
      ret.push_back(AT::CRC_ON);
      break;
    case commadsSet::smsInit:
      ret.push_back(AT::SET_SMS_STORAGE);
      ret.push_back(AT::SMS_TEXT_FORMAT);
      ret.push_back(AT::SMS_GSM);
      break;
  }
  return ret;
}

According to code, 8 push_back function may be called in the longest of the switch operator branches. The analyzer, detecting this, suggests invoking ret.reserve(8).

Here is the list of a few more V827 triggerings:

  • V827 Maximum size of the 'data' vector is known at compile time. Consider pre-allocating it by calling data.reserve(3) ServiceCellular.cpp 1093
  • V827 Maximum size of the 'commandParts' vector is known at compile time. Consider pre-allocating it by calling commandParts.reserve(8) CallForwardingRequest.cpp 42
  • V827 Maximum size of the 'pathElements' vector is known at compile time. Consider pre-allocating it by calling pathElements.reserve(4) AudioCommon.cpp 51

Let's now move on to the next innovative diagnostic. The diagnostic detects containers from the standard library that you can replace with other containers for optimization purposes.

To determine which container type will suit better in a given case, heuristics are used based on what operations are performed on the container. The analyzer also calculates algorithmic complexity of all the operations and suggests a container whose algorithmic complexity is lowest. Let's see what we found with the help of this diagnostic:

V826 Consider replacing the 'dbFileExt' std::vector with std::array. The size is known at compile time. common.cpp 9

void RemoveDbFiles(const std::string &dbName)
{
  std::vector<std::string> dbFileExt = {".db", ".db-journal", ".db-wal"};
  for (const auto &ext : dbFileExt) 
  {
    const auto dbPath = (std::filesystem::path{"sys/user"} / 
                         std::filesystem::path{dbName + ext});
    if (std::filesystem::exists(dbPath)) 
    {
      std::filesystem::remove(dbPath.c_str());
    }
  }
}

In this case, the analyzer says that with the container size known at compile time. It is preferable to use std::array instead of std::vector. It will help avoid dynamic allocation. You can also do the following:

  • Declare an array with the static specifier so that it is calculated once.
  • If string literals are placed inside the container, then replace std::string with std::string_view. Since the filesystem library is used in the code, we can assume that the code is compiled with the C++17 version of the standard and std::string_view is also can be used in the code base.
  • Hmm, now we have the array of std::string_view, both classes are able to work in compile-time. So, you can declare an array with the constexpr specifier.

The function after all fixes looks as follows:

void RemoveDbFiles(const std::string &dbName)
{
  using namespace std::literals;
  static constexpr std::array dbFileExt = 
                                     {".db"sv, ".db-journal"sv, ".db-wal"sv};

  for (auto ext : dbFileExt)
  {
    const auto dbPath = (std::filesystem::path{"sys/user"} /
                        std::filesystem::path{dbName + std::string { ext }});
    if (std::filesystem::exists(dbPath)) 
    {
      std::filesystem::remove(dbPath.c_str());
    }
  }
}

You can compare the output generated by the GCC compiler for the original and optimized code on Compiler Explorer.

In general, the application field of the V826 diagnostic rule is wide and covers many different cases. Here is another example of triggering:

V826 Consider replacing the 'carriers' std::list with std::vector. Contiguous placement of elements in memory can be more efficient. SpecialInputModel.cpp 45

void SpecialInputModel::buildGrid(const std::vector<char32_t> &elements)
{
  while (....) 
  {
    ....
    std::list<gui::Carrier> carriers;
    for (....) 
    {
      ....
      carriers.push_back(....);
      ....
      carriers.push_back(....);
    }
    ....
  }
  ....
  internalData.push_back
              (new gui::SpecialInputTableWidget(...., std::move(carries));
}

This warning is, of course, controversial. That's why the analyzer gives it the third level of severity. It is due to the fact that a developer adds elements only to the end of the container, as it usually happens with std::vector.

Whether MuditaOS developers should fix it or not, as I've already said, is a moot point:

  • On the one hand, they create the std::list, add elements there and pass it forward. In this specific case, it's more efficiently to use std::list, since adding elements to the end is guaranteed in constant time. The addition of elements to the vector takes place in amortized constant time due to possible reallocations that are specific to this container.
  • On the other hand, elements are added for a reason. Already in the SpecialInputTableWidget function, the carriers container is traversed. In this case, it is preferable to use std::vector. The std::list container is not required to place data sequentially. As a result, cache misses are possible when traversing the container. Due to the sequential arrangement of elements in memory, the vector is much more friendly to the processor cache. This gives a gain in linear access to its elements, if the size of the elements is small. The smaller the size of the elements compared to the cache line, the more elements the processor can load in one read.

This is just a couple of all V826 diagnostic warnings that seemed interesting to me. In reality, the analyzer issued much more warnings. Some of these warnings are very easy to fix. For example, like in the case when the container is locally created, used and destroyed after exiting the function. Other warnings are more complicated. Like those where the container is traversed through several functions.

As in the previous case, I am not sure which warnings should be fixed and which ones should not. So I'll leave it to the MuditaOS developers. Meanwhile, we are moving on!

Unused variables

Usually, warnings about unused variables are not so gripping. When you're reading code, you can't be sure that the error found indicates an incorrectly implemented algorithm or that the code does not work as developer expected it to work. Rather, it seems that the troublesome code was changed during refactoring, and somebody simply forgot to delete the unused variable.

When looking through the log with diagnostic warnings, I found one interesting code pattern for which the analyzer complained:

V808 'valStr' object of 'basic_string' type was created but was not utilized. AlarmSettingsModel.cpp 23

void AlarmVolumeModel::setValue(std::uint8_t value)
{
  const auto valStr = std::to_string(value);
  audioModel.setVolume(value, AbstractAudioModel::PlaybackType::Alarm, {});
}

We found similar code fragment 12 lines below:

V808 'valStr' object of 'basic_string' type was created but was not utilized. PrewakeUpSettingsModel.cpp 35

void PrewakeUpChimeVolumeModel::setValue(std::uint8_t value)
{
  const auto valStr = std::to_string(value);
  audioModel.setVolume(value, AbstractAudioModel::PlaybackType::PreWakeup, {});
}

And a couple more warnings issued on the same code pattern:

  • V808 'valStr' object of 'basic_string' type was created but was not utilized. SnoozeSettingsModel.cpp 76
  • V808 'valStr' object of 'basic_string' type was created but was not utilized. BedtimeModel.cpp 80

Several warnings issued on the same code pattern give me philosophical thoughts. Firstly, the code was definitely copy-pasted. Secondly, the unused variable indicates that the code was definitely rewritten. I wonder which of these things happened earlier...

Here are a few more V808s:

  • V808 'deviceAddress' object of 'basic_string' type was created but was not utilized. A2DP.cpp 332
  • V808 'operatorNames' object of 'vector' type was created but was not utilized. NetworkSettings.cpp 263
  • V808 'volume' object of 'optional' type was created but was not utilized. AudioServiceAPI.cpp 224
  • ....

Strings

V817 It is more efficient to seek '/' character rather than a string. TagsFetcher.cpp 28

std::optional<Tags> fetchTagsInternal(std::string filePath)
{
  ....
  if (const auto pos = filePath.rfind("/"); pos == std::string::npos) 
  {
    ....
  }
  ....
}

The analyzer detected the code fragment that looks for a character in a string. The fragment can be optimized. You can use the find overload that receives a character instead of a string. Searching for a substring in a string means going through all the characters in the strings — two loops. If we are searching for a character, we need to go through one loop. Optimized version:

std::optional<Tags> fetchTagsInternal(std::string filePath)
{
  ....
  if (const auto pos = filePath.rfind('/'); pos == std::string::npos) 
  {
    ....
  }
  ....
}

Here are a few more warnings to pay attention to:

  • V817 It is more efficient to seek '\"' character rather than a string. response.cpp 489
  • V817 It is more efficient to seek '\"' character rather than a string. ATURCStream.cpp 45
  • V817 It is more efficient to seek '\"' character rather than a string. ATURCStream.cpp 78
  • V817 It is more efficient to seek '.' character rather than a string. DatabaseInitializer.cpp 97
  • V817 It is more efficient to seek '.' character rather than a string. DbInitializer.cpp 87
  • V817 It is more efficient to seek ' ' character rather than a string. test-gui-TextBlockCursor.cpp 424
  • V817 It is more efficient to seek '+' character rather than a string. CallForwardingRequest.cpp 82
  • V817 It is more efficient to seek ',' character rather than a string. ServiceCellular.cpp 1398
  • V817 It is more efficient to seek 'a' character rather than a string. unittest_utf8.cpp 108

Next, let's look at the warnings that indicate inefficient calculation of the string length:

V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. ATStream.cpp 127

constexpr auto delimiter = "\r\n"
....
void ATStream::countLines()
{
  ....
  auto pos = ....;
  while (pos != std::string::npos) 
  {
    if ((lastPos) != pos) 
    {
      ....
    }
    lastPos = pos + std::strlen(at::delimiter);
  }
}

The analyzer has detected a situation in which each loop's iteration calls the std::strlen function with the delimiter constant. The value of the constant isn't changed. It means that the string length can be calculated beforehand. This optimizes the code. Let's use C++17 and change the constant type to std::string_view. We can get the string length with O(1) by calling the size non-static member function:

constexpr std::string_view delimiter = "\r\n"
....

void ATStream::countLines()
{
  ....
  auto pos = ....;
  auto delimiterLen = delimiter.size();
  while (pos != std::string::npos) 
  {
    if ((lastPos) != pos) 
    {
      ....
    }
    lastPos = pos + delimiterLen;
  }
}

Here is another similar case:

V814 Decreased performance. The 'strlen' function was called multiple times inside the body of a loop. DLCChannel.cpp 140

This is not all the "adventures" of the delimiter constant. The analyzer issued a couple of warnings for another function:

V810 Decreased performance. The 'std::strlen(at::delimiter)' function was called several times with identical arguments. The result should possibly be saved to a temporary variable, which then could be used while calling the 'substr' function. ATStream.cpp 89

V811 Decreased performance. Excessive type casting: string -> char * -> string. Consider inspecting the expression. ATStream.cpp 89

bool ATStream::checkATBegin()
{
  auto pos = atBuffer.find(at::delimiter, std::strlen(at::delimiter));
  ....
  std::string rr = atBuffer.substr(std::strlen(at::delimiter),
                                   pos - std::strlen(at::delimiter)).c_str();
  ....
}
  • The first warning indicates that the strlen function is called twice within the same expression.
  • The second warning indicates that something strange is happening in the code. We call the substr function from the atBuffer variable. The function returns std::string. Next, we call the c_str() function from the result. The called function converts the result to const char*. After that, we again implicitly convert the result to std::string (we calculate the string length — the type that now is const char* — it means calling strlen again) and finally assign the result to the rr variable.

Let's fix both code fragments. Remember that after the fix from the previous example, delimiter is now std::string_view:

bool ATStream::checkATBegin()
{
  auto delimiterLen = delimiter.size();
  auto pos = atBuffer.find(at::delimiter, delimiterLen);
  ....
  std::string rr = atBuffer.substr(delimiterLen
                                   pos - delimiterLen);
  ....
}

Here are similar warnings of V810 and V811 diagnostics, which are worth paying attention to:

  • V810 Decreased performance. The 'std::strlen(at::delimiter)' function was called several times with identical arguments. The result should possibly be saved to a temporary variable, which then could be used while calling the 'substr' function. ATStream.cpp 106
  • V810 Decreased performance. The 'translate_mode_to_attrib(mode)' function was called several times with identical arguments. The result should possibly be saved to a temporary variable, which then could be used while calling the 'f_chmod' function. filesystem_vfat.cpp 560
  • V810 Decreased performance. The 'translate_mode_to_attrib(mode)' function was called several times with identical arguments. The result should possibly be saved to a temporary variable, which then could be used while calling the 'f_chmod' function. filesystem_vfat.cpp 572
  • V811 Decreased performance. Excessive type casting: string -> char * -> string. Consider inspecting the 'ss.str().c_str()' expression. AppMessage.hpp 216
  • V811 Decreased performance. Excessive type casting: string -> char * -> string. Consider inspecting the expression. ATStream.cpp 105
  • V811 Decreased performance. Excessive type casting: string -> char * -> string. Consider inspecting second argument of the function handleStart. ServiceAudio.cpp 73

Other diagnostic warnings

V821 [CERT-DCL19-C] Decreased performance. The 'factory' variable can be constructed in a lower level scope. CallLogDetailsWindow.cpp 147

void CallLogDetailsWindow::initNumberWidget()
{
  ....
  ActiveIconFactory factory(this->application);
  ....
  if (....) 
  {
    ....
  }
  else 
  {
    ....
    numberHBox->addIcon(factory.makeCallIcon(numberView));
    numberHBox->addIcon(factory.makeSMSIcon(numberView));
    ....
  }
}

The analyzer has detected the factory variable that could be created in a lower level scope. By changing the scope of an object, you can optimize the code's performance and memory consumption.

The correct version of the code may look like this:

void CallLogDetailsWindow::initNumberWidget()
{
  ....
  if (....) 
  {
    ....
  }
  else 
  {
    ....
    ActiveIconFactory factory(this->application);
    numberHBox->addIcon(factory.makeCallIcon(numberView));
    numberHBox->addIcon(factory.makeSMSIcon(numberView));
    ....
  }
}

The analyzer issued V821 diagnostic warnings for several more code fragments. Here is the list of them:

  • V821 [CERT-DCL19-C] Decreased performance. The 'size' variable can be constructed in a lower level scope. BoxLayoutSizeStore.cpp 19
  • V821 [CERT-DCL19-C] Decreased performance. The 'local_style' variable can be constructed in a lower level scope. RichTextParser.cpp 385
  • V821 [CERT-DCL19-C] Decreased performance. The 'defaultValue' variable can be constructed in a lower level scope. ServiceAudio.cpp 702
  • V821 [CERT-DCL19-C] Decreased performance. The 'js' variable can be constructed in a lower level scope. i18n.cpp 84
  • V821 [CERT-DCL19-C] Decreased performance. The 'it' variable can be constructed in a lower level scope. disk_manager.cpp 49

Conclusion

Strangely enough, we covered only part of the micro-optimization diagnostic warnings found in MuditaOS. In fact, there are about a thousand of them. I think this article is already lengthy enough and, if I show you more warnings, it will be just hard to read.

As I said at the beginning of the article, if you fix micro-optimization warnings one at a time, it will most likely not greatly affect the performance of the whole project. However, if you fix all of them, or at least most of them, sometimes you can get a noticeable performance gain. But, of course, it depends more on the case, or rather, on how often inefficient code fragments are executed.

One day, at a conference, one of our clients stopped by our stand. They told us that his team increased the project performance by tens of percent by using PVS-Studio. They simply fixed several troublesome functions that for some reason took a vector of strings not by reference but by value. Unfortunately, there are no proofs.

If after reading this article you have a desire to check your project, you can easily do this by requesting a trial key on our website. If you already use PVS-Studio and have not used optimization diagnostics before — this is exactly the right time to try them out.

Popular related articles


Comments (0)

Next comments next comments
close comment form