محلل الكود خاطئ ، يعيش المحلل

من السيء الجمع بين العديد من الإجراءات في تعبير واحد من لغة C ++ ، حيث يصعب فهم هذا الرمز ، ومن الصعب الحفاظ عليه ، كما أنه من السهل ارتكاب خطأ فيه. على سبيل المثال ، قم بإنشاء خطأ من خلال الجمع بين الإجراءات المختلفة عند تقييم وسيطات الوظيفة. نتفق مع التوصية الكلاسيكية بأن الكود يجب أن يكون بسيطًا ومباشرًا. والآن سننظر في حالة مثيرة للاهتمام ، عندما يكون محلل PVS-Studio رسميًا خاطئًا ، ولكن من وجهة نظر عملية ، لا يزال ينبغي تغيير الكود.

أمر تقييم الحجج

ما سيقال الآن هو استمرار للقصة القديمة حول ترتيب حساب الحجج ، والتي كتبنا عنها في مقال " عمق حفرة أرنب أو مقابلة في C ++ في PVS-Studio ".



الجوهر المختصر هو كما يلي. الترتيب الذي يتم به تقييم وسيطات الوظيفة هو سلوك غير محدد. لا يحدد المعيار الترتيب الدقيق الذي يجب على مطوري المترجمين تقييم الوسائط. على سبيل المثال ، من اليسار إلى اليمين (Clang) أو من اليمين إلى اليسار (GCC ، MSVC). قبل معيار C ++ 17 ، عندما تحدث آثار جانبية عند تقييم الحجج ، قد يؤدي ذلك إلى سلوك غير محدد.



مع ظهور معيار C ++ 17 ، تغير الوضع للأفضل: الآن سيبدأ حساب الوسيطة وآثارها الجانبية فقط من اللحظة التي يتم فيها إجراء جميع الحسابات والآثار الجانبية للحجة السابقة. ومع ذلك ، هذا لا يعني أنه لا يوجد مجال للخطأ الآن.



ضع في اعتبارك برنامج اختبار بسيط:

