[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL363067: [libclang] Allow skipping warnings from all included 
files (authored by nik, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D48116?vs=199015=204060#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D48116

Files:
  cfe/trunk/include/clang-c/Index.h
  cfe/trunk/include/clang/Frontend/ASTUnit.h
  cfe/trunk/lib/Frontend/ASTUnit.cpp
  cfe/trunk/test/Index/ignore-warnings-from-headers.cpp
  cfe/trunk/test/Index/ignore-warnings-from-headers.h
  cfe/trunk/tools/c-index-test/c-index-test.c
  cfe/trunk/tools/c-index-test/core_main.cpp
  cfe/trunk/tools/libclang/CIndex.cpp
  cfe/trunk/tools/libclang/Indexing.cpp
  cfe/trunk/unittests/Frontend/ASTUnitTest.cpp
  cfe/trunk/unittests/Frontend/PCHPreambleTest.cpp

Index: cfe/trunk/lib/Frontend/ASTUnit.cpp
===
--- cfe/trunk/lib/Frontend/ASTUnit.cpp
+++ cfe/trunk/lib/Frontend/ASTUnit.cpp
@@ -608,17 +608,20 @@
 };
 
 /// Diagnostic consumer that saves each diagnostic it is given.
-class StoredDiagnosticConsumer : public DiagnosticConsumer {
+class FilterAndStoreDiagnosticConsumer : public DiagnosticConsumer {
   SmallVectorImpl *StoredDiags;
   SmallVectorImpl *StandaloneDiags;
+  bool CaptureNonErrorsFromIncludes = true;
   const LangOptions *LangOpts = nullptr;
   SourceManager *SourceMgr = nullptr;
 
 public:
-  StoredDiagnosticConsumer(
+  FilterAndStoreDiagnosticConsumer(
   SmallVectorImpl *StoredDiags,
-  SmallVectorImpl *StandaloneDiags)
-  : StoredDiags(StoredDiags), StandaloneDiags(StandaloneDiags) {
+  SmallVectorImpl *StandaloneDiags,
+  bool CaptureNonErrorsFromIncludes)
+  : StoredDiags(StoredDiags), StandaloneDiags(StandaloneDiags),
+CaptureNonErrorsFromIncludes(CaptureNonErrorsFromIncludes) {
 assert((StoredDiags || StandaloneDiags) &&
"No output collections were passed to StoredDiagnosticConsumer.");
   }
@@ -634,21 +637,25 @@
 const Diagnostic ) override;
 };
 
-/// RAII object that optionally captures diagnostics, if
+/// RAII object that optionally captures and filters diagnostics, if
 /// there is no diagnostic client to capture them already.
 class CaptureDroppedDiagnostics {
   DiagnosticsEngine 
-  StoredDiagnosticConsumer Client;
+  FilterAndStoreDiagnosticConsumer Client;
   DiagnosticConsumer *PreviousClient = nullptr;
   std::unique_ptr OwningPreviousClient;
 
 public:
   CaptureDroppedDiagnostics(
-  bool RequestCapture, DiagnosticsEngine ,
+  CaptureDiagsKind CaptureDiagnostics, DiagnosticsEngine ,
   SmallVectorImpl *StoredDiags,
   SmallVectorImpl *StandaloneDiags)
-  : Diags(Diags), Client(StoredDiags, StandaloneDiags) {
-if (RequestCapture || Diags.getClient() == nullptr) {
+  : Diags(Diags),
+Client(StoredDiags, StandaloneDiags,
+   CaptureDiagnostics !=
+   CaptureDiagsKind::AllWithoutNonErrorsFromIncludes) {
+if (CaptureDiagnostics != CaptureDiagsKind::None ||
+Diags.getClient() == nullptr) {
   OwningPreviousClient = Diags.takeClient();
   PreviousClient = Diags.getClient();
   Diags.setClient(, false);
@@ -667,8 +674,16 @@
 makeStandaloneDiagnostic(const LangOptions ,
  const StoredDiagnostic );
 
-void StoredDiagnosticConsumer::HandleDiagnostic(DiagnosticsEngine::Level Level,
-const Diagnostic ) {
+static bool isInMainFile(const clang::Diagnostic ) {
+  if (!D.hasSourceManager() || !D.getLocation().isValid())
+return false;
+
+  auto  = D.getSourceManager();
+  return M.isWrittenInMainFile(M.getExpansionLoc(D.getLocation()));
+}
+
+void FilterAndStoreDiagnosticConsumer::HandleDiagnostic(
+DiagnosticsEngine::Level Level, const Diagnostic ) {
   // Default implementation (Warnings/errors count).
   DiagnosticConsumer::HandleDiagnostic(Level, Info);
 
@@ -676,6 +691,11 @@
   // about. This effectively drops diagnostics from modules we're building.
   // FIXME: In the long run, ee don't want to drop source managers from modules.
   if (!Info.hasSourceManager() || () == SourceMgr) {
+if (!CaptureNonErrorsFromIncludes && Level <= DiagnosticsEngine::Warning &&
+!isInMainFile(Info)) {
+  return;
+}
+
 StoredDiagnostic *ResultDiag = nullptr;
 if (StoredDiags) {
   StoredDiags->emplace_back(Level, Info);
@@ -723,10 +743,13 @@
 
 /// Configure the diagnostics object for use with ASTUnit.
 void ASTUnit::ConfigureDiags(IntrusiveRefCntPtr Diags,
- ASTUnit , bool CaptureDiagnostics) {
+ ASTUnit ,
+ CaptureDiagsKind CaptureDiagnostics) {
   assert(Diags.get() && "no 

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan accepted this revision.
yvvan added a comment.
This revision is now accepted and ready to land.

libclang part is quite small here and looks ok. I would just accept it


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

@jkorous, could you take a look or suggest someone who can review the libclang 
changes?
As mentioned before, I've reviewed the clang bits (which are now gone), but 
don't have much experience in `libclang` parts.


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Jan?


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Sorry for the pointless ping, haven't seen the inline comments. They are 
addressed now.

I've also increased CINDEX_VERSION_MINOR so clients can detect availability of 
this new flag.

> It's been a while since I've looked at the ASTUnit code, though, would be 
> good if someone else took an extra look at it.

Benjamin? Argyrios? Would you be so kind? :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik updated this revision to Diff 199015.
nik marked 2 inline comments as done.
nik added a comment.

Addressed inline comments.


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116

Files:
  include/clang-c/Index.h
  include/clang/Frontend/ASTUnit.h
  lib/Frontend/ASTUnit.cpp
  test/Index/ignore-warnings-from-headers.cpp
  test/Index/ignore-warnings-from-headers.h
  tools/c-index-test/c-index-test.c
  tools/c-index-test/core_main.cpp
  tools/libclang/CIndex.cpp
  tools/libclang/Indexing.cpp
  unittests/Frontend/ASTUnitTest.cpp
  unittests/Frontend/PCHPreambleTest.cpp

Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -96,8 +96,8 @@
 FileManager *FileMgr = new FileManager(FSOpts, VFS);
 
 std::unique_ptr AST = ASTUnit::LoadFromCompilerInvocation(
-  CI, PCHContainerOpts, Diags, FileMgr, false, false,
-  /*PrecompilePreambleAfterNParses=*/1);
+CI, PCHContainerOpts, Diags, FileMgr, false, CaptureDiagsKind::None,
+/*PrecompilePreambleAfterNParses=*/1);
 return AST;
   }
 
Index: unittests/Frontend/ASTUnitTest.cpp
===
--- unittests/Frontend/ASTUnitTest.cpp
+++ unittests/Frontend/ASTUnitTest.cpp
@@ -51,8 +51,8 @@
 PCHContainerOps = std::make_shared();
 
 return ASTUnit::LoadFromCompilerInvocation(
-CInvok, PCHContainerOps, Diags, FileMgr, false, false, 0, TU_Complete,
-false, false, isVolatile);
+CInvok, PCHContainerOps, Diags, FileMgr, false, CaptureDiagsKind::None,
+0, TU_Complete, false, false, isVolatile);
   }
 };
 
Index: tools/libclang/Indexing.cpp
===
--- tools/libclang/Indexing.cpp
+++ tools/libclang/Indexing.cpp
@@ -443,10 +443,14 @@
   if (CXXIdx->isOptEnabled(CXGlobalOpt_ThreadBackgroundPriorityForIndexing))
 setThreadBackgroundPriority();
 
-  bool CaptureDiagnostics = !Logger::isLoggingEnabled();
+  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::All;
+  if (TU_options & CXTranslationUnit_IgnoreNonErrorsFromIncludedFiles)
+CaptureDiagnostics = CaptureDiagsKind::AllWithoutNonErrorsFromIncludes;
+  if (Logger::isLoggingEnabled())
+CaptureDiagnostics = CaptureDiagsKind::None;
 
   CaptureDiagnosticConsumer *CaptureDiag = nullptr;
-  if (CaptureDiagnostics)
+  if (CaptureDiagnostics != CaptureDiagsKind::None)
 CaptureDiag = new CaptureDiagnosticConsumer();
 
   // Configure the diagnostics.
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -3352,7 +3352,7 @@
   ASTUnit::LoadEverything, Diags,
   FileSystemOpts, /*UseDebugInfo=*/false,
   CXXIdx->getOnlyLocalDecls(), None,
-  /*CaptureDiagnostics=*/true,
+  CaptureDiagsKind::All,
   /*AllowPCHWithCompilerErrors=*/true,
   /*UserFilesAreVolatile=*/true);
   *out_TU = MakeCXTranslationUnit(CXXIdx, std::move(AU));
@@ -3425,6 +3425,10 @@
   if (options & CXTranslationUnit_KeepGoing)
 Diags->setFatalsAsError(true);
 
+  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::All;
+  if (options & CXTranslationUnit_IgnoreNonErrorsFromIncludedFiles)
+CaptureDiagnostics = CaptureDiagsKind::AllWithoutNonErrorsFromIncludes;
+
   // Recover resources if we crash before exiting this function.
   llvm::CrashRecoveryContextCleanupRegistrar >
@@ -3502,7 +3506,7 @@
   Args->data(), Args->data() + Args->size(),
   CXXIdx->getPCHContainerOperations(), Diags,
   CXXIdx->getClangResourcesPath(), CXXIdx->getOnlyLocalDecls(),
-  /*CaptureDiagnostics=*/true, *RemappedFiles.get(),
+  CaptureDiagnostics, *RemappedFiles.get(),
   /*RemappedFilesKeepOriginalName=*/true, PrecompilePreambleAfterNParses,
   TUKind, CacheCodeCompletionResults, IncludeBriefCommentsInCodeCompletion,
   /*AllowPCHWithCompilerErrors=*/true, SkipFunctionBodies, SingleFileParse,
Index: tools/c-index-test/core_main.cpp
===
--- tools/c-index-test/core_main.cpp
+++ tools/c-index-test/core_main.cpp
@@ -264,7 +264,7 @@
   modulePath, *pchRdr, ASTUnit::LoadASTOnly, Diags,
   FileSystemOpts, /*UseDebugInfo=*/false,
   /*OnlyLocalDecls=*/true, None,
-  /*CaptureDiagnostics=*/false,
+  CaptureDiagsKind::None,
   /*AllowPCHWithCompilerErrors=*/true,
   /*UserFilesAreVolatile=*/false);
   if (!AU) {
Index: tools/c-index-test/c-index-test.c
===
--- tools/c-index-test/c-index-test.c
+++ tools/c-index-test/c-index-test.c
@@ -88,6 +88,8 @@
 options |= CXTranslationUnit_IncludeAttributedTypes;
   if 

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik marked 4 inline comments as done.
nik added inline comments.
Herald added a subscriber: dexonsmith.



Comment at: lib/Frontend/ASTUnit.cpp:682
+  auto  = D.getSourceManager();
+  return M.isInMainFile(M.getExpansionLoc(D.getLocation()));
+}

ilya-biryukov wrote:
> `isWrittenInMainFile` might be a better fit: it does not look at presumed 
> locations. That would be the expected behavior in the most common case, i.e. 
> showing errors in an IDE or a text editor.
Oh, good catch! Thanks! :)



Comment at: lib/Frontend/ASTUnit.cpp:694
+  if ((!Info.hasSourceManager() || () == SourceMgr) &&
+  (StoredDiags || StandaloneDiags)) {
+if (!CaptureNonErrorsFromIncludes

ilya-biryukov wrote:
> Why do we need this extra check? We checked for StoredDiags and Standalone 
> diags in the body of the statement later again.
Ops, looks like a left-over from a previous version, removed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

This LGTM wrt to layering, i.e. it's nice we don't need to change the code of 
diagnostics.
It's been a while since I've looked at the ASTUnit code, though, would be good 
if someone else took an extra look at it.

Added a few nits too.




Comment at: lib/Frontend/ASTUnit.cpp:682
+  auto  = D.getSourceManager();
+  return M.isInMainFile(M.getExpansionLoc(D.getLocation()));
+}

`isWrittenInMainFile` might be a better fit: it does not look at presumed 
locations. That would be the expected behavior in the most common case, i.e. 
showing errors in an IDE or a text editor.



Comment at: lib/Frontend/ASTUnit.cpp:694
+  if ((!Info.hasSourceManager() || () == SourceMgr) &&
+  (StoredDiags || StandaloneDiags)) {
+if (!CaptureNonErrorsFromIncludes

Why do we need this extra check? We checked for StoredDiags and Standalone 
diags in the body of the statement later again.


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik updated this revision to Diff 187318.
nik added a comment.
Herald added a subscriber: jdoerfert.

OK, filtering happens now in FilterAndStoreDiagnosticConsumer, the former
StoredDiagnosticConsumer.


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116

Files:
  include/clang-c/Index.h
  include/clang/Frontend/ASTUnit.h
  lib/Frontend/ASTUnit.cpp
  test/Index/ignore-warnings-from-headers.cpp
  test/Index/ignore-warnings-from-headers.h
  tools/c-index-test/c-index-test.c
  tools/c-index-test/core_main.cpp
  tools/libclang/CIndex.cpp
  tools/libclang/Indexing.cpp
  unittests/Frontend/ASTUnitTest.cpp
  unittests/Frontend/PCHPreambleTest.cpp

Index: unittests/Frontend/PCHPreambleTest.cpp
===
--- unittests/Frontend/PCHPreambleTest.cpp
+++ unittests/Frontend/PCHPreambleTest.cpp
@@ -96,8 +96,8 @@
 FileManager *FileMgr = new FileManager(FSOpts, VFS);
 
 std::unique_ptr AST = ASTUnit::LoadFromCompilerInvocation(
-  CI, PCHContainerOpts, Diags, FileMgr, false, false,
-  /*PrecompilePreambleAfterNParses=*/1);
+CI, PCHContainerOpts, Diags, FileMgr, false, CaptureDiagsKind::None,
+/*PrecompilePreambleAfterNParses=*/1);
 return AST;
   }
 
Index: unittests/Frontend/ASTUnitTest.cpp
===
--- unittests/Frontend/ASTUnitTest.cpp
+++ unittests/Frontend/ASTUnitTest.cpp
@@ -51,8 +51,8 @@
 PCHContainerOps = std::make_shared();
 
 return ASTUnit::LoadFromCompilerInvocation(
-CInvok, PCHContainerOps, Diags, FileMgr, false, false, 0, TU_Complete,
-false, false, isVolatile);
+CInvok, PCHContainerOps, Diags, FileMgr, false, CaptureDiagsKind::None,
+0, TU_Complete, false, false, isVolatile);
   }
 };
 
Index: tools/libclang/Indexing.cpp
===
--- tools/libclang/Indexing.cpp
+++ tools/libclang/Indexing.cpp
@@ -443,10 +443,14 @@
   if (CXXIdx->isOptEnabled(CXGlobalOpt_ThreadBackgroundPriorityForIndexing))
 setThreadBackgroundPriority();
 
-  bool CaptureDiagnostics = !Logger::isLoggingEnabled();
+  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::All;
+  if (TU_options & CXTranslationUnit_IgnoreNonErrorsFromIncludedFiles)
+CaptureDiagnostics = CaptureDiagsKind::AllWithoutNonErrorsFromIncludes;
+  if (Logger::isLoggingEnabled())
+CaptureDiagnostics = CaptureDiagsKind::None;
 
   CaptureDiagnosticConsumer *CaptureDiag = nullptr;
-  if (CaptureDiagnostics)
+  if (CaptureDiagnostics != CaptureDiagsKind::None)
 CaptureDiag = new CaptureDiagnosticConsumer();
 
   // Configure the diagnostics.
Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -3338,7 +3338,7 @@
   ASTUnit::LoadEverything, Diags,
   FileSystemOpts, /*UseDebugInfo=*/false,
   CXXIdx->getOnlyLocalDecls(), None,
-  /*CaptureDiagnostics=*/true,
+  CaptureDiagsKind::All,
   /*AllowPCHWithCompilerErrors=*/true,
   /*UserFilesAreVolatile=*/true);
   *out_TU = MakeCXTranslationUnit(CXXIdx, std::move(AU));
@@ -3411,6 +3411,10 @@
   if (options & CXTranslationUnit_KeepGoing)
 Diags->setSuppressAfterFatalError(false);
 
+  CaptureDiagsKind CaptureDiagnostics = CaptureDiagsKind::All;
+  if (options & CXTranslationUnit_IgnoreNonErrorsFromIncludedFiles)
+CaptureDiagnostics = CaptureDiagsKind::AllWithoutNonErrorsFromIncludes;
+
   // Recover resources if we crash before exiting this function.
   llvm::CrashRecoveryContextCleanupRegistrar >
@@ -3488,7 +3492,7 @@
   Args->data(), Args->data() + Args->size(),
   CXXIdx->getPCHContainerOperations(), Diags,
   CXXIdx->getClangResourcesPath(), CXXIdx->getOnlyLocalDecls(),
-  /*CaptureDiagnostics=*/true, *RemappedFiles.get(),
+  CaptureDiagnostics, *RemappedFiles.get(),
   /*RemappedFilesKeepOriginalName=*/true, PrecompilePreambleAfterNParses,
   TUKind, CacheCodeCompletionResults, IncludeBriefCommentsInCodeCompletion,
   /*AllowPCHWithCompilerErrors=*/true, SkipFunctionBodies, SingleFileParse,
Index: tools/c-index-test/core_main.cpp
===
--- tools/c-index-test/core_main.cpp
+++ tools/c-index-test/core_main.cpp
@@ -264,7 +264,7 @@
   modulePath, *pchRdr, ASTUnit::LoadASTOnly, Diags,
   FileSystemOpts, /*UseDebugInfo=*/false,
   /*OnlyLocalDecls=*/true, None,
-  /*CaptureDiagnostics=*/false,
+  CaptureDiagsKind::None,
   /*AllowPCHWithCompilerErrors=*/true,
   /*UserFilesAreVolatile=*/false);
   if (!AU) {
Index: tools/c-index-test/c-index-test.c
===
--- tools/c-index-test/c-index-test.c
+++ tools/c-index-test/c-index-test.c
@@ -88,6 +88,8 @@
 options |= 

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.
Herald added a project: clang.

>> For filtering in StoredDiagnosticConsumer one needs to pass the new bool 
>> everywhere along where "bool CaptureDiagnostics" is already passed on (to 
>> end up in the StoredDiagnosticConsumer constructor) . Also, 
>> ASTUnit::CaptureDiagnostics is then not enough anymore since the new bool is 
>> also needed in getMainBufferWithPrecompiledPreamble(). One could also (2) 
>> convert  "bool CaptureDiagnostics" to an enum with enumerators like 
>> CaptureNothing, CaptureAll, CaptureAllWithoutNonErrorsFromIncludes to make 
>> this a bit less invasive.
>>  If changing clang's diagnostic interface should be avoided, I tend to go 
>> with (2). Ilya?
> 
> Yeah, LG. The changes in the `ASTUnit` look strictly better than changes in 
> clang - the latter seems to already provide enough to do the filtering.
>  If you avoid changing the `StoredDiagnosticConsumer` (or writing a filtering 
> wrapper for `DiagnosticConsumer`), you'll end up having some diagnostics 
> inside headers generated **after** preamble was built, right?

If there is some #include after the first declaration, possibly. Why is that 
relevant?


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D48116#1322878 , @nik wrote:

> In D48116#1144732 , @ilya-biryukov 
> wrote:
>
> > Have you considered doing the same filtering in ASTUnit's 
> > `StoredDiagnosticConsumer`? It should not be more difficult and allows to 
> > avoid changing the clang's diagnostic interfaces. That's what we do in 
> > clangd.
>
>
> Hmm, it's a bit strange that StoredDiagnosticConsumer should also start to 
> filter things.


I

> For filtering in StoredDiagnosticConsumer one needs to pass the new bool 
> everywhere along where "bool CaptureDiagnostics" is already passed on (to end 
> up in the StoredDiagnosticConsumer constructor) . Also, 
> ASTUnit::CaptureDiagnostics is then not enough anymore since the new bool is 
> also needed in getMainBufferWithPrecompiledPreamble(). One could also (2) 
> convert  "bool CaptureDiagnostics" to an enum with enumerators like 
> CaptureNothing, CaptureAll, CaptureAllWithoutNonErrorsFromIncludes to make 
> this a bit less invasive.
>  If changing clang's diagnostic interface should be avoided, I tend to go 
> with (2). Ilya?

Yeah, LG. The changes in the `ASTUnit` look strictly better than changes in 
clang - the latter seems to already provide enough to do the filtering.
If you avoid changing the `StoredDiagnosticConsumer` (or writing a filtering 
wrapper for `DiagnosticConsumer`), you'll end up having some diagnostics inside 
headers generated **after** preamble was built, right?
That might be either ok or not, depending on the use-case.


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

In D48116#1144732 , @ilya-biryukov 
wrote:

> Have you considered doing the same filtering in ASTUnit's 
> `StoredDiagnosticConsumer`? It should not be more difficult and allows to 
> avoid changing the clang's diagnostic interfaces. That's what we do in clangd.


Hmm, it's a bit strange that StoredDiagnosticConsumer should also start to 
filter things.

For filtering in StoredDiagnosticConsumer one needs to pass the new bool 
everywhere along where "bool CaptureDiagnostics" is already passed on (to end 
up in the StoredDiagnosticConsumer constructor) . Also, 
ASTUnit::CaptureDiagnostics is then not enough anymore since the new bool is 
also needed in getMainBufferWithPrecompiledPreamble(). One could also (2) 
convert  "bool CaptureDiagnostics" to an enum with enumerators like 
CaptureNothing, CaptureAll, CaptureAllWithoutNonErrorsFromIncludes to make this 
a bit less invasive.

If changing clang's diagnostic interface should be avoided, I tend to go with 
(2). Ilya?

  $ git grep "bool CaptureDiagnostics"
  include/clang/Frontend/ASTUnit.h:  bool CaptureDiagnostics = false;
  include/clang/Frontend/ASTUnit.h: ASTUnit , 
bool CaptureDiagnostics,
  include/clang/Frontend/ASTUnit.h: 
IntrusiveRefCntPtr Diags, bool CaptureDiagnostics,
  include/clang/Frontend/ASTUnit.h:  bool CaptureDiagnostics = false, bool 
AllowPCHWithCompilerErrors = false,
  include/clang/Frontend/ASTUnit.h:  bool OnlyLocalDecls = false, bool 
CaptureDiagnostics = false,
  include/clang/Frontend/ASTUnit.h:  bool OnlyLocalDecls = false, bool 
CaptureDiagnostics = false,
  include/clang/Frontend/ASTUnit.h:  bool OnlyLocalDecls = false, bool 
CaptureDiagnostics = false,
  lib/Frontend/ASTUnit.cpp: ASTUnit , bool 
CaptureDiagnostics,
  lib/Frontend/ASTUnit.cpp:bool CaptureDiagnostics, bool 
AllowPCHWithCompilerErrors,
  lib/Frontend/ASTUnit.cpp:bool CaptureDiagnostics, bool 
UserFilesAreVolatile) {
  lib/Frontend/ASTUnit.cpp:bool OnlyLocalDecls, bool CaptureDiagnostics,
  lib/Frontend/ASTUnit.cpp:bool OnlyLocalDecls, bool CaptureDiagnostics,
  lib/Frontend/ASTUnit.cpp:bool OnlyLocalDecls, bool CaptureDiagnostics,
  tools/libclang/Indexing.cpp:  bool CaptureDiagnostics = 
!Logger::isLoggingEnabled();


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.
Herald added a subscriber: arphaman.

In D48116#1144732 , @ilya-biryukov 
wrote:

> Have you considered doing the same filtering in ASTUnit's 
> `StoredDiagnosticConsumer`? It should not be more difficult and allows to 
> avoid changing the clang's diagnostic interfaces. That's what we do in clangd.


Will check.

> I wonder if you want to handle notes and remarks in a special manner? They 
> can be seen as part of the original diagnostic, rather than the separate 
> diagnostic. E.g. showing a note in the main file, but not showing the 
> diagnostic from the headers file that this note comes from, might be 
> confusing to the users.

Looks like this case is handled fine.

WIthout the new option, a note is printed as expected/always (shortened output):

  $ bin/c-index-test -test-load-source function 
ignore-warnings-from-headers.cpp -Wunused-parameter
  ignore-warnings-from-headers.h:1:12: warning: unused parameter 
'unusedInHeader' [-Wunused-parameter]
  ignore-warnings-from-headers.cpp:1:10: note: in file included from 
ignore-warnings-from-headers.cpp:1:
  ignore-warnings-from-headers.cpp:3:12: warning: unused parameter 
'unusedInMainFile' [-Wunused-parameter]

With the new option, note is not printed and thus no confusion should arise:

  $ CINDEXTEST_IGNORE_NONERRORS_FROM_INCLUDED_FILES=1 bin/c-index-test 
-test-load-source function ignore-warnings-from-headers.cpp -Wunused-parameter
  ignore-warnings-from-headers.cpp:3:12: warning: unused parameter 
'unusedInMainFile' [-Wunused-parameter]

I will add "// CHECK-NOT: note: in file included from" to the test.

> Maybe also add tests for diagnostics in the main file with notes/remarks in 
> the header files and vice versa?

"Vice versa" is covered as shown above. If the diagnostic is in main file and a 
note of that one refers to the header, then the note should be shown/included. 
This case seems also fine - I've added a test for this.

In D48116#1144838 , @yvvan wrote:

> But this one misses a way to set this flag for everything except libclang. We 
> can probably change that (Nikolai is in vacation till the end of summer so 
> it's probably my part now) and add some tests outside of Index for it 
> (probably Frontend)


What's the use case of this?


Repository:
  rC Clang

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

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

But this one misses a way to set this flag for everything except libclang. We 
can probably change that (Nikolai is in vacation till the end of summer so it's 
probably my part now) and add some tests outside of Index for it (probably 
Frontend)


Repository:
  rC Clang

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

The idea was to ignore everything including notes and remarks so that only 
errors from system headers are still collected.

Changing consumer might be a possible way to go as well but it requires 
changing (or wrapping) all consumers that we need to be affected. The advantage 
of this change that it also covers consumers that come from plugins that we use 
(tidy and clazy).


Repository:
  rC Clang

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for the late response, was on vacation.

Have you considered doing the same filtering in ASTUnit's 
`StoredDiagnosticConsumer`? It should not be more difficult and allows to avoid 
changing the clang's diagnostic interfaces. That's what we do in clangd.

I wonder if you want to handle notes and remarks in a special manner? They can 
be seen as part of the original diagnostic, rather than the separate 
diagnostic. E.g. showing a note in the main file, but not showing the 
diagnostic from the headers file that this note comes from, might be confusing 
to the users.
Maybe also add tests for diagnostics in the main file with notes/remarks in the 
header files and vice versa?


Repository:
  rC Clang

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

@ilya-biryukov what do you think?


Repository:
  rC Clang

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik updated this revision to Diff 151116.
nik added a comment.

Addressed comments.


Repository:
  rC Clang

https://reviews.llvm.org/D48116

Files:
  include/clang-c/Index.h
  include/clang/Basic/Diagnostic.h
  lib/Basic/DiagnosticIDs.cpp
  test/Index/ignore-warnings-from-headers.cpp
  test/Index/ignore-warnings-from-headers.h
  tools/c-index-test/c-index-test.c
  tools/libclang/CIndex.cpp

Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -3397,6 +3397,9 @@
   if (options & CXTranslationUnit_KeepGoing)
 Diags->setSuppressAfterFatalError(false);
 
+  if (options & CXTranslationUnit_IgnoreNonErrorsFromIncludedFiles)
+  Diags->setSuppressNonErrorsFromIncludedFiles(true);
+
   // Recover resources if we crash before exiting this function.
   llvm::CrashRecoveryContextCleanupRegistrar >
Index: tools/c-index-test/c-index-test.c
===
--- tools/c-index-test/c-index-test.c
+++ tools/c-index-test/c-index-test.c
@@ -84,6 +84,8 @@
 options |= CXTranslationUnit_KeepGoing;
   if (getenv("CINDEXTEST_LIMIT_SKIP_FUNCTION_BODIES_TO_PREAMBLE"))
 options |= CXTranslationUnit_LimitSkipFunctionBodiesToPreamble;
+  if (getenv("CINDEXTEST_IGNORE_NONERRORS_FROM_INCLUDED_FILES"))
+options |= CXTranslationUnit_IgnoreNonErrorsFromIncludedFiles;
 
   return options;
 }
Index: test/Index/ignore-warnings-from-headers.h
===
--- /dev/null
+++ test/Index/ignore-warnings-from-headers.h
@@ -0,0 +1 @@
+void f(int unusedInHeader) {}
Index: test/Index/ignore-warnings-from-headers.cpp
===
--- /dev/null
+++ test/Index/ignore-warnings-from-headers.cpp
@@ -0,0 +1,7 @@
+#include "ignore-warnings-from-headers.h"
+
+void g(int unusedInMainFile) {}
+
+// RUN: env CINDEXTEST_IGNORE_NONERRORS_FROM_INCLUDED_FILES=1 c-index-test -test-load-source function %s -Wunused-parameter 2>&1 | FileCheck %s
+// CHECK-NOT: warning: unused parameter 'unusedInHeader'
+// CHECK: warning: unused parameter 'unusedInMainFile'
Index: lib/Basic/DiagnosticIDs.cpp
===
--- lib/Basic/DiagnosticIDs.cpp
+++ lib/Basic/DiagnosticIDs.cpp
@@ -477,6 +477,14 @@
   Result = diag::Severity::Fatal;
   }
 
+  // If requested, ignore non-errors from all included files.
+  if (Diag.SuppressNonErrorsFromIncludedFiles &&
+  Result <= diag::Severity::Warning && Loc.isValid() &&
+  !Diag.getSourceManager().isInMainFile(
+  Diag.getSourceManager().getExpansionLoc(Loc))) {
+return diag::Severity::Ignored;
+  }
+
   // Custom diagnostics always are emitted in system headers.
   bool ShowInSystemHeader =
   !GetDiagInfo(DiagID) || GetDiagInfo(DiagID)->WarnShowInSystemHeader;
Index: include/clang/Basic/Diagnostic.h
===
--- include/clang/Basic/Diagnostic.h
+++ include/clang/Basic/Diagnostic.h
@@ -213,6 +213,9 @@
   // Suppress all diagnostics.
   bool SuppressAllDiagnostics = false;
 
+  // Suppress non-errors from all included files.
+  bool SuppressNonErrorsFromIncludedFiles = false;
+
   // Elide common types of templates.
   bool ElideType = true;
 
@@ -634,6 +637,10 @@
   }
   bool getSuppressAllDiagnostics() const { return SuppressAllDiagnostics; }
 
