[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

This revision was automatically updated to reflect the committed changes. Closed by commit rL359432: [clangd] Surface diagnostics from headers inside main file (authored by kadircet, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Repository: rL LLVM

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet updated this revision to Diff 197077. kadircet added a comment. - Rebase Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/ https://reviews.llvm.org/D59302 Files: clangd/Diagnostics.cpp clangd/Diagnostics.h

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet updated this revision to Diff 197073. kadircet marked 8 inline comments as done. kadircet added a comment. - Address comments Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/ https://reviews.llvm.org/D59302 Files:

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Nice! Just readability/wording nits Comment at: clangd/Diagnostics.cpp:463 + // header. + auto ShouldAddDiag = [this](const Diag ) { +if (mentionsMainFile(D))

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet updated this revision to Diff 197068. kadircet marked 2 inline comments as done. kadircet added a comment. - Address comments Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/ https://reviews.llvm.org/D59302 Files:

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added inline comments. Comment at: clangd/Diagnostics.cpp:115 + const SourceManager = Info.getSourceManager(); + std::vector IncludeStack; + auto GetIncludeLoc = [](SourceLocation SLoc) { replace `vector` with `SourceLocation` now that we only

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added a comment. This looks nice and minimal, thanks! Happy to LGTM this, unless @sammccall want to take another look (e.g. to account for related information patch) Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet updated this revision to Diff 196822. kadircet marked 5 inline comments as done. kadircet added a comment. - Address comments - Drop include stack Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/ https://reviews.llvm.org/D59302

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet added inline comments. Comment at: clangd/Diagnostics.cpp:78 +// offsets when displaying that information to users. +Position toOneBased(Position P) { + ++P.line; ilya-biryukov wrote: > Could we avoid introducing a function that breaks the invariant of

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added inline comments. Comment at: clangd/Diagnostics.cpp:78 +// offsets when displaying that information to users. +Position toOneBased(Position P) { + ++P.line; Could we avoid introducing a function that breaks the invariant of a type? Having a

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet added inline comments. Comment at: unittests/clangd/DiagnosticsTests.cpp:620 +ParsedAST build(const std::string , +const llvm::StringMap ) { ilya-biryukov wrote: > We were discussing adding the extra files to `TestTU` instead. > Any

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet updated this revision to Diff 195375. kadircet marked 14 inline comments as done. kadircet added a comment. - Address comments Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/ https://reviews.llvm.org/D59302 Files:

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:251 + auto Res = + std::lower_bound(MainFileIncludes.begin(), MainFileIncludes.end(), Inc, + [](const Inclusion , const Inclusion ) { NIT: use `llvm::lower_bound`

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added a comment. Sorry, lost this revision. Looking now Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/ https://reviews.llvm.org/D59302 ___ cfe-commits mailing list

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet added a comment. Ping Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/ https://reviews.llvm.org/D59302 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet updated this revision to Diff 194528. kadircet marked 10 inline comments as done. kadircet added a comment. - Address comments Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/ https://reviews.llvm.org/D59302 Files:

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet added inline comments. Comment at: clangd/Diagnostics.cpp:396 + for (llvm::StringRef Inc : IncludeStack) +OS << "In file included from: " << Inc << ":\n"; +} ilya-biryukov wrote: > kadircet wrote: > > ilya-biryukov wrote: >

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:405 - std::vector Diags = ASTDiags.take(); + auto ShouldSurfaceDiag = [](const Diag ) { +if (D.Severity == DiagnosticsEngine::Level::Error || The name suggests we filter **all**

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet updated this revision to Diff 191831. kadircet marked 8 inline comments as done. kadircet added a comment. - Address comments Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/ https://reviews.llvm.org/D59302 Files:

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet added inline comments. Comment at: clangd/Diagnostics.cpp:396 + for (llvm::StringRef Inc : IncludeStack) +OS << "In file included from: " << Inc << ":\n"; +} ilya-biryukov wrote: > NIT: could we reuse the function from clang

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added a comment. Thanks for changes. A few more comments. Comment at: clangd/ClangdUnit.cpp:259 +// Generates a mapping from start of an include directive to its range. +std::map +positionToRangeMap(const std::vector ) { Could we binary-search in

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet updated this revision to Diff 191465. kadircet marked an inline comment as done. kadircet added a comment. - Make limit a hardcoded value rather then a command line argument - Limit diags per include directive Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet planned changes to this revision. kadircet added a comment. - Limit per include directive - Use hardcoded value for limit Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/ https://reviews.llvm.org/D59302

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet updated this revision to Diff 191323. kadircet marked 2 inline comments as done. kadircet added a comment. - Show include stack in diagnostic message - Point to exact include location Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added inline comments. Comment at: clangd/ClangdUnit.cpp:256 +// directive inside main file. +// If a header is transitively included in multiple direct includes of main, we +// choose the first one. We should find a way to point into an exact

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added a comment. In D59302#1431274 , @kadircet wrote: > The optional part is rather limiting number of diagnostics that is going to > be surfaced. So if you have thousands of errors inside preamble only first > 100 of them will appear

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet updated this revision to Diff 191265. kadircet added a comment. - Only surface diagnostics with level fatal or error Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59302/new/ https://reviews.llvm.org/D59302 Files: clangd/ClangdServer.cpp

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet added a comment. In D59302#1431045 , @ilya-biryukov wrote: > I'll look more closely into the details, but just a high-level question now: > why would we want to make this optional and not simply surface these extra > diagnostics? The

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

ilya-biryukov added a comment. I'll look more closely into the details, but just a high-level question now: why would we want to make this optional and not simply surface these extra diagnostics? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION

[PATCH] D59302: [clangd] Surface diagnostics from headers inside main file

kadircet created this revision. kadircet added reviewers: ioeric, ilya-biryukov. Herald added subscribers: cfe-commits, jdoerfert, arphaman, jkorous, MaskRay. Herald added a project: clang. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D59302 Files: clangd/ClangdServer.cpp