[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-20 Thread Haojian Wu via Phabricator via cfe-commits 
hokein added a comment.

oops, sorry for the trouble it caused, and thanks for @kadircet fixing it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

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

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-19 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet added a comment.

sorry for the hassle 2133e8b9f942f91ec54e28c580fccf6d6b26c62e 
 should fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

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

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-19 Thread Mike Hommey via Phabricator via cfe-commits 
glandium added a comment.

with `-DLLVM_LINK_LLVM_DYLIB=ON`, in case it matters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

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

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-19 Thread Mike Hommey via Phabricator via cfe-commits 
glandium added a comment.

I'm still seeing the build error on clangd-fuzzer on a commit that clearly has 
e84d69f52d9a9fab9162128d8fe8ebec99ea60da 
 in its 
history.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

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

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-19 Thread Khem Raj via Phabricator via cfe-commits 
raj.khem added a comment.

In D140875#4065824 , @hokein wrote:

> In D140875#4065763 , @ckandeler 
> wrote:
>
>> With this, I now get:
>> FAILED: bin/clangd-fuzzer 
>> : && /usr/lib/icecream/libexec/icecc/bin/c++ -fPIC 
>> -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time 
>> -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual 
>> -Wno-missing-field-initializers -pedantic -Wno-long-long 
>> -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess 
>> -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type 
>> -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment 
>> -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color 
>> -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual 
>> -fno-strict-aliasing -O2 -g -DNDEBUG -fuse-ld=lld -Wl,--color-diagnostics
>> -Wl,--gc-sections 
>> tools/clang/tools/extra/clangd/fuzzer/CMakeFiles/clangd-fuzzer.dir/FuzzerClangdMain.cpp.o
>>  
>> tools/clang/tools/extra/clangd/fuzzer/CMakeFiles/clangd-fuzzer.dir/clangd-fuzzer.cpp.o
>>  -o bin/clangd-fuzzer  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libclangDaemon.a  
>> lib/libclangdSupport.a  lib/libclangPseudo.a  lib/libclangPseudoGrammar.a  
>> lib/libclangTidyAndroidModule.a  lib/libclangTidyAbseilModule.a  
>> lib/libclangTidyAlteraModule.a  lib/libclangTidyBoostModule.a  
>> lib/libclangTidyCERTModule.a  lib/libclangTidyConcurrencyModule.a  
>> lib/libclangTidyDarwinModule.a  lib/libclangTidyFuchsiaModule.a  
>> lib/libclangTidyHICPPModule.a  lib/libclangTidyBugproneModule.a  
>> lib/libclangTidyCppCoreGuidelinesModule.a  lib/libclangTidyGoogleModule.a  
>> lib/libclangTidyLinuxKernelModule.a  lib/libclangTidyLLVMModule.a  
>> lib/libclangTidyLLVMLibcModule.a  lib/libclangTidyMiscModule.a  
>> lib/libclangAnalysis.a  lib/libclangASTMatchers.a  lib/libclangAST.a  
>> lib/libclangLex.a  lib/libclangBasic.a  lib/libclangTidyModernizeModule.a  
>> lib/libclangTidyObjCModule.a  lib/libclangTidyOpenMPModule.a  
>> lib/libclangTidyPerformanceModule.a  lib/libclangTidyPortabilityModule.a  
>> lib/libclangTidyReadabilityModule.a  lib/libclangTidyZirconModule.a  
>> lib/libclangTidyMPIModule.a  lib/libclangTidyUtils.a  lib/libclangTidy.a  
>> lib/libclang-cpp.so.16git  lib/libLLVM-16git.so && :
>> ld.lld: error: undefined symbol: 
>> clang::include_cleaner::walkUsed(llvm::ArrayRef, 
>> llvm::ArrayRef, 
>> clang::include_cleaner::PragmaIncludes const*, clang::SourceManager const&, 
>> llvm::function_ref> llvm::ArrayRef)>)
>>
> referenced by IncludeCleaner.cpp:504 
> (/sda/home/christian/dev/llvm/clang-tools-extra/clangd/IncludeCleaner.cpp:504)
>
>   
> IncludeCleaner.cpp.o:(clang::clangd::computeUnusedIncludesExperimental(clang::clangd::ParsedAST&)
>  (.localalias)) in archive lib/libclangDaemon.a
>
> sorry, should be fixed in 
> https://github.com/llvm/llvm-project/commit/e84d69f52d9a9fab9162128d8fe8ebec99ea60da.

this seems to be not enough, I am also seeing clangd build failiures

  /mnt/b/yoe/master/build/tmp/hosttools/ld: 
lib/libclangDaemon.a(Preamble.cpp.o): in function `clang::clangd::(anonymous 
namespace)::CppFilePreambleCallbacks::BeforeExecute(clang::CompilerInstance&)':
  
Preamble.cpp:(.text._ZN5clang6clangd12_GLOBAL__N_124CppFilePreambleCallbacks13BeforeExecuteERNS_16CompilerInstanceE+0x8b):
 undefined reference to 
`clang::include_cleaner::PragmaIncludes::record(clang::CompilerInsta
  nce const&)'
  /mnt/b/yoe/master/build/tmp/hosttools/ld: 
lib/libclangDaemon.a(IncludeCleaner.cpp.o): in function 
`clang::clangd::computeUnusedIncludesExperimental(clang::clangd::ParsedAST&) 
[clone .localalias]':
  
IncludeCleaner.cpp:(.text._ZN5clang6clangd33computeUnusedIncludesExperimentalERNS0_9ParsedASTE+0x3a1):
 undefined reference to 
`clang::include_cleaner::walkUsed(llvm::ArrayRef, 
llvm::ArrayRef, clang::include_cleaner::PragmaIncludes 
const*, clang::SourceManager const&, llvm::function_ref)>)'
  collect2: error: ld returned 1 exit status
  [193/193] Linking CXX executable bin/clangd
  FAILED: bin/clangd


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

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

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-19 Thread Haojian Wu via Phabricator via cfe-commits 
hokein added a comment.

In D140875#4065763 , @ckandeler wrote:

