[PATCH] D60409: [clangd] Add -header-insertion=never flag to disable include insertion in code completion

2019-04-10 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added a comment.

In D60409#1461579 , @phosek wrote:

> Our Mac builders have started failing after this change with the following:
>
>   [3145/3502] Building CXX object 
> tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o
>   FAILED: 
> tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o 
>   /b/s/w/ir/cache/goma/client/gomacc /b/s/w/ir/k/cipd/bin/clang++  
> -DCLANG_VENDOR="\"Fuchsia \"" -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE 
> -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
> -Itools/clang/tools/extra/clangd/tool 
> -I/b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool 
> -I/b/s/w/ir/k/llvm-project/clang/include -Itools/clang/include 
> -I/usr/include/libxml2 -Iinclude -I/b/s/w/ir/k/llvm-project/llvm/include 
> -I/b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool/.. 
> -Itools/clang/tools/extra/clangd/tool/.. -fPIC -fvisibility-inlines-hidden 
> -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra 
> -Wno-unused-parameter -Wwrite-strings -Wcast-qual 
> -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough 
> -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor 
> -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -fno-common 
> -Woverloaded-virtual -Wno-nested-anon-types -O3 -gline-tables-only   -UNDEBUG 
>  -fno-exceptions -fno-rtti -MD -MT 
> tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o 
> -MF 
> tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o.d 
> -o tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o 
> -c /b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp
>   
> /b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp:474:22: 
> error: use of undeclared identifier 'newXPCTransport'
>   TransportLayer = newXPCTransport();
>^
>   1 error generated.
>
>
> I don't understand why since this change hasn't touched the failing line.


It has been fixed by rCTE358103 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60409



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

[PATCH] D60409: [clangd] Add -header-insertion=never flag to disable include insertion in code completion

2019-04-10 Thread Petr Hosek via Phabricator via cfe-commits 
phosek added a comment.

Our Mac builders have started failing after this change with the following:

  [3145/3502] Building CXX object 
tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o
  FAILED: 
tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o 
  /b/s/w/ir/cache/goma/client/gomacc /b/s/w/ir/k/cipd/bin/clang++  
-DCLANG_VENDOR="\"Fuchsia \"" -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE 
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
-Itools/clang/tools/extra/clangd/tool 
-I/b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool 
-I/b/s/w/ir/k/llvm-project/clang/include -Itools/clang/include 
-I/usr/include/libxml2 -Iinclude -I/b/s/w/ir/k/llvm-project/llvm/include 
-I/b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool/.. 
-Itools/clang/tools/extra/clangd/tool/.. -fPIC -fvisibility-inlines-hidden 
-Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra 
-Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers 
-pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default 
-Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor 
-Wstring-conversion -fdiagnostics-color -fno-common -Woverloaded-virtual 
-Wno-nested-anon-types -O3 -gline-tables-only   -UNDEBUG  -fno-exceptions 
-fno-rtti -MD -MT 
tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o -MF 
tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o.d -o 
tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o -c 
/b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp
  /b/s/w/ir/k/llvm-project/clang-tools-extra/clangd/tool/ClangdMain.cpp:474:22: 
error: use of undeclared identifier 'newXPCTransport'
  TransportLayer = newXPCTransport();
   ^
  1 error generated.

I don't understand why since this change hasn't touched the failing line.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60409



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

[PATCH] D60409: [clangd] Add -header-insertion=never flag to disable include insertion in code completion

2019-04-10 Thread Sam McCall via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rL358075: [clangd] Add -header-insertion=never flag to disable 
include insertion in code… (authored by sammccall, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60409?vs=194150&id=194493#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60409

Files:
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/clangd/CodeComplete.h
  clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

Index: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
@@ -6,8 +6,9 @@
 //
 //===--===//
 
-#include "Features.inc"
 #include "ClangdLSPServer.h"
+#include "CodeComplete.h"
+#include "Features.inc"
 #include "Path.h"
 #include "Protocol.h"
 #include "Trace.h"
@@ -154,6 +155,20 @@
 "debug-origin", llvm::cl::desc("Show origins of completion items"),
 llvm::cl::init(CodeCompleteOptions().ShowOrigins), llvm::cl::Hidden);
 