+  void setSuppressNonErrorsFromIncludedFiles(bool Val = true) {
+SuppressNonErrorsFromIncludedFiles = Val;
+  }
+
   /// Set type eliding, to skip outputting same types occurring in
   /// template types.
   void setElideType(bool Val = true) { ElideType = Val; }
Index: include/clang-c/Index.h
===
--- include/clang-c/Index.h
+++ include/clang-c/Index.h
@@ -1332,7 +1332,17 @@
*
* The function bodies of the main file are not skipped.
*/
-  CXTranslationUnit_LimitSkipFunctionBodiesToPreamble = 0x800
+  CXTranslationUnit_LimitSkipFunctionBodiesToPreamble = 0x800,
+
+  /**
+   * Used to indicate that non-errors from included files should be ignored.
+   *
+   * If set, clang_getDiagnosticSetFromTU() will not report e.g. warnings from
+   * included files anymore. This speeds up clang_getDiagnosticSetFromTU() for
+   * the case where these warnings are not of interest, as for an IDE for
+   * example, which typically shows only the diagnostics in the main file.
+   */
+  CXTranslationUnit_IgnoreNonErrorsFromIncludedFiles = 0x1000
 };
 
 /**
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

Cool! That's actually quite a small change with big outcome!




Comment at: include/clang/Basic/Diagnostic.h:216
 
+  // Suppress warnings from all included files.
+  bool SuppressWarningsFromIncludedFiles = false;

Probably mention that it also suppresses everything with lower priority? 



Comment at: lib/Basic/DiagnosticIDs.cpp:484
+  !Diag.getSourceManager().isInMainFile(
+  Diag.getSourceManager().getExpansionLoc(Loc)))
+return diag::Severity::Ignored;

{ } around return


Repository:
  rC Clang

https://reviews.llvm.org/D48116



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

[PATCH] D48116: [libclang] Allow skipping warnings from all included files

[Nikolai Kosjar via Phabricator via cfe-commits]

nik created this revision.
Herald added a subscriber: cfe-commits.

Depending on the included files and the used warning flags, e.g. -
Weverything, a huge number of warnings can be reported for included
files. As processing that many diagnostics comes with a performance
impact and not all clients are interested in those diagnostics, add a
flag to skip them.


Repository:
  rC Clang

https://reviews.llvm.org/D48116

Files:
  include/clang-c/Index.h
  include/clang/Basic/Diagnostic.h
  lib/Basic/DiagnosticIDs.cpp
  test/Index/ignore-warnings-from-headers.cpp
  test/Index/ignore-warnings-from-headers.h
  tools/c-index-test/c-index-test.c
  tools/libclang/CIndex.cpp

Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -3397,6 +3397,9 @@
   if (options & CXTranslationUnit_KeepGoing)
 Diags->setSuppressAfterFatalError(false);
 
+  if (options & CXTranslationUnit_IgnoreWarningsFromIncludedFiles)
+  Diags->setSuppressWarningsFromIncludedFiles(true);
+
   // Recover resources if we crash before exiting this function.
   llvm::CrashRecoveryContextCleanupRegistrar >
Index: tools/c-index-test/c-index-test.c
===
--- tools/c-index-test/c-index-test.c
+++ tools/c-index-test/c-index-test.c
@@ -84,6 +84,8 @@
 options |= CXTranslationUnit_KeepGoing;
   if (getenv("CINDEXTEST_LIMIT_SKIP_FUNCTION_BODIES_TO_PREAMBLE"))
 options |= CXTranslationUnit_LimitSkipFunctionBodiesToPreamble;
+  if (getenv("CINDEXTEST_IGNORE_WARNINGS_FROM_INCLUDED_FILES"))
+options |= CXTranslationUnit_IgnoreWarningsFromIncludedFiles;
 
   return options;
 }
Index: test/Index/ignore-warnings-from-headers.h
===
--- /dev/null
+++ test/Index/ignore-warnings-from-headers.h
@@ -0,0 +1 @@
+void f(int unusedInHeader) {}
Index: test/Index/ignore-warnings-from-headers.cpp
===
--- /dev/null
+++ test/Index/ignore-warnings-from-headers.cpp
@@ -0,0 +1,7 @@
+#include "ignore-warnings-from-headers.h"
+
+void g(int unusedInMainFile) {}
+
+// RUN: env CINDEXTEST_IGNORE_WARNINGS_FROM_INCLUDED_FILES=1 c-index-test -test-load-source function %s -Wunused-parameter 2>&1 | FileCheck %s
+// CHECK-NOT: warning: unused parameter 'unusedInHeader'
+// CHECK: warning: unused parameter 'unusedInMainFile'
Index: lib/Basic/DiagnosticIDs.cpp
===
--- lib/Basic/DiagnosticIDs.cpp
+++ lib/Basic/DiagnosticIDs.cpp
@@ -477,6 +477,13 @@
   Result = diag::Severity::Fatal;
   }
 
+  // If requested, ignore warnings from all headers.
+  if (Diag.SuppressWarningsFromIncludedFiles && Result <= diag::Severity::Warning &&
+  Loc.isValid() &&
+  !Diag.getSourceManager().isInMainFile(
+  Diag.getSourceManager().getExpansionLoc(Loc)))
+return diag::Severity::Ignored;
+
   // Custom diagnostics always are emitted in system headers.
   bool ShowInSystemHeader =
   !GetDiagInfo(DiagID) || GetDiagInfo(DiagID)->WarnShowInSystemHeader;
Index: include/clang/Basic/Diagnostic.h
===
--- include/clang/Basic/Diagnostic.h
+++ include/clang/Basic/Diagnostic.h
@@ -213,6 +213,9 @@
   // Suppress all diagnostics.
   bool SuppressAllDiagnostics = false;
 
+  // Suppress warnings from all included files.
+  bool SuppressWarningsFromIncludedFiles = false;
+
   // Elide common types of templates.
   bool ElideType = true;
 
@@ -634,6 +637,10 @@
   }
   bool getSuppressAllDiagnostics() const { return SuppressAllDiagnostics; }
 
+  void setSuppressWarningsFromIncludedFiles(bool Val = true) {
+SuppressWarningsFromIncludedFiles = Val;
+  }
+
   /// Set type eliding, to skip outputting same types occurring in
   /// template types.
   void setElideType(bool Val = true) { ElideType = Val; }
Index: include/clang-c/Index.h
===
--- include/clang-c/Index.h
+++ include/clang-c/Index.h
@@ -1332,7 +1332,17 @@
*
* The function bodies of the main file are not skipped.
*/
-  CXTranslationUnit_LimitSkipFunctionBodiesToPreamble = 0x800
+  CXTranslationUnit_LimitSkipFunctionBodiesToPreamble = 0x800,
+
+  /**
+   * Used to indicate that warnings from included files should be ignored.
+   *
+   * If set, clang_getDiagnosticSetFromTU() will not report warnings from
+   * included files anymore. This speeds up clang_getDiagnosticSetFromTU() for
+   * the case where these warnings are not of interest, as for an IDE for
+   * example, which typically shows only the diagnostics in the main file.
+   */
+  CXTranslationUnit_IgnoreWarningsFromIncludedFiles = 0x1000
 };
 
 /**
___
cfe-commits mailing list
cfe-commits@lists.llvm.org