> With this, I now get:
> FAILED: bin/clangd-fuzzer 
> : && /usr/lib/icecream/libexec/icecc/bin/c++ -fPIC 
> -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time 
> -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual 
> -Wno-missing-field-initializers -pedantic -Wno-long-long 
> -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess 
> -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type 
> -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment 
> -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color 
> -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual 
> -fno-strict-aliasing -O2 -g -DNDEBUG -fuse-ld=lld -Wl,--color-diagnostics
> -Wl,--gc-sections 
> tools/clang/tools/extra/clangd/fuzzer/CMakeFiles/clangd-fuzzer.dir/FuzzerClangdMain.cpp.o
>  
> tools/clang/tools/extra/clangd/fuzzer/CMakeFiles/clangd-fuzzer.dir/clangd-fuzzer.cpp.o
>  -o bin/clangd-fuzzer  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libclangDaemon.a  
> lib/libclangdSupport.a  lib/libclangPseudo.a  lib/libclangPseudoGrammar.a  
> lib/libclangTidyAndroidModule.a  lib/libclangTidyAbseilModule.a  
> lib/libclangTidyAlteraModule.a  lib/libclangTidyBoostModule.a  
> lib/libclangTidyCERTModule.a  lib/libclangTidyConcurrencyModule.a  
> lib/libclangTidyDarwinModule.a  lib/libclangTidyFuchsiaModule.a  
> lib/libclangTidyHICPPModule.a  lib/libclangTidyBugproneModule.a  
> lib/libclangTidyCppCoreGuidelinesModule.a  lib/libclangTidyGoogleModule.a  
> lib/libclangTidyLinuxKernelModule.a  lib/libclangTidyLLVMModule.a  
> lib/libclangTidyLLVMLibcModule.a  lib/libclangTidyMiscModule.a  
> lib/libclangAnalysis.a  lib/libclangASTMatchers.a  lib/libclangAST.a  
> lib/libclangLex.a  lib/libclangBasic.a  lib/libclangTidyModernizeModule.a  
> lib/libclangTidyObjCModule.a  lib/libclangTidyOpenMPModule.a  
> lib/libclangTidyPerformanceModule.a  lib/libclangTidyPortabilityModule.a  
> lib/libclangTidyReadabilityModule.a  lib/libclangTidyZirconModule.a  
> lib/libclangTidyMPIModule.a  lib/libclangTidyUtils.a  lib/libclangTidy.a  
> lib/libclang-cpp.so.16git  lib/libLLVM-16git.so && :
> ld.lld: error: undefined symbol: 
> clang::include_cleaner::walkUsed(llvm::ArrayRef, 
> llvm::ArrayRef, 
> clang::include_cleaner::PragmaIncludes const*, clang::SourceManager const&, 
> llvm::function_ref llvm::ArrayRef)>)
>
 referenced by IncludeCleaner.cpp:504 
 (/sda/home/christian/dev/llvm/clang-tools-extra/clangd/IncludeCleaner.cpp:504)

   
 IncludeCleaner.cpp.o:(clang::clangd::computeUnusedIncludesExperimental(clang::clangd::ParsedAST&)
  (.localalias)) in archive lib/libclangDaemon.a

sorry, should be fixed in 
https://github.com/llvm/llvm-project/commit/e84d69f52d9a9fab9162128d8fe8ebec99ea60da.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

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

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-19 Thread Christian Kandeler via Phabricator via cfe-commits 
ckandeler added a comment.

With this, I now get:
FAILED: bin/clangd-fuzzer 
: && /usr/lib/icecream/libexec/icecc/bin/c++ -fPIC -fno-semantic-interposition 
-fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra 
-Wno-unused-parameter -Wwrite-strings -Wcast-qual 
-Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough 
-Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move 
-Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor 
-Wsuggest-override -Wno-comment -Wno-misleading-indentation 
-Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections 
-fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O2 -g 
-DNDEBUG -fuse-ld=lld -Wl,--color-diagnostics-Wl,--gc-sections 
tools/clang/tools/extra/clangd/fuzzer/CMakeFiles/clangd-fuzzer.dir/FuzzerClangdMain.cpp.o
 
tools/clang/tools/extra/clangd/fuzzer/CMakeFiles/clangd-fuzzer.dir/clangd-fuzzer.cpp.o
 -o bin/clangd-fuzzer  -Wl,-rpath,"\$ORIGIN/../lib"  lib/libclangDaemon.a  
lib/libclangdSupport.a  lib/libclangPseudo.a  lib/libclangPseudoGrammar.a  
lib/libclangTidyAndroidModule.a  lib/libclangTidyAbseilModule.a  
lib/libclangTidyAlteraModule.a  lib/libclangTidyBoostModule.a  
lib/libclangTidyCERTModule.a  lib/libclangTidyConcurrencyModule.a  
lib/libclangTidyDarwinModule.a  lib/libclangTidyFuchsiaModule.a  
lib/libclangTidyHICPPModule.a  lib/libclangTidyBugproneModule.a  
lib/libclangTidyCppCoreGuidelinesModule.a  lib/libclangTidyGoogleModule.a  
lib/libclangTidyLinuxKernelModule.a  lib/libclangTidyLLVMModule.a  
lib/libclangTidyLLVMLibcModule.a  lib/libclangTidyMiscModule.a  
lib/libclangAnalysis.a  lib/libclangASTMatchers.a  lib/libclangAST.a  
lib/libclangLex.a  lib/libclangBasic.a  lib/libclangTidyModernizeModule.a  
lib/libclangTidyObjCModule.a  lib/libclangTidyOpenMPModule.a  
lib/libclangTidyPerformanceModule.a  lib/libclangTidyPortabilityModule.a  
lib/libclangTidyReadabilityModule.a  lib/libclangTidyZirconModule.a  
lib/libclangTidyMPIModule.a  lib/libclangTidyUtils.a  lib/libclangTidy.a  
lib/libclang-cpp.so.16git  lib/libLLVM-16git.so && :
ld.lld: error: undefined symbol: 
clang::include_cleaner::walkUsed(llvm::ArrayRef, 
llvm::ArrayRef, 
clang::include_cleaner::PragmaIncludes const*, clang::SourceManager const&, 
llvm::function_ref)>)

>>> referenced by IncludeCleaner.cpp:504 
>>> (/sda/home/christian/dev/llvm/clang-tools-extra/clangd/IncludeCleaner.cpp:504)
>>>
>>>   
>>> IncludeCleaner.cpp.o:(clang::clangd::computeUnusedIncludesExperimental(clang::clangd::ParsedAST&)
>>>  (.localalias)) in archive lib/libclangDaemon.a


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

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

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-19 Thread Haojian Wu via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.
Closed by commit rG939dce12f9f3: [clangd] Implement unused include warnings 
with include-cleaner library. (authored by hokein).

