[PATCH] D48881: [clangd] Avoid collecting symbols from broken TUs in global-symbol-builder.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:88
+(CI.getDiagnosticClient().getNumErrors() > 0)) {
+  llvm::errs() << "Found errors in the translation unit. Igoring "
+  "collected symbols...\n";

ioeric wrote:
> ilya-biryukov wrote:
> > NIT: Maybe use clangd's `log` here? To get timestamps in the output, etc.
> The default logger doesn't seem to add timestamp? I'll keep `llvm::errs()` 
> here for consistency in this file.
> The default logger doesn't seem to add timestamp?
Ah, yes, it's the `JSONOutput` that adds timestamps Sorry.

> I'll keep llvm::errs() here for consistency in this file.
We write to `stderr` at the bottom of the file to show errors to the user. 
This instance looks much more like a logging routine.

Since our dependencies (e.g. `SymbolCollector`) use logs, I'd argue it's 
actually **more** consistent across the program to use `log` here as well. E.g. 
if someone adds a `LoggingSession` that writes timestamps, not only 
SymbolCollector logs should start showing those, but this one as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D48881



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48881: [clangd] Avoid collecting symbols from broken TUs in global-symbol-builder.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL336252: [clangd] Avoid collecting symbols from broken TUs in 
global-symbol-builder. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D48881

Files:
  
clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp


Index: 
clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
===
--- 
clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
+++ 
clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
@@ -82,6 +82,14 @@
   void EndSourceFileAction() override {
 WrapperFrontendAction::EndSourceFileAction();
 
+const auto  = getCompilerInstance();
+if (CI.hasDiagnostics() &&
+(CI.getDiagnosticClient().getNumErrors() > 0)) {
+  llvm::errs() << "Found errors in the translation unit. Igoring "
+  "collected symbols...\n";
+  return;
+}
+
 auto Symbols = Collector->takeSymbols();
 for (const auto  : Symbols) {
   Ctx->reportResult(Sym.ID.str(), SymbolToYAML(Sym));


Index: clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
===
--- clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
+++ clang-tools-extra/trunk/clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
@@ -82,6 +82,14 @@
   void EndSourceFileAction() override {
 WrapperFrontendAction::EndSourceFileAction();
 
+const auto  = getCompilerInstance();
+if (CI.hasDiagnostics() &&
+(CI.getDiagnosticClient().getNumErrors() > 0)) {
+  llvm::errs() << "Found errors in the translation unit. Igoring "
+  "collected symbols...\n";
+  return;
+}
+
 auto Symbols = Collector->takeSymbols();
 for (const auto  : Symbols) {
   Ctx->reportResult(Sym.ID.str(), SymbolToYAML(Sym));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48881: [clangd] Avoid collecting symbols from broken TUs in global-symbol-builder.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:88
+(CI.getDiagnosticClient().getNumErrors() > 0)) {
+  llvm::errs() << "Found errors in the translation unit. Igoring "
+  "collected symbols...\n";

ilya-biryukov wrote:
> NIT: Maybe use clangd's `log` here? To get timestamps in the output, etc.
The default logger doesn't seem to add timestamp? I'll keep `llvm::errs()` here 
for consistency in this file.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48881



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48881: [clangd] Avoid collecting symbols from broken TUs in global-symbol-builder.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM with a minor NIT




Comment at: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp:88
+(CI.getDiagnosticClient().getNumErrors() > 0)) {
+  llvm::errs() << "Found errors in the translation unit. Igoring "
+  "collected symbols...\n";

NIT: Maybe use clangd's `log` here? To get timestamps in the output, etc.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48881



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48881: [clangd] Avoid collecting symbols from broken TUs in global-symbol-builder.

[Eric Liu via Phabricator via cfe-commits]

ioeric created this revision.
ioeric added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, jkorous, MaskRay.

For example, template parameter might not be resolved in a broken TU,
which can result in wrong USR/SymbolID.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48881

Files:
  clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp


Index: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
===
--- clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
+++ clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
@@ -82,6 +82,14 @@
   void EndSourceFileAction() override {
 WrapperFrontendAction::EndSourceFileAction();
 
+const auto  = getCompilerInstance();
+if (CI.hasDiagnostics() &&
+(CI.getDiagnosticClient().getNumErrors() > 0)) {
+  llvm::errs() << "Found errors in the translation unit. Igoring "
+  "collected symbols...\n";
+  return;
+}
+
 auto Symbols = Collector->takeSymbols();
 for (const auto  : Symbols) {
   Ctx->reportResult(Sym.ID.str(), SymbolToYAML(Sym));


Index: clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
===
--- clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
+++ clangd/global-symbol-builder/GlobalSymbolBuilderMain.cpp
@@ -82,6 +82,14 @@
   void EndSourceFileAction() override {
 WrapperFrontendAction::EndSourceFileAction();
 
+const auto  = getCompilerInstance();
+if (CI.hasDiagnostics() &&
+(CI.getDiagnosticClient().getNumErrors() > 0)) {
+  llvm::errs() << "Found errors in the translation unit. Igoring "
+  "collected symbols...\n";
+  return;
+}
+
 auto Symbols = Collector->takeSymbols();
 for (const auto  : Symbols) {
   Ctx->reportResult(Sym.ID.str(), SymbolToYAML(Sym));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits