[PATCH] D106201: [clangd] Add tests covering existing header-guard behavior. NFC

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:725
+  TU.Code = R"cpp(
+    #pragma once
+    ;
----------------
kadircet wrote:
> ```
> #include "self.h"
> #pragma once
> ```
> 
> might also be an interesting case (with preamble/main file split variations). I think all of these should raise a warning for sure, I don't think we should mark these as pragma guarded. (interestingly clangd actually somewhat works on this case today, but it feels like an accident and this code won't actually compile, so I don't think preserving clangd's current behviour would be beneficial to anyone).
Done, but only with a couple of splits, as I don't think we can cover all these edge cases exhaustively and the main behavior (diagnostic) is kinda obvious.

> I don't think we should mark these as pragma guarded

I can't see any principled reason (or way) to make them not pragma guarded.
`#pragma once` at the end of a file is a perfectly valid header guard and not observably different from having it at the top unless the file transitively includes itself.


================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:804
+  // The diagnostic is unfortunate in this case, but correct per our model.
+  // Ultimately the include is skipped and the code is parsed correctly though.
+  EXPECT_THAT(*AST.getDiagnostics(),
----------------
kadircet wrote:
> but this is actually wrong from compiler's perspective, right ? if user wanted to compile implementation file they would hit redefinition errors. i think we should expect a header guard/pragma once on the implementation file on the common case.
The compiler doesn't really have a perspective on this code, nobody ever tries to compile impl.h (and we probably don't have a real non-inferred compile command for it).

The include guard isn't required for the compiler/build system, as long as impl.h is only ever included from iface.h and the latter is guarded. And including impl.h directly usually won't compile (guarded or not) so there's no great reason to include-guard impl.h apart from tools.

This is common I think, and seems principled:
```
// impl.h
#ifndef IFACE_H // include guard from another file!
#error Do not include this directly!
#endif
// no include guard
// actual impl follows
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106201