ilya-biryukov added inline comments.
================ Comment at: clangd/ClangdUnit.cpp:251 + auto Res = + std::lower_bound(MainFileIncludes.begin(), MainFileIncludes.end(), Inc, + [](const Inclusion &LHS, const Inclusion &RHS) { ---------------- NIT: use `llvm::lower_bound` ================ Comment at: clangd/ClangdUnit.cpp:252 + std::lower_bound(MainFileIncludes.begin(), MainFileIncludes.end(), Inc, + [](const Inclusion &LHS, const Inclusion &RHS) { + return LHS.R.start.line < RHS.R.start.line; ---------------- NIT: `lower_bound` also works when you work on a different type, no need for the temporary variable. ``` lower_bound(MainFileIncludes…, [](const Inclusion &L, const Position &Pos) { return L.start.line < Pos.line; }) ``` (the order of lamda parameters might need to be reversed, I always forget what's the correct one) ================ Comment at: clangd/ClangdUnit.cpp:255 + }); + return Res->R; +} ---------------- NIT: add a sanity check that the include was actually there: `assert(Res->R == Pos)` ================ Comment at: clangd/Diagnostics.cpp:55 + } + if (!IncludeStack.empty()) { + D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str(); ---------------- reduce nesting by inverting the condition: ``` if (IncludeStack.empty()) return; // rest of the code ``` ================ Comment at: clangd/Diagnostics.cpp:205 + if (I != E - 1) + OS << "In file included from: "; + OS << D.IncludeStack[I] << ": "; ---------------- Could we get the message first and the include stack later? The first line is often showed in various lists and having `In file included from` there is not very helpful. ================ Comment at: clangd/Diagnostics.cpp:463 + // header. + auto ShouldAddDiag = [this](const Diag &D) { + if (mentionsMainFile(D)) ---------------- Could you inline `ShouldAddDiag` into its single callsite? ================ Comment at: clangd/Diagnostics.h:87 + /// file is in front. + std::vector<std::string> IncludeStack; }; ---------------- Could you store `pair</*Filename*/string, Position>` instead? Would make it simpler to adapt D60267 to this change. ================ Comment at: clangd/Diagnostics.h:131 llvm::Optional<Diag> LastDiag; + // Counts diagnostics coming from a header in the main file. + llvm::DenseMap<int, int> NumberOfDiagsAtLine; ---------------- NIT: use tripple slash comments ================ Comment at: clangd/Diagnostics.h:132 + // Counts diagnostics coming from a header in the main file. + llvm::DenseMap<int, int> NumberOfDiagsAtLine; }; ---------------- NIT: the name makes me believe this does a different thing. Maybe mention "include" in the name to make it less confusing? Something like `DiagsInsideIncludes` would work. Another NIT: we don't need a map anymore: `DenseSet<int>` should be enough ================ Comment at: unittests/clangd/DiagnosticsTests.cpp:54 + return false; + for (size_t I = 0, E = IncludeStack.size(); I < E; ++I) + if (IncludeStack[I] != arg.IncludeStack[I]) ---------------- NIT: maybe simplify to `std::equal(IncludeStack.begin(), IncludeStack.end(), arg.IncludeStack.begin())` ================ Comment at: unittests/clangd/DiagnosticsTests.cpp:620 +ParsedAST build(const std::string &MainFile, + const llvm::StringMap<std::string> &Files) { ---------------- We were discussing adding the extra files to `TestTU` instead. Any reason this did not work out? 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 https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits