[PATCH] D7982: [clang-tidy] Add readability-duplicate-include check

Carlos Galvez via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Jan 21 23:37:59 PST 2022

carlosgalvezp requested changes to this revision.
carlosgalvezp added inline comments.
This revision now requires changes to proceed.

Comment at: clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include.h:1
LegalizeAdulthood wrote:
> carlosgalvezp wrote:
> > Nit: `#ifndef` 
> It's a style thing.  I don't prefer `#ifndef` because it only differs from `#ifdef` by a single letter.
You make a very valid point and I would definitely agree with that for "regular" code.

However we are talking about a very special case here - header guards. Header guards have a de-facto standard based on `#ifndef/#define`. It's actually less error prone to write it like that - a well-written header guard will have perfect visual alignment:

#ifndef FOO_H
#define FOO_H

If you missed the `n` in `#ifndef`, you would notice the misalignment immediately and know what to fix.

#ifdef FOO_H
#define FOO_H

This also helps making sure that the macro name is identical on both lines. This visual alignment is broken when using `#if !defined`, which makes it harder to detect these problems.

Furthermore, I have run a quick search in the whole repo and I cannot find one single instance where a header guard is written in the way that you propose here.

$ git grep -E "#ifndef[ ]+[A-Z_]+_H" | wc -l

$ git grep -E "#if[ ]+\!defined\([A-Z_]+_H\)" | wc -l
All such 7 instances are exclusively used for error handling or other logic, not for defining header guards.

Therefore I don't see this as a style choice, but rather a repo-wide convention that shall be followed, and so I feel it's my duty as reviewer to request changes. 

I understand this is annoying after 7 years waiting for a patch, but I think it's a very easy fix to do. I can approve the patch right away after this if fixed. Thank you for your patience! :) 



More information about the cfe-commits mailing list