Reviewing a Peer’s Optimization

A Code Review Summary for SPO600

For the final project of the Software Portability Course, the class was tasked with reviewing the code of a peer who’d set up a pull request for Chris’ GLIBC repository. For my code review, I decided my good friend John would be a worthy candidate for my review musings, his pull request being found here for those interested in viewing.

There were a few stylistic issues that I brought up, all of which a simple fix would remedy.

The Code

The Stylistic Issues In Question

Throughout the GLIBC, a common coding convention can be found throughout the familiar and obscure files. In such convention, is the inclusion of a space between the function name and arguments. John’s editor perhaps did not detect this, and instead replaced all of his code with the common function sans-space argument arrangement.

As you can see below, the issue is a minor one which is easily overlooked.

Coding Convention Issue #1: Correct

Coding Convention Issue #1: Incorrect

Another convention issue, was the rearranging of declaration syntax for variables and functions. I won’t deny, the GLIBC’s coding style is unfamiliar to someone who’s experience with C derives from few courses, and I did question why that style of C syntax was deemed essential for the time. Perhaps to reduce line length? This idea does make sense on the low-resolution terminals utilized in the days of old, but does look rather odd to the modern programmer.

Coding Convention Issue #2: Correct

Coding Convention Issue #2: Incorrect

Conclusion

John’s optimizations enable support for 64-bit processing, which is a big improvement to modern servers & programs alike. Having gone through his code modifications for the review, I did not see any issues regarding logic or operations, which in the end will always be the make-it / break-it factor. He did damn good work with the optimization, and with the stylistic change I’ve mentioned above, I think the upstream developers will accept his code contribution without hesitation.

 

Leave a Reply

Your email address will not be published. Required fields are marked *