怎樣做代碼Review

最新 05-25

Review是極為重要的代碼質量工藝。本文介紹這個工藝步驟的執行要領。

很多人都說Review很重要，但我們本身是否真的認為這個步驟非常重要呢？我們在生活中，說某個事情重要，其實有紙面上的，有實踐中的。不少同學和同學說話的時候都勸人家不要玩遊戲，自己回家第一件事是打開遊戲機。長大了也見不得會更好，老同學聚會勸人家「少抽點煙，對身體不好」，說完就自己點一支。

Review這個事情一樣，說的時候，都說「Review」很重要，實際上，到時間不夠的時候，首先割掉的是Review。甚至，在項目上多給你一個月，你首先也不投在Review上，而寧願投在系統測試上。我們得從這個角度看待所謂「Review的重要性」，否則，所謂「Review很重要」根本就是一句空話。

錯誤地認可「Review很重要」，不僅僅是個工作量取捨的問題。還是個如何應用Review的問題。為什麼我們寧願投入更多的時間到測試上，或者增加功能上，而不是代碼Review上呢？因為我們不一定注意到Review能解決多少其實系統測試測試不到的問題。

我們要這樣理解這個問題：一本書，或者你自己寫一篇博文，不Review一次或者幾次。你對拿出正式出版，有多大信心？我個人是一點信心都沒有。Review是唯一保證一段文字是否正確的手段。

但我們在程序上不是這樣認為，因為我們可以運行，程序本質上不是用來看，而是用來運行。這種情況，為什麼還需要Review？我想是因為運行無法窮舉，如果你的程序的出入口很容易窮舉，你確實可以用系統測試代碼Review。其實比如CPU核的驗證就是這樣的，我們也信不過肉眼的Review，這種就直接上測試套，隨機覆蓋，但這種測試都是以月為單位來做的，你是不是耗得起就要看你這是個什麼業務了。

解決無法窮舉的基本工藝有兩個，一個是Review，一個是UT。這兩個解決不同的問題，不能互相取代。Review主要解決的問題是語義，UT解決的問題主要是流程。

比如你調用了a=malloc(size)，並且允許size=0，那麼你就要判斷一下你要支持的所有目標平台，是不是都支持malloc(size)的入口是0，並且，這樣分配出來的指針是可以用free來釋放的（不少非posix的平台可不支持這個），這種問題用UT或者系統測試，都測試不出來。這種問題就是Review的強項。

換過來，比如你做一組malloc()，然後在中間出現錯誤的時候要退回去。或者做一個多重循環遍歷一個Hash數組並上一個單向鏈表。這種用人腦就遠遠不如計算機了，就不得通過Review來發現，而應該採用UT。

這樣，我們就可以總結出Review的關鍵了。軟體工程之所以是工程，就是重點解決成本問題。Review就是個工程問題，我們要採用最低成本的方法達成最好的結果。

所以Review的工程要領或者說工藝要求是：

1. 不通過Review發現UT可以發現的問題，不用Review取代UT

2. Review的評審重點是語義邏輯（包括錯誤的函數語義理解，錯誤的名稱定義，錯誤的Comment，錯誤的Assert，都是），代碼構架（比如是否有冗餘），統計，跟蹤，異常等處理邏輯在業務上的正確性，還有缺少的功能，可靠性，可維護性，可測試性等「反面理解」的問題。

3. 基於2，所以Review採用廣度優先遍歷，而不是深度優先遍歷（UT才採用廣度優先遍歷）。也就是說，看完一個函數，在看下一個函數，而不跟隨到那個函數的下面。

比如下面這個函數：

/** * prot_autoc_read_82599 - Hides MAC differences needed for AUTOC read * @hw: pointer to hardware structure * @locked: Return the if we locked for this read. * @reg_val: Value we read from AUTOC * * For this part (82599) we need to wrap read-modify-writes with a possible * FW/SW lock. It is assumed this lock will be freed with the next * prot_autoc_write_82599(). Note, that locked can only be true in cases * where this function doesn t return an error. **/ static s32 prot_autoc_read_82599(struct ixgbe_hw *hw, bool *locked, u32 *reg_val) { s32 ret_val; *locked = false; /* If LESM is on then we need to hold the SW/FW semaphore. */ if (ixgbe_verify_lesm_fw_enabled_82599(hw)) { ret_val = hw->mac.ops.acquire_swfw_sync(hw, IXGBE_GSSR_MAC_CSR_SM); if (ret_val) return IXGBE_ERR_SWFW_SYNC; *locked = true; } *reg_val = IXGBE_READ_REG(hw, IXGBE_AUTOC); return 0; }

Review這個代碼，很多人看到ixgbe_verify_lesm_fw_enabled_82599(hw)就想著跟過去看看那個流程有沒有問題。但你不是計算機，你執行它幹什麼？你要做的是，判斷這裡說lesm的firmware使能了，你是不是應該acquire它，如果沒有使能，你又能不能去讀那個寄存器。這樣才會聚焦的到「語義」上。至於穿過去的流程，你的腦子肯定沒有計算機好使，不反對看，但那不是重點。

而且，Review應該對這裡描述下來的每句話，包括注釋，都要「過眼」。這個函數是不是應該static的？讀寄存器到底應該用s32還是u32返回？為什麼？s32, u32, int, size_t, void *這種類型選擇，如果考慮到代碼是可以運行在多個硬體平台上的，基本上都有唯一選擇。

還有注釋，我這個可以正是在Linux主線代碼中隨便拷出來的，這個注釋就有錯。

@locked: Return the if we locked for this read.

Return the是什麼鬼意思？

無論是Review還是編程，都要基於「語義」，你的程序才具有延續性，僅僅能跑，是無法支持長遠的，這就叫基於「語義」來編程。

最後談談組織，我經驗比較好的組織方法是這樣：

1. 每次選擇1000行左右的代碼，不包括作者2-3人參與，必須是對相關模塊熟識的。

2. 先由作者做一個代碼介紹，按廣度優先遍歷整個代碼，全體參與需要5-10個小時

3. （可選）如果代碼足夠複雜，可以下來再做一次個人Review，時間上我沒有經驗，而且一旦下來的，這些人看不看難說得很

4. 匯總會議，記錄下所有的問題

5. （可選）作者修改代碼，然後最後做一次跟蹤，時間視問題多寡而定

6。強調：代碼Review必須在UT之前做