+static llvm::cl::opt HeaderInsertion(
+"header-insertion",
+llvm::cl::desc("Add #include directives when accepting code completions"),
+llvm::cl::init(CodeCompleteOptions().InsertIncludes),
+llvm::cl::values(
+clEnumValN(CodeCompleteOptions::IWYU, "iwyu",
+   "Include what you use. "
+   "Insert the owning header for top-level symbols, unless the "
+   "header is already directly included or the symbol is "
+   "forward-declared."),
+clEnumValN(
+CodeCompleteOptions::NeverInsert, "never",
+"Never insert #include directives as part of code completion")));
+
 static llvm::cl::opt HeaderInsertionDecorators(
 "header-insertion-decorators",
 llvm::cl::desc("Prepend a circular dot or space before the completion "
@@ -438,6 +453,7 @@
   CCOpts.Limit = LimitResults;
   CCOpts.BundleOverloads = CompletionStyle != Detailed;
   CCOpts.ShowOrigins = ShowOrigins;
+  CCOpts.InsertIncludes = HeaderInsertion;
   if (!HeaderInsertionDecorators) {
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -191,7 +191,9 @@
 
   // Returns a token identifying the overload set this is part of.
   // 0 indicates it's not part of any overload set.
-  size_t overloadSet() const {
+  size_t overloadSet(const CodeCompleteOptions &Opts) const {
+if (!Opts.BundleOverloads)
+  return 0;
 llvm::SmallString<256> Scratch;
 if (IndexResult) {
   switch (IndexResult->SymInfo.Kind) {
@@ -208,7 +210,7 @@
 // This could break #include insertion.
 return llvm::hash_combine(
 (IndexResult->Scope + IndexResult->Name).toStringRef(Scratch),
-headerToInsertIfAllowed().getValueOr(""));
+headerToInsertIfAllowed(Opts).getValueOr(""));
   default:
 return 0;
   }
@@ -223,12 +225,14 @@
   D->printQualifiedName(OS);
 }
 return llvm::hash_combine(Scratch,
-  headerToInsertIfAllowed().getValueOr(""));
+  headerToInsertIfAllowed(Opts).getValueOr(""));
   }
 
   // The best header to include if include insertion is allowed.
-  llvm::Optional headerToInsertIfAllowed() const {
-if (RankedIncludeHeaders.empty())
+  llvm::Optional
+  headerToInsertIfAllowed(const CodeCompleteOptions &Opts) const {
+if (Opts.InsertIncludes == CodeCompleteOptions::NeverInsert ||
+RankedIncludeHeaders.empty())
   return None;
 if (SemaResult && SemaResult->Declaration) {
   // Avoid inserting new #include if the declaration is found in the current
@@ -338,7 +342,7 @@
   Includes.calculateIncludePath(*ResolvedDeclaring, *ResolvedInserted),
   Includes.shouldInsertInclude(*ResolvedDeclaring, *ResolvedInserted));
 };
-bool ShouldInsert = C.headerToInsertIfAllowed().hasValue();
+bool ShouldInsert = C.headerToInsertIfAllowed(Opts).hasValue();
 // Calculate include paths and edits for all possible headers.
 for (const auto &Inc : C.RankedIncludeHeaders) {
   if (auto ToInclude = Inserted(Inc)) {
@@ -1373,7 +1377,7 @@
   if (C.IndexResult)
 C.RankedIncludeHeaders = getRankedIncludes(*C.IndexResult);
   C.Name = IndexResult ? IndexResult->Name : Reco

[PATCH] D60409: [clangd] Add -header-insertion=never flag to disable include insertion in code completion

2019-04-10 Thread Sam McCall via Phabricator via cfe-commits 
sammccall accepted this revision.
sammccall marked 2 inline comments as done.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/CodeComplete.cpp:1176
   // This is available after Sema has run.
-  llvm::Optional Inserter;  // Available during runWithSema.
+  llvm::Optional Inserter;  // Optional during runWithSema.
   llvm::Optional FileProximity; // Initialized once Sema runs.

ioeric wrote:
> Why optional? In the current implementation, it's always initialized.
Oops, this was left-over from a previous iteration.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60409



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

[PATCH] D60409: [clangd] Add -header-insertion=never flag to disable include insertion in code completion

2019-04-08 Thread Eric Liu via Phabricator via cfe-commits 
ioeric added a comment.

lg!




Comment at: clangd/CodeComplete.cpp:1176
   // This is available after Sema has run.
-  llvm::Optional Inserter;  // Available during runWithSema.
+  llvm::Optional Inserter;  // Optional during runWithSema.
   llvm::Optional FileProximity; // Initialized once Sema runs.

Why optional? In the current implementation, it's always initialized.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60409



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

[PATCH] D60409: [clangd] Add -header-insertion=never flag to disable include insertion in code completion

2019-04-08 Thread Sam McCall via Phabricator via cfe-commits 
sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

One clear use case: use with an editor that reacts poorly to edits above the 
cursor.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D60409

Files:
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -32,7 +32,6 @@
 using ::llvm::Failed;
 using ::testing::AllOf;
 using ::testing::Contains;
-using ::testing::Each;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::HasSubstr;
@@ -556,6 +555,16 @@
  {Sym});
   EXPECT_THAT(Results.Completions,
   ElementsAre(AllOf(Named("X"), InsertInclude("\"bar.h\"";
+  // Can be disabled via option.
+  CodeCompleteOptions NoInsertion;
+  NoInsertion.InsertIncludes = CodeCompleteOptions::NeverInsert;
+  Results = completions(Server,
+ R"cpp(
+  int main() { ns::^ }
+  )cpp",
+ {Sym}, NoInsertion);
+  EXPECT_THAT(Results.Completions,
+  ElementsAre(AllOf(Named("X"), Not(InsertInclude();
   // Duplicate based on inclusions in preamble.
   Results = completions(Server,
 R"cpp(
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -6,8 +6,9 @@
 //
 //===--===//
 
-#include "Features.inc"
 #include "ClangdLSPServer.h"
+#include "CodeComplete.h"
+#include "Features.inc"
 #include "Path.h"
 #include "Protocol.h"
 #include "Trace.h"
@@ -154,6 +155,20 @@
 "debug-origin", llvm::cl::desc("Show origins of completion items"),
 llvm::cl::init(CodeCompleteOptions().ShowOrigins), llvm::cl::Hidden);
 
+static llvm::cl::opt HeaderInsertion(
+"header-insertion",
+llvm::cl::desc("Add #include directives when accepting code completions"),
+llvm::cl::init(CodeCompleteOptions().InsertIncludes),
+llvm::cl::values(
+clEnumValN(CodeCompleteOptions::IWYU, "iwyu",
+   "Include what you use. "
+   "Insert the owning header for top-level symbols, unless the "
+   "header is already directly included or the symbol is "
+   "forward-declared."),
+clEnumValN(
+CodeCompleteOptions::NeverInsert, "never",
+"Never insert #include directives as part of code completion")));
+
 static llvm::cl::opt HeaderInsertionDecorators(
 "header-insertion-decorators",
 llvm::cl::desc("Prepend a circular dot or space before the completion "
@@ -430,6 +445,7 @@
   CCOpts.Limit = LimitResults;
   CCOpts.BundleOverloads = CompletionStyle != Detailed;
   CCOpts.ShowOrigins = ShowOrigins;
+  CCOpts.InsertIncludes = HeaderInsertion;
   if (!HeaderInsertionDecorators) {
 CCOpts.IncludeIndicator.Insert.clear();
 CCOpts.IncludeIndicator.NoInsert.clear();
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -68,6 +68,11 @@
   /// If more results are available, we set CompletionList.isIncomplete.
   size_t Limit = 0;
 
+  enum IncludeInsertion {
+IWYU,
+NeverInsert,
+  } InsertIncludes = IncludeInsertion::IWYU;
+
   /// A visual indicator to prepend to the completion label to indicate whether
   /// completion result would trigger an #include insertion or not.
   struct IncludeInsertionIndicator {
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -190,7 +190,9 @@
 
   // Returns a token identifying the overload set this is part of.
   // 0 indicates it's not part of any overload set.
-  size_t overloadSet() const {
+  size_t overloadSet(const CodeCompleteOptions &Opts) const {
+if (!Opts.BundleOverloads)
+  return 0;
 llvm::SmallString<256> Scratch;
 if (IndexResult) {
   switch (IndexResult->SymInfo.Kind) {
@@ -207,7 +209,7 @@
 // This could break #include insertion.
 return llvm::hash_combine(
 (IndexResult->Scope + IndexResult->Name).toStringRef(Scratch),
-headerToInsertIfAllowed().getValueOr(""));
+headerToInsertIfAllowed(Opts).getValueOr(""));
   default:
 return 0;
   }
@@ -222,12 +224,14 @@
   D->printQualifiedName(OS);
 }
 return llvm::hash_combine(Scratch,
-  headerToInsertIfAllowed().getValueOr(""));
+