aaron.ballman added a comment.
LGTM assuming precommit CI doesn't discover any surprises, but hopefully
@njames93 or @carlosgalvezp can weigh in as well since they've been in this
code more recently than I have.
================
Comment at:
clang-tools-extra/unittests/clang-tidy/ReadabilityModuleTest.cpp:569-571
+ runPragmaOnceCheck("#ifndef BOTH_H\n"
+ "#define BOTH_H\n"
+ "#pragma once\n"
----------------
KyleFromKitware wrote:
> aaron.ballman wrote:
> > How about a test for:
> > ```
> > #if !defined(HEADER_GUARD_H)
> > #define HEADER_GUARD_H 1
> >
> > #endif
> > ```
> > and a test for what happens when there's no header guard or pragma once in
> > the file.
> > How about a test for:
> > ```
> > #if !defined(HEADER_GUARD_H)
> > #define HEADER_GUARD_H 1
> >
> > #endif
> > ```
>
> Good catch. The case of `!defined()` was not being handled by the existing
> header guard check except to basically ignore it. The `#pragma once` check
> crashed on this case. I've updated it to also ignore the `!defined()` case.
>
> > and a test for what happens when there's no header guard or pragma once in
> > the file.
>
> I already have this, see the "neither" test at the bottom.
>> and a test for what happens when there's no header guard or pragma once in
>> the file.
> I already have this, see the "neither" test at the bottom.
Ah! I was misreading:
```
EXPECT_EQ("#pragma once\n"
"void neither();\n",
runPragmaOnceCheck("void neither();\n", "neither.h",
StringRef("use #pragma once")));
```
as having the pragma once in there, which is backwards. Thanks!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142123/new/
https://reviews.llvm.org/D142123
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