Changed prior to commit:
  https://reviews.llvm.org/D140875?vs=490452=490472#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -342,6 +342,8 @@
   auto AST = TU.build();
   EXPECT_THAT(computeUnusedIncludes(AST),
   ElementsAre(Pointee(writtenInclusion("";
+  EXPECT_THAT(computeUnusedIncludesExperimental(AST),
+  ElementsAre(Pointee(writtenInclusion("";
 }
 
 TEST(IncludeCleaner, GetUnusedHeaders) {
@@ -377,6 +379,10 @@
   computeUnusedIncludes(AST),
   UnorderedElementsAre(Pointee(writtenInclusion("\"unused.h\"")),
Pointee(writtenInclusion("\"dir/unused.h\"";
+  EXPECT_THAT(
+  computeUnusedIncludesExperimental(AST),
+  UnorderedElementsAre(Pointee(writtenInclusion("\"unused.h\"")),
+   Pointee(writtenInclusion("\"dir/unused.h\"";
 }
 
 TEST(IncludeCleaner, VirtualBuffers) {
@@ -531,6 +537,9 @@
 // IWYU pragma: private, include "public.h"
 void foo() {}
   )cpp");
+  Config Cfg;
+  Cfg.Diagnostics.UnusedIncludes = Config::Experiment;
+  WithContextValue Ctx(Config::Key, std::move(Cfg));
   ParsedAST AST = TU.build();
 
   auto ReferencedFiles = findReferencedFiles(
@@ -545,6 +554,7 @@
   ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
   EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+  EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, RecursiveInclusion) {
@@ -573,6 +583,7 @@
 
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
   EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+  EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, IWYUPragmaExport) {
@@ -597,6 +608,7 @@
   // FIXME: This is not correct: foo.h is unused but is not diagnosed as such
   // because we ignore headers with IWYU export pragmas for now.
   EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+  EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, NoDiagsForObjC) {
Index: clang-tools-extra/clangd/Preamble.h
===
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -27,6 +27,7 @@
 #include "Diagnostics.h"
 #include "FS.h"
 #include "Headers.h"
+#include "clang-include-cleaner/Record.h"
 #include "index/CanonicalIncludes.h"
 #include "support/Path.h"
 #include "clang/Frontend/CompilerInvocation.h"
@@ -57,6 +58,8 @@
   // Processes like code completions and go-to-definitions will need #include
   // information, and their compile action skips preamble range.
   IncludeStructure Includes;
+  // Captures #include-mapping information in #included headers.
+  include_cleaner::PragmaIncludes Pragmas;
   // Macros defined in the preamble section of the main file.
   // Users care about headers vs main-file, not preamble vs non-preamble.
   // These should be treated as main-file entities e.g. for code completion.
Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -11,6 +11,7 @@
 #include "Config.h"
 #include "Headers.h"
 #include "SourceCode.h"
+#include "clang-include-cleaner/Record.h"
 #include "support/Logger.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
@@ -77,6 +78,9 @@
 
   std::vector takeMarks() { return std::move(Marks); }
 
+  include_cleaner::PragmaIncludes takePragmaIncludes() {
+return std::move(Pragmas);
+  }
   CanonicalIncludes takeCanonicalIncludes() { return std::move(CanonIncludes); }
 
   bool isMainFileIncludeGuarded() const { return IsMainFileIncludeGuarded; }
@@ -118,6 +122,9 @@
 LangOpts = ();
 SourceMgr = ();

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-19 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.

thanks!




Comment at: clang-tools-extra/clangd/ParsedAST.h:111
 
+  const include_cleaner::PragmaIncludes *getPragmaIncludes() const;
+

can you add a comment saying `might be null if AST is built without a preamble`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

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

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-19 Thread Haojian Wu via Phabricator via cfe-commits 
hokein added inline comments.



Comment at: clang-tools-extra/clangd/ParsedAST.cpp:609
+  Config::UnusedIncludesPolicy::Experiment)
+Pragmas.record(*Clang);
   // Copy over the macros in the preamble region of the main file, and combine

kadircet wrote:
> hokein wrote:
> > kadircet wrote:
> > > why do we need to collect pragmas in main file? i think we already have 
> > > necessary information available via `IncludeStructure` (it stores keeps 
> > > and exports, and we don't care about anything else in the main file 
> > > AFAICT). so let's just use the PragmaIncludes we're getting from the 
> > > Preamble directly? without even making a copy and returning a reference 
> > > from the `Preamble` instead in `ParsedAST::getPragmaIncludes`
> > > i think we already have necessary information available via 
> > > IncludeStructure (it stores keeps and exports, and we don't care about 
> > > anything else in the main file AFAICT)
> > 
> > The IncludeStructure doesn't have a full support for IWYU export pragma, it 
> > only tracks the headers that have the export pragma.
> > 
> > My understand of the end goal is to use the `PragmaInclude` to handle every 
> > IWYU-related things, and we can remove all these IWYU bits in the 
> > `IncludeStructure`,  clangd IWYU pragma handling 
> > [code](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Headers.cpp#L127-L166).
> > The IncludeStructure doesn't have a full support for IWYU export pragma, it 
> > only tracks the headers that have the export pragma.
> 
> And I think that's all we need for IWYU pragmas inside the main file (as main 
> file is a leaf and exporting headers from it doesn't change anything apart 
> from making sure we keep them around)
> 
> > My understand of the end goal is to use the PragmaInclude to handle every 
> > IWYU-related things, and we can remove all these IWYU bits in the 
> > IncludeStructure, clangd IWYU pragma handling code.
> 
> Yes, I agree with some version of that, but until then this is introducing 
> some extra changes (+ copying around more information in every AST build). So 
> can we leave this piece out of the initial patch and just use the 
> PragmaInclude we have from Preamble without copying it around?
> Yes, I agree with some version of that, but until then this is introducing 
> some extra changes (+ copying around more information in every AST build). So 
> can we leave this piece out of the initial patch and just use the 
> PragmaInclude we have from Preamble without copying it around?

Sure, that sounds a good plan, and have a better narrowed scope of the patch. 



Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:345
   auto Unused = computeUnusedIncludes(AST);
   EXPECT_THAT(Unused, ElementsAre(Pointee(writtenInclusion("";
+  EXPECT_THAT(computeUnusedIncludesNew(AST),

kadircet wrote:
> can you inline the call to `computeUnusedIncludes` into the EXPECT statement 
> here?
sure, since this is not directly related to this patch, addressed in 
ccb67491f0dd55c5bd8ed5f71cb802422bfaa969.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

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

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-19 Thread Haojian Wu via Phabricator via cfe-commits 
hokein updated this revision to Diff 490452.
hokein marked 3 inline comments as done.
hokein added a comment.

address comments:

- rename to computeUnusedIncludesExperimental
- use PragmaInclude from preamble, limit the scope of the patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp

Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -342,6 +342,8 @@
   auto AST = TU.build();
   EXPECT_THAT(computeUnusedIncludes(AST),
   ElementsAre(Pointee(writtenInclusion("";
+  EXPECT_THAT(computeUnusedIncludesExperimental(AST),
+  ElementsAre(Pointee(writtenInclusion("";
 }
 
 TEST(IncludeCleaner, GetUnusedHeaders) {
@@ -377,6 +379,10 @@
   computeUnusedIncludes(AST),
   UnorderedElementsAre(Pointee(writtenInclusion("\"unused.h\"")),
Pointee(writtenInclusion("\"dir/unused.h\"";
+  EXPECT_THAT(
+  computeUnusedIncludesExperimental(AST),
+  UnorderedElementsAre(Pointee(writtenInclusion("\"unused.h\"")),
+   Pointee(writtenInclusion("\"dir/unused.h\"";
 }
 
 TEST(IncludeCleaner, VirtualBuffers) {
@@ -531,6 +537,9 @@
 // IWYU pragma: private, include "public.h"
 void foo() {}
   )cpp");
+  Config Cfg;
+  Cfg.Diagnostics.UnusedIncludes = Config::Experiment;
+  WithContextValue Ctx(Config::Key, std::move(Cfg));
   ParsedAST AST = TU.build();
 
   auto ReferencedFiles = findReferencedFiles(
@@ -545,6 +554,7 @@
   ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
   EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+  EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, RecursiveInclusion) {
@@ -573,6 +583,7 @@
 
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
   EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+  EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, IWYUPragmaExport) {
@@ -597,6 +608,7 @@
   // FIXME: This is not correct: foo.h is unused but is not diagnosed as such
   // because we ignore headers with IWYU export pragmas for now.
   EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+  EXPECT_THAT(computeUnusedIncludesExperimental(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, NoDiagsForObjC) {
Index: clang-tools-extra/clangd/Preamble.h
===
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -27,6 +27,7 @@
 #include "Diagnostics.h"
 #include "FS.h"
 #include "Headers.h"
+#include "clang-include-cleaner/Record.h"
 #include "index/CanonicalIncludes.h"
 #include "support/Path.h"
 #include "clang/Frontend/CompilerInvocation.h"
@@ -57,6 +58,8 @@
   // Processes like code completions and go-to-definitions will need #include
   // information, and their compile action skips preamble range.
   IncludeStructure Includes;
+  // Captures #include-mapping information in #included headers.
+  include_cleaner::PragmaIncludes Pragmas;
   // Macros defined in the preamble section of the main file.
   // Users care about headers vs main-file, not preamble vs non-preamble.
   // These should be treated as main-file entities e.g. for code completion.
Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -11,6 +11,7 @@
 #include "Config.h"
 #include "Headers.h"
 #include "SourceCode.h"
+#include "clang-include-cleaner/Record.h"
 #include "support/Logger.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
@@ -77,6 +78,9 @@
 
   std::vector takeMarks() { return std::move(Marks); }
 
+  include_cleaner::PragmaIncludes takePragmaIncludes() {
+return std::move(Pragmas);
+  }
   CanonicalIncludes takeCanonicalIncludes() { return std::move(CanonIncludes); }
 
   bool isMainFileIncludeGuarded() const { return IsMainFileIncludeGuarded; }
@@ -118,6 +122,9 @@
 LangOpts = ();
 SourceMgr = ();
 Includes.collect(CI);
+if (Config::current().Diagnostics.UnusedIncludes ==
+Config::UnusedIncludesPolicy::Experiment)
+

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-19 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet added inline comments.



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:479
+}
+// FIXME: !!this is a hacky way to collect macro references.
+std::vector Macros;

hokein wrote:
> kadircet wrote:
> > this might behave slightly different than what we have in `RecordedPP`, and 
> > rest of the applications of include-cleaner will be using that. can we 
> > expose the pieces in RecordedPP that collects MacroRefs as a separate 
> > handler and attach that collector (combined with the skipped range 
> > collection inside `CollectMainFileMacros` and also still converting to 
> > `MainFileMacros` in the end (as we can't store 
> > sourcelocations/identifierinfos from preamble)?
> > 
> > afterwards we can use the names stored in there to get back to 
> > `IdentifierInfo`s and Ranges stored to get back to `SourceLocation`s. WDYT?
> Yeah, this is a better solution, but I'm not sure whether we should do this 
> before the release cut, it has a side effect of changing the 
> find-macro-reference behavior in clangd. It requires some design/implement 
> work.
> 
> I agree that the current solution is hacky, and eventually will be replaced, 
> but it follows the existing `findReferencedMacros`, so it is not that bad. I 
> tend to land this version before the release cut. What do you think?
> Yeah, this is a better solution, but I'm not sure whether we should do this 
> before the release cut, it has a side effect of changing the 
> find-macro-reference behavior in clangd.

OK, i think you're right about possibly changing semantic highlighting for 
clangd. Let's push this as a follow-up after the branch cut then.

> It requires some design/implement work.

No action needed here just wanted to layout some ideas;
We just need to change the implementation details of CollectMainFileMacros to 
wrap the RecordedPP and add skipped ranges support to it. We might need to do a 
little plumbing to convert between types (as RecordedPP stores 
SourceLocations/MacroInfos that can't be used across preamble and AST) but that 
should be trivial to marshal (we can do the conversion inside EndOfMainFile). I 
haven't fully tried this though, so if you tried and faced certain 
incompatibilities PLMK.

> I agree that the current solution is hacky, and eventually will be replaced, 
> but it follows the existing findReferencedMacros, so it is not that bad. I 
> tend to land this version before the release cut. What do you think?

I am worried that landing this version and letting people use it for 6 months 
might result in us getting bug reports that we can't address until we converge 
(or even worse get bug reports due to behavior change after we converge). But 
changes here are somewhat more justified as we put this as an experimental 
feature, compared to regressions in existing clangd behavior as you pointed out.



Comment at: clang-tools-extra/clangd/IncludeCleaner.h:100
 std::vector computeUnusedIncludes(ParsedAST );
+std::vector computeUnusedIncludesNew(ParsedAST );
 

s/computeUnusedIncludesNew/computeUnusedIncludesExperimental/

can you also add a comment saying that this uses include-cleaner library to 
perform usage analysis?



Comment at: clang-tools-extra/clangd/ParsedAST.cpp:609
+  Config::UnusedIncludesPolicy::Experiment)
+Pragmas.record(*Clang);
   // Copy over the macros in the preamble region of the main file, and combine

hokein wrote:
> kadircet wrote:
> > why do we need to collect pragmas in main file? i think we already have 
> > necessary information available via `IncludeStructure` (it stores keeps and 
> > exports, and we don't care about anything else in the main file AFAICT). so 
> > let's just use the PragmaIncludes we're getting from the Preamble directly? 
> > without even making a copy and returning a reference from the `Preamble` 
> > instead in `ParsedAST::getPragmaIncludes`
> > i think we already have necessary information available via 
> > IncludeStructure (it stores keeps and exports, and we don't care about 
> > anything else in the main file AFAICT)
> 
> The IncludeStructure doesn't have a full support for IWYU export pragma, it 
> only tracks the headers that have the export pragma.
> 
> My understand of the end goal is to use the `PragmaInclude` to handle every 
> IWYU-related things, and we can remove all these IWYU bits in the 
> `IncludeStructure`,  clangd IWYU pragma handling 
> [code](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Headers.cpp#L127-L166).
> The IncludeStructure doesn't have a full support for IWYU export pragma, it 
> only tracks the headers that have the export pragma.

And I think that's all we need for IWYU pragmas inside the main file (as main 
file is a leaf and exporting headers from it doesn't change anything apart from 
making sure we keep them around)

> My understand of the end goal is to use the PragmaInclude to

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-18 Thread Haojian Wu via Phabricator via cfe-commits 
hokein updated this revision to Diff 490287.
hokein added a comment.

update the tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/IncludeCleaner.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
  clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
  clang-tools-extra/include-cleaner/lib/Record.cpp

Index: clang-tools-extra/include-cleaner/lib/Record.cpp
===
--- clang-tools-extra/include-cleaner/lib/Record.cpp
+++ clang-tools-extra/include-cleaner/lib/Record.cpp
@@ -150,7 +150,7 @@
   RecordPragma(const CompilerInstance , PragmaIncludes *Out)
   : SM(CI.getSourceManager()),
 HeaderInfo(CI.getPreprocessor().getHeaderSearchInfo()), Out(Out),
-UniqueStrings(Arena) {}
+UniqueStrings(Out->Arena) {}
 
   void FileChanged(SourceLocation Loc, FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -176,7 +176,6 @@
   std::unique(It.getSecond().begin(), It.getSecond().end()),
   It.getSecond().end());
 }
-Out->Arena = std::move(Arena);
   }
 
   void InclusionDirective(SourceLocation HashLoc, const Token ,
@@ -307,7 +306,6 @@
   const SourceManager 
   HeaderSearch 
   PragmaIncludes *Out;
-  llvm::BumpPtrAllocator Arena;
   /// Intern table for strings. Contents are on the arena.
   llvm::StringSaver UniqueStrings;
 
@@ -336,6 +334,27 @@
   std::vector KeepStack;
 };
 
+PragmaIncludes::PragmaIncludes(const PragmaIncludes & In) {
+  *this = In;
+}
+
+PragmaIncludes ::operator=(const PragmaIncludes ) {
+  ShouldKeep = In.ShouldKeep;
+  NonSelfContainedFiles = In.NonSelfContainedFiles;
+  Arena.Reset();
+  llvm::UniqueStringSaver Strings(Arena);
+  IWYUPublic.clear();
+  for (const auto& It : In.IWYUPublic)
+IWYUPublic.try_emplace(It.first, Strings.save(It.second));
+  IWYUExportBy.clear();
+  for (const auto& UIDAndExporters : In.IWYUExportBy) {
+const auto& [UID, Exporters] = UIDAndExporters;
+for (StringRef ExporterHeader : Exporters)
+   IWYUExportBy.try_emplace(UID).first->second.push_back(ExporterHeader);
+  }
+  return *this;
+ }
+
 void PragmaIncludes::record(const CompilerInstance ) {
   auto Record = std::make_unique(CI, this);
   CI.getPreprocessor().addCommentHandler(Record.get());
Index: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
===
--- clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
+++ clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
@@ -48,6 +48,13 @@
 /// defines the symbol.
 class PragmaIncludes {
 public:
+  PragmaIncludes() = default;
+  PragmaIncludes(PragmaIncludes &&) = default;
+  PragmaIncludes =(PragmaIncludes &&) = default;
+
+  PragmaIncludes(const PragmaIncludes &);
+  PragmaIncludes =(const PragmaIncludes &);
+
   /// Installs an analysing PPCallback and CommentHandler and populates results
   /// to the structure.
   void record(const CompilerInstance );
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -343,6 +343,8 @@
 
   auto Unused = computeUnusedIncludes(AST);
   EXPECT_THAT(Unused, ElementsAre(Pointee(writtenInclusion("";
+  EXPECT_THAT(computeUnusedIncludesNew(AST),
+  ElementsAre(Pointee(writtenInclusion("";
 }
 
 TEST(IncludeCleaner, GetUnusedHeaders) {
@@ -379,6 +381,10 @@
 UnusedIncludes.push_back(Include->Written);
   EXPECT_THAT(UnusedIncludes,
   UnorderedElementsAre("\"unused.h\"", "\"dir/unused.h\""));
+  EXPECT_THAT(
+  computeUnusedIncludesNew(AST),
+  UnorderedElementsAre(Pointee(writtenInclusion("\"unused.h\"")),
+   Pointee(writtenInclusion("\"dir/unused.h\"";
 }
 
 TEST(IncludeCleaner, VirtualBuffers) {
@@ -533,6 +539,9 @@
 // IWYU pragma: private, include "public.h"
 void foo() {}
   )cpp");
+  Config Cfg;
+  Cfg.Diagnostics.UnusedIncludes = Config::Experiment;
+  WithContextValue Ctx(Config::Key, std::move(Cfg));
   ParsedAST AST = TU.build();
 
   auto ReferencedFiles = findReferencedFiles(
@@ -547,6 +556,7 @@
   ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-18 Thread Haojian Wu via Phabricator via cfe-commits 
hokein added inline comments.



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:471
+
+// FIXME: this map should probably be in IncludeStructure.
+llvm::StringMap> BySpelling;

kadircet wrote:
> agreed, let's change `StdlibHeaders` in `IncludeStructure` to actually be a 
> `BySpelling` mapping, it should be no-op for existing implementation as we're 
> only checking for stdlib headers already (and there's no other usage)
sure, I will address it in a followup patch.



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:479
+}
+// FIXME: !!this is a hacky way to collect macro references.
+std::vector Macros;

kadircet wrote:
> this might behave slightly different than what we have in `RecordedPP`, and 
> rest of the applications of include-cleaner will be using that. can we expose 
> the pieces in RecordedPP that collects MacroRefs as a separate handler and 
> attach that collector (combined with the skipped range collection inside 
> `CollectMainFileMacros` and also still converting to `MainFileMacros` in the 
> end (as we can't store sourcelocations/identifierinfos from preamble)?
> 
> afterwards we can use the names stored in there to get back to 
> `IdentifierInfo`s and Ranges stored to get back to `SourceLocation`s. WDYT?
Yeah, this is a better solution, but I'm not sure whether we should do this 
before the release cut, it has a side effect of changing the 
find-macro-reference behavior in clangd. It requires some design/implement work.

I agree that the current solution is hacky, and eventually will be replaced, 
but it follows the existing `findReferencedMacros`, so it is not that bad. I 
tend to land this version before the release cut. What do you think?



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:487
+Macros.push_back({Tok.location(),
+  include_cleaner::Macro{/*Name=*/nullptr, DefLoc},
+  include_cleaner::RefType::Explicit});

kadircet wrote:
> you can get the `IdentifierInfo` using `Name` inside `Macro` and PP.
oh, right. Done.



Comment at: clang-tools-extra/clangd/ParsedAST.cpp:609
+  Config::UnusedIncludesPolicy::Experiment)
+Pragmas.record(*Clang);
   // Copy over the macros in the preamble region of the main file, and combine

kadircet wrote:
> why do we need to collect pragmas in main file? i think we already have 
> necessary information available via `IncludeStructure` (it stores keeps and 
> exports, and we don't care about anything else in the main file AFAICT). so 
> let's just use the PragmaIncludes we're getting from the Preamble directly? 
> without even making a copy and returning a reference from the `Preamble` 
> instead in `ParsedAST::getPragmaIncludes`
> i think we already have necessary information available via IncludeStructure 
> (it stores keeps and exports, and we don't care about anything else in the 
> main file AFAICT)

The IncludeStructure doesn't have a full support for IWYU export pragma, it 
only tracks the headers that have the export pragma.

My understand of the end goal is to use the `PragmaInclude` to handle every 
IWYU-related things, and we can remove all these IWYU bits in the 
`IncludeStructure`,  clangd IWYU pragma handling 
[code](https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/Headers.cpp#L127-L166).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

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

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-18 Thread Haojian Wu via Phabricator via cfe-commits 
hokein updated this revision to Diff 490137.
hokein marked 4 inline comments as done.
hokein added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
  clang-tools-extra/include-cleaner/lib/Record.cpp

Index: clang-tools-extra/include-cleaner/lib/Record.cpp
===
--- clang-tools-extra/include-cleaner/lib/Record.cpp
+++ clang-tools-extra/include-cleaner/lib/Record.cpp
@@ -150,7 +150,7 @@
   RecordPragma(const CompilerInstance , PragmaIncludes *Out)
   : SM(CI.getSourceManager()),
 HeaderInfo(CI.getPreprocessor().getHeaderSearchInfo()), Out(Out),
-UniqueStrings(Arena) {}
+UniqueStrings(Out->Arena) {}
 
   void FileChanged(SourceLocation Loc, FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -176,7 +176,6 @@
   std::unique(It.getSecond().begin(), It.getSecond().end()),
   It.getSecond().end());
 }
-Out->Arena = std::move(Arena);
   }
 
   void InclusionDirective(SourceLocation HashLoc, const Token ,
@@ -307,7 +306,6 @@
   const SourceManager 
   HeaderSearch 
   PragmaIncludes *Out;
-  llvm::BumpPtrAllocator Arena;
   /// Intern table for strings. Contents are on the arena.
   llvm::StringSaver UniqueStrings;
 
@@ -336,6 +334,27 @@
   std::vector KeepStack;
 };
 
+PragmaIncludes::PragmaIncludes(const PragmaIncludes & In) {
+  *this = In;
+}
+
+PragmaIncludes ::operator=(const PragmaIncludes ) {
+  ShouldKeep = In.ShouldKeep;
+  NonSelfContainedFiles = In.NonSelfContainedFiles;
+  Arena.Reset();
+  llvm::UniqueStringSaver Strings(Arena);
+  IWYUPublic.clear();
+  for (const auto& It : In.IWYUPublic)
+IWYUPublic.try_emplace(It.first, Strings.save(It.second));
+  IWYUExportBy.clear();
+  for (const auto& UIDAndExporters : In.IWYUExportBy) {
+const auto& [UID, Exporters] = UIDAndExporters;
+for (StringRef ExporterHeader : Exporters)
+   IWYUExportBy.try_emplace(UID).first->second.push_back(ExporterHeader);
+  }
+  return *this;
+ }
+
 void PragmaIncludes::record(const CompilerInstance ) {
   auto Record = std::make_unique(CI, this);
   CI.getPreprocessor().addCommentHandler(Record.get());
Index: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
===
--- clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
+++ clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
@@ -48,6 +48,13 @@
 /// defines the symbol.
 class PragmaIncludes {
 public:
+  PragmaIncludes() = default;
+  PragmaIncludes(PragmaIncludes &&) = default;
+  PragmaIncludes =(PragmaIncludes &&) = default;
+
+  PragmaIncludes(const PragmaIncludes &);
+  PragmaIncludes =(const PragmaIncludes &);
+
   /// Installs an analysing PPCallback and CommentHandler and populates results
   /// to the structure.
   void record(const CompilerInstance );
Index: clang-tools-extra/clangd/Preamble.h
===
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -27,6 +27,7 @@
 #include "Diagnostics.h"
 #include "FS.h"
 #include "Headers.h"
+#include "clang-include-cleaner/Record.h"
 #include "index/CanonicalIncludes.h"
 #include "support/Path.h"
 #include "clang/Frontend/CompilerInvocation.h"
@@ -57,6 +58,8 @@
   // Processes like code completions and go-to-definitions will need #include
   // information, and their compile action skips preamble range.
   IncludeStructure Includes;
+
+  include_cleaner::PragmaIncludes Pragmas;
   // Macros defined in the preamble section of the main file.
   // Users care about headers vs main-file, not preamble vs non-preamble.
   // These should be treated as main-file entities e.g. for code completion.
Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -11,6 +11,7 @@
 #include "Config.h"
 #include "Headers.h"
 #include "SourceCode.h"
+#include "clang-include-cleaner/Record.h"
 #include "support/Logger.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
@@ -77,6 +78,9 @@
 
   std::vector takeMarks() { return std::move(Marks); }
 
+  include_cleaner::PragmaIncludes takePragmaIncludes() {
+return std::move(Pragmas);
+  }
   CanonicalIncludes takeCanonicalIncludes() { return

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-18 Thread Kadir Cetinkaya via Phabricator via cfe-commits 
kadircet added a comment.

regarding testing, i think updating all tests that we have in 
`IncludeCleanerTests` that call `computeUnusedIncludes` to also call the new 
function that'll use include-cleaner library should be enough.




Comment at: clang-tools-extra/clangd/CMakeLists.txt:5
 
+include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../include-cleaner/include)
+

can you move this near the include_directories for clang-tidy below (near line 
63)



Comment at: clang-tools-extra/clangd/Config.h:91
 
-  enum UnusedIncludesPolicy { Strict, None };
+  enum UnusedIncludesPolicy { Strict, None, Experiment };
   /// Controls warnings and errors when parsing code.

can you also add a comment saying that "`Experiment` tries to provide the same 
behaviour as `Strict` but using include-cleaner"



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:471
+
+// FIXME: this map should probably be in IncludeStructure.
+llvm::StringMap> BySpelling;

agreed, let's change `StdlibHeaders` in `IncludeStructure` to actually be a 
`BySpelling` mapping, it should be no-op for existing implementation as we're 
only checking for stdlib headers already (and there's no other usage)



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:479
+}
+// FIXME: !!this is a hacky way to collect macro references.
+std::vector Macros;

this might behave slightly different than what we have in `RecordedPP`, and 
rest of the applications of include-cleaner will be using that. can we expose 
the pieces in RecordedPP that collects MacroRefs as a separate handler and 
attach that collector (combined with the skipped range collection inside 
`CollectMainFileMacros` and also still converting to `MainFileMacros` in the 
end (as we can't store sourcelocations/identifierinfos from preamble)?

afterwards we can use the names stored in there to get back to 
`IdentifierInfo`s and Ranges stored to get back to `SourceLocation`s. WDYT?



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:487
+Macros.push_back({Tok.location(),
+  include_cleaner::Macro{/*Name=*/nullptr, DefLoc},
+  include_cleaner::RefType::Explicit});

you can get the `IdentifierInfo` using `Name` inside `Macro` and PP.



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:512
+});
+std::vector Unused;
+for (const auto  : Includes.MainFileIncludes) {

let's use `getUnused` directly here, with an empty set of `PublicHeaders`.



Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:539
   const Config  = Config::current();
-  if (Cfg.Diagnostics.UnusedIncludes != Config::UnusedIncludesPolicy::Strict ||
+  if (Cfg.Diagnostics.UnusedIncludes == Config::UnusedIncludesPolicy::None ||
   Cfg.Diagnostics.SuppressAll ||

can you keep `computeUnusedIncludes` the same and introduce a new function 
that'll call `include_cleaner`? afterwards we can just do a switch on policy 
here and call the relevant function.



Comment at: clang-tools-extra/clangd/ParsedAST.cpp:609
+  Config::UnusedIncludesPolicy::Experiment)
+Pragmas.record(*Clang);
   // Copy over the macros in the preamble region of the main file, and combine

why do we need to collect pragmas in main file? i think we already have 
necessary information available via `IncludeStructure` (it stores keeps and 
exports, and we don't care about anything else in the main file AFAICT). so 
let's just use the PragmaIncludes we're getting from the Preamble directly? 
without even making a copy and returning a reference from the `Preamble` 
instead in `ParsedAST::getPragmaIncludes`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

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

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-17 Thread Haojian Wu via Phabricator via cfe-commits 
hokein updated this revision to Diff 489959.
hokein added a comment.

rebase and update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140875

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
  clang-tools-extra/include-cleaner/lib/Record.cpp

Index: clang-tools-extra/include-cleaner/lib/Record.cpp
===
--- clang-tools-extra/include-cleaner/lib/Record.cpp
+++ clang-tools-extra/include-cleaner/lib/Record.cpp
@@ -150,7 +150,7 @@
   RecordPragma(const CompilerInstance , PragmaIncludes *Out)
   : SM(CI.getSourceManager()),
 HeaderInfo(CI.getPreprocessor().getHeaderSearchInfo()), Out(Out),
-UniqueStrings(Arena) {}
+UniqueStrings(Out->Arena) {}
 
   void FileChanged(SourceLocation Loc, FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -176,7 +176,6 @@
   std::unique(It.getSecond().begin(), It.getSecond().end()),
   It.getSecond().end());
 }
-Out->Arena = std::move(Arena);
   }
 
   void InclusionDirective(SourceLocation HashLoc, const Token ,
@@ -307,7 +306,6 @@
   const SourceManager 
   HeaderSearch 
   PragmaIncludes *Out;
-  llvm::BumpPtrAllocator Arena;
   /// Intern table for strings. Contents are on the arena.
   llvm::StringSaver UniqueStrings;
 
@@ -336,6 +334,27 @@
   std::vector KeepStack;
 };
 
+PragmaIncludes::PragmaIncludes(const PragmaIncludes & In) {
+  *this = In;
+}
+
+PragmaIncludes ::operator=(const PragmaIncludes ) {
+  ShouldKeep = In.ShouldKeep;
+  NonSelfContainedFiles = In.NonSelfContainedFiles;
+  Arena.Reset();
+  llvm::UniqueStringSaver Strings(Arena);
+  IWYUPublic.clear();
+  for (const auto& It : In.IWYUPublic)
+IWYUPublic.try_emplace(It.first, Strings.save(It.second));
+  IWYUExportBy.clear();
+  for (const auto& UIDAndExporters : In.IWYUExportBy) {
+const auto& [UID, Exporters] = UIDAndExporters;
+for (StringRef ExporterHeader : Exporters)
+   IWYUExportBy.try_emplace(UID).first->second.push_back(ExporterHeader);
+  }
+  return *this;
+ }
+
 void PragmaIncludes::record(const CompilerInstance ) {
   auto Record = std::make_unique(CI, this);
   CI.getPreprocessor().addCommentHandler(Record.get());
Index: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
===
--- clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
+++ clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
@@ -48,6 +48,13 @@
 /// defines the symbol.
 class PragmaIncludes {
 public:
+  PragmaIncludes() = default;
+  PragmaIncludes(PragmaIncludes &&) = default;
+  PragmaIncludes =(PragmaIncludes &&) = default;
+
+  PragmaIncludes(const PragmaIncludes &);
+  PragmaIncludes =(const PragmaIncludes &);
+
   /// Installs an analysing PPCallback and CommentHandler and populates results
   /// to the structure.
   void record(const CompilerInstance );
Index: clang-tools-extra/clangd/Preamble.h
===
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -27,6 +27,7 @@
 #include "Diagnostics.h"
 #include "FS.h"
 #include "Headers.h"
+#include "clang-include-cleaner/Record.h"
 #include "index/CanonicalIncludes.h"
 #include "support/Path.h"
 #include "clang/Frontend/CompilerInvocation.h"
@@ -57,6 +58,8 @@
   // Processes like code completions and go-to-definitions will need #include
   // information, and their compile action skips preamble range.
   IncludeStructure Includes;
+
+  include_cleaner::PragmaIncludes Pragmas;
   // Macros defined in the preamble section of the main file.
   // Users care about headers vs main-file, not preamble vs non-preamble.
   // These should be treated as main-file entities e.g. for code completion.
Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -11,6 +11,7 @@
 #include "Config.h"
 #include "Headers.h"
 #include "SourceCode.h"
+#include "clang-include-cleaner/Record.h"
 #include "support/Logger.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
@@ -77,6 +78,9 @@
 
   std::vector takeMarks() { return std::move(Marks); }
 
+  include_cleaner::PragmaIncludes takePragmaIncludes() {
+return std::move(Pragmas);
+  }
   CanonicalIncludes takeCanonicalIncludes() { return std::move(CanonIncludes); }
 
   bool

[PATCH] D140875: [clangd] prototype: Implement unused include warnings with include-cleaner library.

2023-01-03 Thread Haojian Wu via Phabricator via cfe-commits 
hokein created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
hokein requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

A prototype of using include-cleaner library in clangd:

- (re)implement clangd's "unused include" warnings with the library
- the new implementation is hidden under a flag 
`Config::UnusedIncludesPolicy::Experiment`


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140875

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/IncludeCleaner.cpp
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
  clang-tools-extra/include-cleaner/lib/Record.cpp

Index: clang-tools-extra/include-cleaner/lib/Record.cpp
===
--- clang-tools-extra/include-cleaner/lib/Record.cpp
+++ clang-tools-extra/include-cleaner/lib/Record.cpp
@@ -148,7 +148,7 @@
   RecordPragma(const CompilerInstance , PragmaIncludes *Out)
   : SM(CI.getSourceManager()),
 HeaderInfo(CI.getPreprocessor().getHeaderSearchInfo()), Out(Out),
-UniqueStrings(Arena) {}
+UniqueStrings(Out->Arena) {}
 
   void FileChanged(SourceLocation Loc, FileChangeReason Reason,
SrcMgr::CharacteristicKind FileType,
@@ -174,7 +174,6 @@
   std::unique(It.getSecond().begin(), It.getSecond().end()),
   It.getSecond().end());
 }
-Out->Arena = std::move(Arena);
   }
 
   void InclusionDirective(SourceLocation HashLoc, const Token ,
@@ -287,7 +286,6 @@
   const SourceManager 
   HeaderSearch 
   PragmaIncludes *Out;
-  llvm::BumpPtrAllocator Arena;
   /// Intern table for strings. Contents are on the arena.
   llvm::StringSaver UniqueStrings;
 
@@ -316,6 +314,27 @@
   std::vector KeepStack;
 };
 
+PragmaIncludes::PragmaIncludes(const PragmaIncludes & In) {
+  *this = In;
+}
+
+PragmaIncludes ::operator=(const PragmaIncludes ) {
+  ShouldKeep = In.ShouldKeep;
+  NonSelfContainedFiles = In.NonSelfContainedFiles;
+  Arena.Reset();
+  llvm::UniqueStringSaver Strings(Arena);
+  IWYUPublic.clear();
+  for (const auto& It : In.IWYUPublic)
+IWYUPublic.try_emplace(It.first, Strings.save(It.second));
+  IWYUExportBy.clear();
+  for (const auto& UIDAndExporters : In.IWYUExportBy) {
+const auto& [UID, Exporters] = UIDAndExporters;
+for (StringRef ExporterHeader : Exporters)
+   IWYUExportBy.try_emplace(UID).first->second.push_back(ExporterHeader);
+  }
+  return *this;
+ }
+
 void PragmaIncludes::record(const CompilerInstance ) {
   auto Record = std::make_unique(CI, this);
   CI.getPreprocessor().addCommentHandler(Record.get());
Index: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
===
--- clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
+++ clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h
@@ -48,6 +48,13 @@
 /// defines the symbol.
 class PragmaIncludes {
 public:
+  PragmaIncludes() = default;
+  PragmaIncludes(PragmaIncludes &&) = default;
+  PragmaIncludes =(PragmaIncludes &&) = default;
+
+  PragmaIncludes(const PragmaIncludes &);
+  PragmaIncludes =(const PragmaIncludes &);
+
   /// Installs an analysing PPCallback and CommentHandler and populates results
   /// to the structure.
   void record(const CompilerInstance );
Index: clang-tools-extra/clangd/Preamble.h
===
--- clang-tools-extra/clangd/Preamble.h
+++ clang-tools-extra/clangd/Preamble.h
@@ -27,6 +27,7 @@
 #include "Diagnostics.h"
 #include "FS.h"
 #include "Headers.h"
+#include "clang-include-cleaner/Record.h"
 #include "index/CanonicalIncludes.h"
 #include "support/Path.h"
 #include "clang/Frontend/CompilerInvocation.h"
@@ -57,6 +58,8 @@
   // Processes like code completions and go-to-definitions will need #include
   // information, and their compile action skips preamble range.
   IncludeStructure Includes;
+
+  include_cleaner::PragmaIncludes Pragmas;
   // Macros defined in the preamble section of the main file.
   // Users care about headers vs main-file, not preamble vs non-preamble.
   // These should be treated as main-file entities e.g. for code completion.
Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -11,6 +11,7 @@
 #include "Config.h"
 #include "Headers.h"
 #include "SourceCode.h"
+#include "clang-include-cleaner/Record.h"
 #include "support/Logger.h"
 #include