IBProvider Developer's Blog

Вести с полей

На сайт загружена новая тестовая сборка IBProvider — v3.55.0.29115.

В неё, в том числе, вошли результаты борьбы с предупреждениями PVS-Studio.

95% это замена assert на assert_hint.

//-V:assert_hint:779, 547
#define assert_hint(cond) assert(cond);

Из этих 95% большая часть — в моей переработанной версии libtommath, которую перед тем как затащить в проект пришлось перетряхнуть и нафаршировать ассертами.

Для остальных 5% пришлось напрячься.

Самый мутный случай в этой самой libtommath — функция mp_div. PVS говорит что в строке 361 условие всегда false:

t2.dp[0] = (i < 2) ? 0 : x.dp[i - 2];
t2.dp[1] = (i < 1) ? 0 : x.dp[i - 1]; // [2018-12-24] Always = x.dp[i - 1];
t2.dp[2] = x.get_safe(i);
t2.used  = 3;

И судя по моим выводам, это действительно так. Сам код пока не трогал, но добавил ассерты, которые это подтверждают. Будем посмотреть на работу отладочной сборки провайдера под тестами.

Остальные изменения можно описать так — «пишите просто, еще проще». Например.

Было:

const wchar_t* TDBID::GetUnicodeNamePtr(const DBID& dbid,bool NotNull)
{
 const wchar_t* ptr=NULL;

 if(self_type::HasName(dbid))
  ptr=dbid.uName.pwszName;

 if(NotNull && ptr==NULL)
  return L"";

 return ptr;
}//GetUnicodeNamePtr

Стало:

const wchar_t* TDBID::GetUnicodeNamePtr(const DBID& dbid,bool const NotNull)
{
 if(!self_type::HasName(dbid))
 {
  if(NotNull)
   return L"";

  return structure::null_ptr;
 }//if no name

 if(NotNull)
 {
  if(!dbid.uName.pwszName)
   return L"";

  assert_hint(dbid.uName.pwszName);
 }//if NotNull

 return dbid.uName.pwszName;
}//GetUnicodeNamePtr

PVS спотыкался об локальную переменную ptr и говорил, что в коде

   if(wcscmp(self_type::GetUnicodeNamePtr(*pid1,/*NotNULL*/true),
             self_type::GetUnicodeNamePtr(*pid2,/*NotNULL*/true))!=0)
   {
    return false;
   }

возможна передача нулевых указателей в wcscmp. Не рюхает он такие косвенные вещи.

Второй пример. Метод чтения из storage-объекта.
Было:

template<class Allocator>
HRESULT __stdcall basic_dbvariant_sstream<Allocator>::Read(void*  const pv,
                                                           ULONG        cb,
                                                           ULONG* const pcbRead)
{
 OLE_LIB_IMETHOD_PROLOG

 ::SetErrorInfo(0,structure::null_ptr);

 if(pcbRead!=NULL)
  (*pcbRead)=0;

 if(pv==NULL && cb!=0)
  return E_INVALIDARG;

 OLE_LIB__DECLARE_HR(S_OK);

 _OLE_TRY_
 {
  const lock_guard_type __lock__(m_guard);

  assert(m_pos<=m_size);

  const size_type cb_tail=size_type(m_size-m_pos);

  if(cb_tail==0)
  {
   hr=S_FALSE;
  }
  else
  {
   assert_hint(cb_tail>0);

   if(cb_tail<cb)
    structure::static_numeric_cast(&cb,cb_tail);

   assert(cb<=cb_tail);

   if(cb>0)
   {
    memcpy(pv,&m_data[m_pos],cb);

    m_pos+=cb;

    if(pcbRead)
     (*pcbRead)=cb;
   }//if cb>0

   assert_hint(hr==S_OK);
  }//else
 }
 _OLE_CATCHES2_CODE_

 assert(hr==S_OK || hr==S_FALSE || FAILED(hr));

 return hr;
}//Read

Стало:

