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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
