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