template<class Allocator>
HRESULT __stdcall basic_dbvariant_sstream<Allocator>::Read(void*  const pv,
                                                           ULONG        cb,
                                                           ULONG* const pcbRead)
{
 OLE_LIB_IMETHOD_PROLOG

 ::SetErrorInfo(0,structure::null_ptr);

 if(pcbRead!=NULL)
  (*pcbRead)=0;

 if(cb!=0)
 {
  assert_hint(cb>0);

  if(pv==NULL)
   return E_INVALIDARG;
 }//if cb!=0

 OLE_LIB__DECLARE_HR(S_OK);

 _OLE_TRY_
 {
  const lock_guard_type __lock__(m_guard);

  assert(m_pos<=m_size);

  const size_type cb_tail=size_type(m_size-m_pos);

  if(cb_tail==0)
  {
   hr=S_FALSE;
  }
  else
  if(cb==0)
  {
   //none
   assert_hint(hr==S_OK);
  }
  else
  {
   assert_hint(cb_tail>0);
   assert_hint(cb>0);

   if(cb_tail<cb)
    structure::static_numeric_cast(&cb,cb_tail);

   assert(cb<=cb_tail);
   assert(cb>0);

   assert(pv);

   memcpy(pv,&m_data[m_pos],cb);

   m_pos+=cb;

   if(pcbRead)
    (*pcbRead)=cb;

   assert_hint(hr==S_OK);
  }//else
 }
 _OLE_CATCHES2_CODE_

 assert(hr==S_OK || hr==S_FALSE || FAILED(hr));

 return hr;
}//Read

Если я правильно помню, PVS говорила что в memcpy может заехать нулевой pv. Перестроил. Новый вариант PVS-у понравился. Мне, определенно, тоже.

Еще один случай. По поводу которого я даже написал в ООН PVS и получил ответ из местного жека — «не выпендривайтесь, пишите проще» 🙂

 if(n!=0 && (pSourceData==NULL || pTargetData==NULL))
  return E_FAIL;

 for(;n;--n)
 {
  const HRESULT
   copy_hr=ole_lib::Variant_Copy(reinterpret_cast<VARIANT*>(pTargetData),
                                 reinterpret_cast<const VARIANT*>(pSourceData));

  assert(copy_hr==S_OK || FAILED(copy_hr));

  if(FAILED(copy_hr))
   return copy_hr;

  pSourceData+=cbSourceElement;
  pTargetData+=cbTargetElement;
 }//for n

Стало:

 if(n)
 {
  assert_hint(n>0);

  if(pSourceData==NULL || pTargetData==NULL)
   return E_FAIL;

  for(;;)
  {
   assert(n>0);
   assert(pSourceData);
   assert(pTargetData);

   const HRESULT
    copy_hr=ole_lib::Variant_Copy(reinterpret_cast<VARIANT*>(pTargetData),
                                  reinterpret_cast<const VARIANT*>(pSourceData));

   assert(copy_hr==S_OK || FAILED(copy_hr));

   if(FAILED(copy_hr))
    return copy_hr;

   pSourceData+=cbSourceElement;
   pTargetData+=cbTargetElement;

   --n;

   if(n==0)
    break;
  }//for[ever]
 }//if n

 assert_hint(n==0);

Ну да, код стал проще и лучше. Но больше 🙂

—
А баги, Карл, баги PVS нашла?

Нашла, блин. Но, к счастью, не в провайдере. Самые эпичные выглядят так:

1.1          (dima     08-Oct-04): template<class t_level,class t_string>
1.1          (dima     08-Oct-04): t_pair_level_name<t_level,t_string>::t_pair_level_name(const self_type& x)
1.1          (dima     08-Oct-04):  :level(level),
1.1          (dima     08-Oct-04):   name (name)
1.1          (dima     08-Oct-04): {;}
1.1          (dima     08-Oct-04): //------------------------------------------------------------------------
1.1          (dima     08-Oct-04): template<class t_level,class t_string>
1.1          (dima     08-Oct-04): RELEASE_CODE(inline)
1.1          (dima     08-Oct-04): t_pair_level_name<t_level,t_string>&
1.1          (dima     08-Oct-04): t_pair_level_name<t_level,t_string>::operator = (const self_type& x)
1.1          (dima     08-Oct-04): {
1.1          (dima     08-Oct-04):  level =level;
1.1          (dima     08-Oct-04):  name  =name;
1.1          (dima     08-Oct-04): 
1.1          (dima     08-Oct-04):  return *x;
1.1          (dima     08-Oct-04): }//operator =

… и не встать. Я помню этот долбанный октябрь 2004 года. Повезло, что этот код нигде не использовался. Но, подключался.

Вывод — не надо писать код «про запас». Это отчетливо понимаешь только с ближе к пенсии.

Еще PVS-нашла неинициализированный член класса в примерах на C++ для IBProvider. Не критично, но тем не менее.

—
В целом.

Штука полезная. Я бы даже сказал «у моих программ есть два друга — тесты и статические анализаторы кода«.

Но хуже семечек.

IBProvider она анализирует 28 минут (~1440 файлов). На десятиядерном процессоре.

Ни разу не упала 🙂

PS. Простите меня за то, что пользовался триалом PVS 🙂