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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

i mostly agree with the desired behaviours laid out by the tests, mentioned a coupe extra cases and wrinkly looking parts in comments.



================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:724
+
+  TU.Code = R"cpp(
+    #pragma once
----------------
i think we want the same behaviour for

```
;
#pragma once
#include "self.h"
```


================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:725
+  TU.Code = R"cpp(
+    #pragma once
+    ;
----------------
```
#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).


================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:733
+
+  TU.Code = R"cpp(
+    #ifndef GUARD
----------------
what about
```
#include "self.h"
#ifndef GUARD
#define GUARD
;
#endif
```

I suppose this shouldn't be header guarded and raise diags


================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:745
+
+  TU.Code = R"cpp(
+    #ifndef GUARD
----------------
similar to above might be nice checking following isn't guarded and doesn't raise diags (as we won't include main file infinitely many times).
```
;
#ifndef GUARD
#define GUARD
#include "self.h"
#endif
```


================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:759
+// into interface and implementation files (e.g. *.h vs *.inl).
+// There files mutually include each other, and need careful handling of include
+// guards (which interact with preambles).
----------------
s/There/These


================
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(),
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106201