[PATCH] D7982: [clang-tidy] Add readability-duplicate-include check
Richard via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Jan 22 10:40:53 PST 2022
LegalizeAdulthood added inline comments.
================
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
----------------
carlosgalvezp wrote:
> 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! :)
When it comes to matters that I consider a style preference, I
always prioritize the style of the team/project/code base over
my own style preferences unless I'm the only person working
on the existing code base `:)`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D7982/new/
https://reviews.llvm.org/D7982
More information about the cfe-commits
mailing list