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

[Carlos Galvez via Phabricator via cfe-commits]

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
+#if !defined(READABILITY_DUPLICATE_INCLUDE_H)
+#define READABILITY_DUPLICATE_INCLUDE_H
----------------
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
9573

$ git grep -E "#if[ ]+\!defined\([A-Z_]+_H\)" | wc -l
7
```
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! :) 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D7982/new/

https://reviews.llvm.org/D7982