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