#include <cstdio>
int main()
{
  int i = 1;
  printf("%d, %d\n", i, i++);
  return 0;
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

ماذا سيطبع هذا الرمز؟ لا تزال الإجابة تعتمد على المترجم والإصدار والمزاج. اعتمادًا على المترجم ، يمكن طباعة "1 ، 1" و "2 ، 1". في الواقع ، باستخدام برنامج Compiler Explorer ، أحصل على النتائج التالية:

• ينتج برنامج تم تجميعه باستخدام Clang 11.0.0 "1 ، 1".
• ينتج برنامج تم تجميعه مع GCC 10.2 "2 ، 1".

لا يوجد سلوك غير محدد في هذا البرنامج ، ولكن هناك سلوك غير محدد (ترتيب تقييم الحجج).

كود من مشروع CSV Parser

دعنا نعود إلى مقتطف الشفرة من مشروع CSV Parser ، الذي ذكرته في مقالة " التحقق من مجموعة مكتبات C ++ فقط (awesome-hpp) ".



أنا والمحلل نعلم أنه يمكن تقييم الحجج بترتيب مختلف. لذلك اعتبر المحلل وبعده أن هذا الرمز خاطئ:

std::unique_ptr<char[]> buffer(new char[BUFFER_UPPER_LIMIT]);
....
this->feed_state->feed_buffer.push_back(
    std::make_pair<>(std::move(buffer), line_buffer - buffer.get()));
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

تحذير PVS-Studio: V769 مؤشر "buffer.get ()" في تعبير "line_buffer - buffer.get ()" يساوي nullptr. القيمة الناتجة لا معنى لها ولا ينبغي استخدامها. csv.hpp 4957 في



الواقع ، كلانا مخطئ وليس هناك خطأ. حول الفروق الدقيقة ستكون أبعد ، لكن في الوقت الحالي لنبدأ بفروق بسيطة.



لذا ، دعنا نرى سبب خطورة كتابة كود مثل هذا:

Foo(std::move(buffer), line_buffer - buffer.get());
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

أعتقد أنه يمكنك تخمين الإجابة. تعتمد النتيجة على الترتيب الذي يتم به تقييم الوسيطات. ضع في اعتبارك هذا في الكود التركيبي التالي:

#include <iostream>
#include <memory>   

void Print(std::unique_ptr<char[]> p, ptrdiff_t diff)
{
    std::cout << diff << std::endl;
} 

void Print2(ptrdiff_t diff, std::unique_ptr<char[]> p)
{
    std::cout << diff << std::endl;
} 

int main()
{
    {
        std::unique_ptr<char[]> buffer(new char[100]);
        char *ptr = buffer.get() + 22;
        Print(std::move(buffer), ptr - buffer.get());
    }
    {
        std::unique_ptr<char[]> buffer(new char[100]);
        char *ptr = buffer.get() + 22;
        Print2(ptr - buffer.get(), std::move(buffer));
    }
    return 0;
}
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

دعنا نستخدم Compiler Explorer مرة أخرى ونرى ناتج هذا البرنامج الذي تم تجميعه بواسطة مترجمين مختلفين.



مترجم كلانج 11.0.0. النتيجة :

23387846
22
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

مترجم GCC 10.2. النتيجة :

22
26640070
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

نتوقع النتيجة ، ولا يمكنك الكتابة بهذه الطريقة. هذا هو بالضبط ما يحذر منه محلل PVS-Studio.



أود أن أنهي هذا ، لكن كل شيء أكثر تعقيدًا بعض الشيء. الحقيقة هي أننا نتحدث عن تمرير الوسائط حسب القيمة ، وعند إنشاء قالب دالة std :: make_pair ، سيكون كل شيء مختلفًا. دعنا نواصل الغوص في الفروق الدقيقة ونكتشف سبب خطأ PVS-Studio في هذه الحالة.

الأمراض المنقولة جنسيا :: make_pair

دعنا نلقي نظرة على موقع الويب cppreference ونرى كيف تغير قالب وظيفة std :: make_pair .



حتى C ++ 11:

template <class T1، class T2>

std :: pair <T1، T2> make_pair (T1 t، T2 u) ؛

منذ C ++ 11 ، حتى C ++ 14:

template <class T1، class T2>

std :: pair <V1، V2> make_pair (T1 && t، T2 && u) ؛

منذ C ++ 14:

template< class T1, class T2 >

constexpr std::pair<V1,V2> make_pair( T1&& t, T2&& u );

كما ترى ، ذات مرة ، أخذ std :: make_pair الحجج حسب القيمة. إذا كانت std :: unique_ptr موجودة في تلك الأيام ، فإن الكود الذي تمت مناقشته أعلاه سيكون بالفعل غير صحيح. ما إذا كان هذا الرمز سيعمل أم لا هو مسألة حظ. من الناحية العملية ، بالطبع ، لن يظهر هذا الموقف أبدًا ، حيث ظهر std :: unique_ptr في C ++ 11 كبديل لـ std :: auto_ptr .



دعنا نعود إلى عصرنا. بدءًا من إصدار معيار C ++ 11 ، بدأ المُنشئ في استخدام دلالات النقل.



هناك نقطة دقيقة هنا في أن std :: move لا تنقل شيئًا في الواقع ، ولكنها تحول الكائن فقط إلى مرجع rvalue . وهذا سيسمحstd :: make_pair قم بتمرير المؤشر إلى std :: unique_ptr الجديدة ، تاركًا nullptr في المؤشر الذكي الأصلي. لكن تمرير المؤشر هذا لن يحدث حتى ندخل إلى داخل std :: make_pair . بحلول ذلك الوقت ، قمنا بالفعل بحساب line_buffer - buffer.get () ، وسيكون كل شيء على ما يرام. أي أن استدعاء الدالة buffer.get () لا يمكنه إرجاع القيمة nullptr في وقت تقييمه ، بغض النظر عن وقت حدوثه بالضبط.



أنا آسف على الوصف المعقد. خلاصة القول هي أن هذا الرمز صحيح. وفي الواقع ، أعطى محلل PVS-Studio الثابت في هذه الحالة نتيجة إيجابية خاطئة. ومع ذلك ، فإن فريقنا ليس متأكدًا من أننا يجب أن نسرع ​​لإجراء تغييرات على منطق المحلل في مثل هذه المواقف.

مات الملك، عاش الملك!

لقد اكتشفنا أن العملية المذكورة في المقال تبين أنها خاطئة. شكرًا لأحد قرائنا الذي لفت انتباهنا إلى خصوصية تنفيذ std :: make_pair .



ومع ذلك ، هذا هو الحال عندما لا نكون متأكدين مما إذا كان الأمر يستحق تحسين سلوك المحلل. النقطة المهمة هي أن هذا الرمز معقد للغاية. يجب أن تعترف بأن ما قمنا بتحليله من الكود لا يستحق مثل هذا التحقيق التفصيلي ، الذي استنفد مقالة كاملة. إذا كان هذا الرمز يتطلب الكثير من الاهتمام ، فهو رمز سيء للغاية.



ومن المناسب هنا التذكير بمقال " الإيجابيات الكاذبة أعداؤنا ، لكن ربما يظلون أصدقاءك ". المنشور ليس لنا ، لكننا نتفق معه.



ربما هذا هو الحال بالذات. قد يكون التحذير خاطئًا ، لكنه يشير إلى مكان أفضل لإعادة البناء. يكفي أن تكتب شيئًا مثل هذا:

auto delta = line_buffer - buffer.get();
this->feed_state->feed_buffer.push_back(
  std::make_pair(std::move(buffer), delta));
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

أو يمكنك جعل الكود أفضل في هذه الحالة باستخدام طريقة emplace_back :

auto delta = line_buffer - buffer.get();
this->feed_state->feed_buffer.emplace_back(std::move(buffer), delta);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

سيقوم هذا الرمز بإنشاء كائن std :: pair النهائي في الحاوية "في المكان" ، متجاوزًا إنشاء كائن مؤقت ونقله إلى الحاوية. بالمناسبة ، يقترح محلل PVS-Studio إجراء مثل هذا الاستبدال عن طريق إصدار تحذير V823 من مجموعة القواعد الخاصة بالتحسينات الدقيقة للكود.



سيصبح الكود أبسط وأوضح بشكل لا لبس فيه لأي قارئ ومحلل. لا توجد ميزة في حشر أكبر عدد ممكن من الإجراءات في سطر واحد من التعليمات البرمجية.



نعم في هذه الحالة كان محظوظا ولا خطأ. لكن من غير المحتمل أن يكون المؤلف ، عند كتابة هذا الرمز ، قد احتفظ بكل ما ناقشناه في رأسه. على الأرجح ، كان الحظ هو الذي لعب. ومرة أخرى قد لا تكون محظوظًا.

خاتمة

لذلك ، اكتشفنا أنه لا يوجد خطأ حقيقي. المحلل يولد إيجابية خاطئة. ربما سنزيل التحذير في مثل هذه الحالات فقط ، وربما لا. سوف نفكر في الأمر لاحقًا. بعد كل شيء ، هذه حالة نادرة إلى حد ما ، والرمز الذي يتم فيه تقييم الحجج مع الآثار الجانبية أمر خطير بشكل عام ، ومن الأفضل تجنبه. من الجدير إعادة البناء على الأقل لأغراض وقائية.



عرض الكود:

Foo(std::move(buffer), line_buffer - buffer.get());
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
    

من السهل كسرها عن طريق تغيير شيء ما في مكان آخر في البرنامج. رمز مثل هذا يصعب الحفاظ عليه. وهو أيضًا غير سار لأنه يمكن أن يعطي شعورًا خاطئًا بأن كل شيء يعمل بشكل صحيح. في الواقع ، هذه مجرد مصادفة ، ويمكن أن ينكسر كل شيء إذا قمت بتغيير المحول البرمجي أو إعدادات التحسين.



اجعل شفرتك أسهل!

إذا كنت ترغب في مشاركة هذه المقالة مع جمهور يتحدث الإنجليزية ، فيرجى استخدام رابط الترجمة: Andrey Karpov. محلل الكود خاطئ. يعيش المحلل! ...