r343075 - [CUDA] Fix two failed test cases using --cuda-path-ignore-env

[Jiading Gai via cfe-commits]

Author: jiadinggai
Date: Wed Sep 26 00:07:48 2018
New Revision: 343075

URL: http://llvm.org/viewvc/llvm-project?rev=343075&view=rev
Log:
[CUDA] Fix two failed test cases using --cuda-path-ignore-env

Add --cuda-path-ignore-env option to those test cases to ensure the clang 
driver always pick the CUDA path specified by --sysroot.

Reviewers: tra, Hahnfeld

Differential Revision: https://reviews.llvm.org/D52259


Modified:
cfe/trunk/test/Driver/cuda-detect.cu
cfe/trunk/test/Driver/cuda-macosx.cu

Modified: cfe/trunk/test/Driver/cuda-detect.cu
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/cuda-detect.cu?rev=343075&r1=343074&r2=343075&view=diff
==
--- cfe/trunk/test/Driver/cuda-detect.cu (original)
+++ cfe/trunk/test/Driver/cuda-detect.cu Wed Sep 26 00:07:48 2018
@@ -14,9 +14,9 @@
 
 
 // RUN: %clang -v --target=i386-unknown-linux \
-// RUN:   --sysroot=%S/Inputs/CUDA 2>&1 | FileCheck %s
+// RUN:   --sysroot=%S/Inputs/CUDA --cuda-path-ignore-env 2>&1 | FileCheck %s
 // RUN: %clang -v --target=i386-apple-macosx \
-// RUN:   --sysroot=%S/Inputs/CUDA 2>&1 | FileCheck %s
+// RUN:   --sysroot=%S/Inputs/CUDA --cuda-path-ignore-env 2>&1 | FileCheck %s
 
 // RUN: %clang -v --target=i386-unknown-linux \
 // RUN:   --cuda-path=%S/Inputs/CUDA/usr/local/cuda 2>&1 | FileCheck %s

Modified: cfe/trunk/test/Driver/cuda-macosx.cu
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/cuda-macosx.cu?rev=343075&r1=343074&r2=343075&view=diff
==
--- cfe/trunk/test/Driver/cuda-macosx.cu (original)
+++ cfe/trunk/test/Driver/cuda-macosx.cu Wed Sep 26 00:07:48 2018
@@ -3,6 +3,6 @@
 // REQUIRES: nvptx-registered-target
 //
 // RUN: %clang -v --target=i386-apple-macosx \
-// RUN:   --sysroot=%S/Inputs/CUDA-macosx 2>&1 | FileCheck %s
+// RUN:   --sysroot=%S/Inputs/CUDA-macosx --cuda-path-ignore-env 2>&1 | 
FileCheck %s
 
 // CHECK: Found CUDA installation: {{.*}}/Inputs/CUDA-macosx/usr/local/cuda


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

[PATCH] D52259: [CUDA] Fix two failed test cases using --cuda-path-ignore-env

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC343075: [CUDA] Fix two failed test cases using 
--cuda-path-ignore-env (authored by jiadinggai, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D52259

Files:
  test/Driver/cuda-detect.cu
  test/Driver/cuda-macosx.cu


Index: test/Driver/cuda-detect.cu
===
--- test/Driver/cuda-detect.cu
+++ test/Driver/cuda-detect.cu
@@ -14,9 +14,9 @@
 
 
 // RUN: %clang -v --target=i386-unknown-linux \
-// RUN:   --sysroot=%S/Inputs/CUDA 2>&1 | FileCheck %s
+// RUN:   --sysroot=%S/Inputs/CUDA --cuda-path-ignore-env 2>&1 | FileCheck %s
 // RUN: %clang -v --target=i386-apple-macosx \
-// RUN:   --sysroot=%S/Inputs/CUDA 2>&1 | FileCheck %s
+// RUN:   --sysroot=%S/Inputs/CUDA --cuda-path-ignore-env 2>&1 | FileCheck %s
 
 // RUN: %clang -v --target=i386-unknown-linux \
 // RUN:   --cuda-path=%S/Inputs/CUDA/usr/local/cuda 2>&1 | FileCheck %s
Index: test/Driver/cuda-macosx.cu
===
--- test/Driver/cuda-macosx.cu
+++ test/Driver/cuda-macosx.cu
@@ -3,6 +3,6 @@
 // REQUIRES: nvptx-registered-target
 //
 // RUN: %clang -v --target=i386-apple-macosx \
-// RUN:   --sysroot=%S/Inputs/CUDA-macosx 2>&1 | FileCheck %s
+// RUN:   --sysroot=%S/Inputs/CUDA-macosx --cuda-path-ignore-env 2>&1 | 
FileCheck %s
 
 // CHECK: Found CUDA installation: {{.*}}/Inputs/CUDA-macosx/usr/local/cuda


Index: test/Driver/cuda-detect.cu
===
--- test/Driver/cuda-detect.cu
+++ test/Driver/cuda-detect.cu
@@ -14,9 +14,9 @@
 
 
 // RUN: %clang -v --target=i386-unknown-linux \
-// RUN:   --sysroot=%S/Inputs/CUDA 2>&1 | FileCheck %s
+// RUN:   --sysroot=%S/Inputs/CUDA --cuda-path-ignore-env 2>&1 | FileCheck %s
 // RUN: %clang -v --target=i386-apple-macosx \
-// RUN:   --sysroot=%S/Inputs/CUDA 2>&1 | FileCheck %s
+// RUN:   --sysroot=%S/Inputs/CUDA --cuda-path-ignore-env 2>&1 | FileCheck %s
 
 // RUN: %clang -v --target=i386-unknown-linux \
 // RUN:   --cuda-path=%S/Inputs/CUDA/usr/local/cuda 2>&1 | FileCheck %s
Index: test/Driver/cuda-macosx.cu
===
--- test/Driver/cuda-macosx.cu
+++ test/Driver/cuda-macosx.cu
@@ -3,6 +3,6 @@
 // REQUIRES: nvptx-registered-target
 //
 // RUN: %clang -v --target=i386-apple-macosx \
-// RUN:   --sysroot=%S/Inputs/CUDA-macosx 2>&1 | FileCheck %s
+// RUN:   --sysroot=%S/Inputs/CUDA-macosx --cuda-path-ignore-env 2>&1 | FileCheck %s
 
 // CHECK: Found CUDA installation: {{.*}}/Inputs/CUDA-macosx/usr/local/cuda
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52529: [Frontend] Simplify -print-decl-contexts with DeclNodes.inc

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: rsmith, arphaman.
Herald added subscribers: cfe-commits, jfb.

Currently the switch conditions display Decl nodes with space-separated 
lowercase words. Some conditions are missing and will cause llvm_unreachable() 
failure. It is simpler to just use DeclNodes.inc names uninterpretedly.


Repository:
  rC Clang

https://reviews.llvm.org/D52529

Files:
  lib/Frontend/ASTConsumers.cpp

Index: lib/Frontend/ASTConsumers.cpp
===
--- lib/Frontend/ASTConsumers.cpp
+++ lib/Frontend/ASTConsumers.cpp
@@ -418,117 +418,27 @@
   Out << "  ";
 
 Decl::Kind DK = I->getKind();
+
+if (auto *DC = dyn_cast(I)) {
+  PrintDeclContext(DC, Indentation + 1);
+  continue;
+}
+
 switch (DK) {
-case Decl::Namespace:
-case Decl::Enum:
-case Decl::Record:
-case Decl::CXXRecord:
-case Decl::ObjCMethod:
-case Decl::ObjCInterface:
-case Decl::ObjCCategory:
-case Decl::ObjCProtocol:
-case Decl::ObjCImplementation:
-case Decl::ObjCCategoryImpl:
-case Decl::LinkageSpec:
-case Decl::Block:
-case Decl::Function:
-case Decl::CXXMethod:
-case Decl::CXXConstructor:
-case Decl::CXXDestructor:
-case Decl::CXXConversion:
-case Decl::ClassTemplateSpecialization:
-case Decl::ClassTemplatePartialSpecialization: {
-  DeclContext* DC = cast(I);
-  PrintDeclContext(DC, Indentation+2);
-  break;
-}
-case Decl::IndirectField:
-  Out << " " << *cast(I) << '\n';
-  break;
-case Decl::Label:
-  Out << " " << *cast(I) << '\n';
-  break;
-case Decl::Field:
-  Out << " " << *cast(I) << '\n';
-  break;
-case Decl::Typedef:
-case Decl::TypeAlias:
-  Out << " " << *cast(I) << '\n';
-  break;
-case Decl::EnumConstant:
-  Out << " " << *cast(I) << '\n';
-  break;
-case Decl::Var:
-  Out << " " << *cast(I) << '\n';
-  break;
-case Decl::ImplicitParam:
-  Out << " " << *cast(I) << '\n';
-  break;
-case Decl::ParmVar:
-  Out << " " << *cast(I) << '\n';
-  break;
-case Decl::ObjCProperty:
-  Out << " " << *cast(I) << '\n';
-  break;
-case Decl::FunctionTemplate:
-  Out << " " << *cast(I) << '\n';
-  break;
-case Decl::TypeAliasTemplate:
-  Out << " " << *cast(I)
-  << '\n';
-  break;
-case Decl::FileScopeAsm:
-  Out << "\n";
-  break;
-case Decl::UsingDirective:
-  Out << "\n";
-  break;
-case Decl::NamespaceAlias:
-  Out << " " << *cast(I) << '\n';
-  break;
-case Decl::ClassTemplate:
-  Out << " " << *cast(I) << '\n';
-  break;
-case Decl::OMPThreadPrivate: {
-  Out << " " << '"' << I << "\"\n";
-  break;
-}
-case Decl::Friend: {
-  Out << "";
-  if (const NamedDecl *ND = cast(I)->getFriendDecl())
-Out << ' ' << *ND;
-  Out << "\n";
-  break;
-}
-case Decl::Using:
-  Out << " " << *cast(I) << "\n";
-  break;
-case Decl::UsingShadow:
-  Out << " " << *cast(I) << "\n";
-  break;
-case Decl::UnresolvedUsingValue:
-  Out << " " << *cast(I)
-  << "\n";
-  break;
-case Decl::Empty:
-  Out << "\n";
-  break;
-case Decl::AccessSpec:
-  Out << "\n";
-  break;
-case Decl::VarTemplate:
-  Out << " " << *cast(I) << "\n";
-  break;
-case Decl::StaticAssert:
-  Out << "\n";
-  break;
-
-default:
-  Out << "DeclKind: " << DK << '"' << I << "\"\n";
-  llvm_unreachable("decl unhandled");
+#define NAMED(DERIVED, BASE) \
+case Decl::DERIVED:  \
+  Out << "<" << #DERIVED << "> " << *cast(I) << "\n"; \
+  continue;
+#define DECL(DERIVED, BASE)  \
+case Decl::DERIVED:  \
+  Out << "<" << #DERIVED << ">\n";   \
+  continue;
+#define ABSTRACT_DECL(DECL)
+#include "clang/AST/DeclNodes.inc"
 }
   }
 }
+
 std::unique_ptr clang::CreateDeclContextPrinter() {
   return llvm::make_unique();
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52530: [analyzer] scan-build: if --status-bugs is passed, don't forget about the exit status of the actual build

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: sylvestre.ledru, alexfh, jroelofs, ygribov.
Herald added subscribers: Szelethus, mikhail.ramalho, a.sidorin, szepet, 
xazax.hun.
Herald added a reviewer: george.karpenkov.

This has been bothering me for a while, but only now i have actually looked 
into this.
I'm using one CI job for static analysis - clang static analyzers as compilers 
+ clang-tidy via cmake.
And i'd like for the build to fail if at least one of those finds issues.
If clang-tidy finds issues, it will fail the build since the warnings-as-errors 
is set.
If static analyzer finds anything, since --status-bugs is set, it will fail the 
build.
But if clang-tidy find anything, but static analyzer does not, the build 
succeeds :/


Repository:
  rC Clang

https://reviews.llvm.org/D52530

Files:
  tools/scan-build/bin/scan-build


Index: tools/scan-build/bin/scan-build
===
--- tools/scan-build/bin/scan-build
+++ tools/scan-build/bin/scan-build
@@ -1908,7 +1908,7 @@
 
 if ($Options{ExitStatusFoundBugs}) {
   exit 1 if ($NumBugs > 0);
-  exit 0;
+  exit $ExitStatus;
 }
   }
 }


Index: tools/scan-build/bin/scan-build
===
--- tools/scan-build/bin/scan-build
+++ tools/scan-build/bin/scan-build
@@ -1908,7 +1908,7 @@
 
 if ($Options{ExitStatusFoundBugs}) {
   exit 1 if ($NumBugs > 0);
-  exit 0;
+  exit $ExitStatus;
 }
   }
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52499: [clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074)

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In https://reviews.llvm.org/D52499#1245474, @majnemer wrote:

> FWIW, Microsoft's newest documentation does not say that `/O2` and `/O1` 
> imply `/Gs`: 
> https://docs.microsoft.com/en-us/cpp/build/reference/o1-o2-minimize-size-maximize-speed?view=vs-2017


Great, thanks for finding that!


https://reviews.llvm.org/D52499



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

[PATCH] D52397: [libc++] Remove Fuchsia-specific knowledge to pick the ABI version

[Petr Hosek via Phabricator via cfe-commits]

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

LGTM


Repository:
  rCXX libc++

https://reviews.llvm.org/D52397



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

[PATCH] D52499: [clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074)

[Hans Wennborg via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL343077: [clang-cl] Make /Gs imply default stack probes, not 
/Gs0 (PR39074) (authored by hans, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52499?vs=166918&id=167058#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52499

Files:
  cfe/trunk/include/clang/Driver/CLCompatOptions.td
  cfe/trunk/test/Driver/cl-options.c


Index: cfe/trunk/include/clang/Driver/CLCompatOptions.td
===
--- cfe/trunk/include/clang/Driver/CLCompatOptions.td
+++ cfe/trunk/include/clang/Driver/CLCompatOptions.td
@@ -90,8 +90,8 @@
   Alias;
 def _SLASH_GS : CLFlag<"GS">, HelpText<"Enable buffer security check 
(default)">;
 def _SLASH_GS_ : CLFlag<"GS-">, HelpText<"Disable buffer security check">;
-// FIXME: Not sure /Gs really means /Gs0 (see PR39074).
-def : CLFlag<"Gs">, HelpText<"Same as /Gs0">, Alias, 
AliasArgs<["0"]>;
+def : CLFlag<"Gs">, HelpText<"Use stack probes (default)">,
+  Alias, AliasArgs<["4096"]>;
 def _SLASH_Gs : CLJoined<"Gs">,
   HelpText<"Set stack probe size (default 4096)">, Alias;
 def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">,
@@ -121,9 +121,9 @@
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;
 def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>,
-  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /Gs /GF /Gy)">;
+  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>,
-  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /Gs /GF 
/Gy)">;
+  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>,
   HelpText<"Disable function inlining">;
 def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>,
Index: cfe/trunk/test/Driver/cl-options.c
===
--- cfe/trunk/test/Driver/cl-options.c
+++ cfe/trunk/test/Driver/cl-options.c
@@ -101,7 +101,7 @@
 // Gy_-NOT: -ffunction-sections
 
 // RUN: %clang_cl /Gs -### -- %s 2>&1 | FileCheck -check-prefix=Gs %s
-// Gs: "-mstack-probe-size=0"
+// Gs: "-mstack-probe-size=4096"
 // RUN: %clang_cl /Gs0 -### -- %s 2>&1 | FileCheck -check-prefix=Gs0 %s
 // Gs0: "-mstack-probe-size=0"
 // RUN: %clang_cl /Gs4096 -### -- %s 2>&1 | FileCheck -check-prefix=Gs4096 %s


Index: cfe/trunk/include/clang/Driver/CLCompatOptions.td
===
--- cfe/trunk/include/clang/Driver/CLCompatOptions.td
+++ cfe/trunk/include/clang/Driver/CLCompatOptions.td
@@ -90,8 +90,8 @@
   Alias;
 def _SLASH_GS : CLFlag<"GS">, HelpText<"Enable buffer security check (default)">;
 def _SLASH_GS_ : CLFlag<"GS-">, HelpText<"Disable buffer security check">;
-// FIXME: Not sure /Gs really means /Gs0 (see PR39074).
-def : CLFlag<"Gs">, HelpText<"Same as /Gs0">, Alias, AliasArgs<["0"]>;
+def : CLFlag<"Gs">, HelpText<"Use stack probes (default)">,
+  Alias, AliasArgs<["4096"]>;
 def _SLASH_Gs : CLJoined<"Gs">,
   HelpText<"Set stack probe size (default 4096)">, Alias;
 def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">,
@@ -121,9 +121,9 @@
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;
 def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>,
-  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /Gs /GF /Gy)">;
+  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>,
-  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /Gs /GF /Gy)">;
+  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>,
   HelpText<"Disable function inlining">;
 def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>,
Index: cfe/trunk/test/Driver/cl-options.c
===
--- cfe/trunk/test/Driver/cl-options.c
+++ cfe/trunk/test/Driver/cl-options.c
@@ -101,7 +101,7 @@
 // Gy_-NOT: -ffunction-sections
 
 // RUN: %clang_cl /Gs -### -- %s 2>&1 | FileCheck -check-prefix=Gs %s
-// Gs: "-mstack-probe-size=0"
+// Gs: "-mstack-probe-size=4096"
 // RUN: %clang_cl /Gs0 -### -- %s 2>&1 | FileCheck -check-prefix=Gs0 %s
 // Gs0: "-mstack-probe-size=0"
 // RUN: %clang_cl /Gs4096 -### -- %s 2>&1 | FileCheck -check-prefix=Gs4096 %s
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r343077 - [clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074)

[Hans Wennborg via cfe-commits]

Author: hans
Date: Wed Sep 26 00:39:04 2018
New Revision: 343077

URL: http://llvm.org/viewvc/llvm-project?rev=343077&view=rev
Log:
[clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074)

Differential revision: https://reviews.llvm.org/D52499

Modified:
cfe/trunk/include/clang/Driver/CLCompatOptions.td
cfe/trunk/test/Driver/cl-options.c

Modified: cfe/trunk/include/clang/Driver/CLCompatOptions.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/CLCompatOptions.td?rev=343077&r1=343076&r2=343077&view=diff
==
--- cfe/trunk/include/clang/Driver/CLCompatOptions.td (original)
+++ cfe/trunk/include/clang/Driver/CLCompatOptions.td Wed Sep 26 00:39:04 2018
@@ -90,8 +90,8 @@ def _SLASH_GF_ : CLFlag<"GF-">, HelpText
   Alias;
 def _SLASH_GS : CLFlag<"GS">, HelpText<"Enable buffer security check 
(default)">;
 def _SLASH_GS_ : CLFlag<"GS-">, HelpText<"Disable buffer security check">;
-// FIXME: Not sure /Gs really means /Gs0 (see PR39074).
-def : CLFlag<"Gs">, HelpText<"Same as /Gs0">, Alias, 
AliasArgs<["0"]>;
+def : CLFlag<"Gs">, HelpText<"Use stack probes (default)">,
+  Alias, AliasArgs<["4096"]>;
 def _SLASH_Gs : CLJoined<"Gs">,
   HelpText<"Set stack probe size (default 4096)">, Alias;
 def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">,
@@ -121,9 +121,9 @@ def _SLASH_O : CLJoined<"O">,
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;
 def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>,
-  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /Gs /GF /Gy)">;
+  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>,
-  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /Gs /GF 
/Gy)">;
+  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>,
   HelpText<"Disable function inlining">;
 def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>,

Modified: cfe/trunk/test/Driver/cl-options.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/cl-options.c?rev=343077&r1=343076&r2=343077&view=diff
==
--- cfe/trunk/test/Driver/cl-options.c (original)
+++ cfe/trunk/test/Driver/cl-options.c Wed Sep 26 00:39:04 2018
@@ -101,7 +101,7 @@
 // Gy_-NOT: -ffunction-sections
 
 // RUN: %clang_cl /Gs -### -- %s 2>&1 | FileCheck -check-prefix=Gs %s
-// Gs: "-mstack-probe-size=0"
+// Gs: "-mstack-probe-size=4096"
 // RUN: %clang_cl /Gs0 -### -- %s 2>&1 | FileCheck -check-prefix=Gs0 %s
 // Gs0: "-mstack-probe-size=0"
 // RUN: %clang_cl /Gs4096 -### -- %s 2>&1 | FileCheck -check-prefix=Gs4096 %s


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

[PATCH] D52499: [clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074)

[Hans Wennborg via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC343077: [clang-cl] Make /Gs imply default stack probes, not 
/Gs0 (PR39074) (authored by hans, committed by ).

Repository:
  rL LLVM

https://reviews.llvm.org/D52499

Files:
  include/clang/Driver/CLCompatOptions.td
  test/Driver/cl-options.c


Index: include/clang/Driver/CLCompatOptions.td
===
--- include/clang/Driver/CLCompatOptions.td
+++ include/clang/Driver/CLCompatOptions.td
@@ -90,8 +90,8 @@
   Alias;
 def _SLASH_GS : CLFlag<"GS">, HelpText<"Enable buffer security check 
(default)">;
 def _SLASH_GS_ : CLFlag<"GS-">, HelpText<"Disable buffer security check">;
-// FIXME: Not sure /Gs really means /Gs0 (see PR39074).
-def : CLFlag<"Gs">, HelpText<"Same as /Gs0">, Alias, 
AliasArgs<["0"]>;
+def : CLFlag<"Gs">, HelpText<"Use stack probes (default)">,
+  Alias, AliasArgs<["4096"]>;
 def _SLASH_Gs : CLJoined<"Gs">,
   HelpText<"Set stack probe size (default 4096)">, Alias;
 def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">,
@@ -121,9 +121,9 @@
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;
 def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>,
-  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /Gs /GF /Gy)">;
+  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>,
-  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /Gs /GF 
/Gy)">;
+  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>,
   HelpText<"Disable function inlining">;
 def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>,
Index: test/Driver/cl-options.c
===
--- test/Driver/cl-options.c
+++ test/Driver/cl-options.c
@@ -101,7 +101,7 @@
 // Gy_-NOT: -ffunction-sections
 
 // RUN: %clang_cl /Gs -### -- %s 2>&1 | FileCheck -check-prefix=Gs %s
-// Gs: "-mstack-probe-size=0"
+// Gs: "-mstack-probe-size=4096"
 // RUN: %clang_cl /Gs0 -### -- %s 2>&1 | FileCheck -check-prefix=Gs0 %s
 // Gs0: "-mstack-probe-size=0"
 // RUN: %clang_cl /Gs4096 -### -- %s 2>&1 | FileCheck -check-prefix=Gs4096 %s


Index: include/clang/Driver/CLCompatOptions.td
===
--- include/clang/Driver/CLCompatOptions.td
+++ include/clang/Driver/CLCompatOptions.td
@@ -90,8 +90,8 @@
   Alias;
 def _SLASH_GS : CLFlag<"GS">, HelpText<"Enable buffer security check (default)">;
 def _SLASH_GS_ : CLFlag<"GS-">, HelpText<"Disable buffer security check">;
-// FIXME: Not sure /Gs really means /Gs0 (see PR39074).
-def : CLFlag<"Gs">, HelpText<"Same as /Gs0">, Alias, AliasArgs<["0"]>;
+def : CLFlag<"Gs">, HelpText<"Use stack probes (default)">,
+  Alias, AliasArgs<["4096"]>;
 def _SLASH_Gs : CLJoined<"Gs">,
   HelpText<"Set stack probe size (default 4096)">, Alias;
 def _SLASH_Gy : CLFlag<"Gy">, HelpText<"Put each function in its own section">,
@@ -121,9 +121,9 @@
 // FIXME: Not sure why we have -O0 here; MSVC doesn't support that.
 def : CLFlag<"O0">, Alias, HelpText<"Disable optimization">;
 def : CLFlag<"O1">, Alias<_SLASH_O>, AliasArgs<["1"]>,
-  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /Gs /GF /Gy)">;
+  HelpText<"Optimize for size (equivalent to /Og /Os /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"O2">, Alias<_SLASH_O>, AliasArgs<["2"]>,
-  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /Gs /GF /Gy)">;
+  HelpText<"Optimize for speed (equivalent to /Og /Oi /Ot /Oy /Ob2 /GF /Gy)">;
 def : CLFlag<"Ob0">, Alias<_SLASH_O>, AliasArgs<["b0"]>,
   HelpText<"Disable function inlining">;
 def : CLFlag<"Ob1">, Alias<_SLASH_O>, AliasArgs<["b1"]>,
Index: test/Driver/cl-options.c
===
--- test/Driver/cl-options.c
+++ test/Driver/cl-options.c
@@ -101,7 +101,7 @@
 // Gy_-NOT: -ffunction-sections
 
 // RUN: %clang_cl /Gs -### -- %s 2>&1 | FileCheck -check-prefix=Gs %s
-// Gs: "-mstack-probe-size=0"
+// Gs: "-mstack-probe-size=4096"
 // RUN: %clang_cl /Gs0 -### -- %s 2>&1 | FileCheck -check-prefix=Gs0 %s
 // Gs0: "-mstack-probe-size=0"
 // RUN: %clang_cl /Gs4096 -### -- %s 2>&1 | FileCheck -check-prefix=Gs4096 %s
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52530: [analyzer] scan-build: if --status-bugs is passed, don't forget about the exit status of the actual build

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 167060.
lebedev.ri added a reviewer: krememek.
lebedev.ri added a comment.

Updated the `--help`, too.


Repository:
  rC Clang

https://reviews.llvm.org/D52530

Files:
  tools/scan-build/bin/scan-build


Index: tools/scan-build/bin/scan-build
===
--- tools/scan-build/bin/scan-build
+++ tools/scan-build/bin/scan-build
@@ -1207,7 +1207,7 @@
 
By default, the exit status of scan-build is the same as the executed build
command. Specifying this option causes the exit status of scan-build to be 1
-   if it found potential bugs and 0 otherwise.
+   if it found potential bugs and the exit status of the build itself 
otherwise.
 
  --exclude 
 
@@ -1908,7 +1908,7 @@
 
 if ($Options{ExitStatusFoundBugs}) {
   exit 1 if ($NumBugs > 0);
-  exit 0;
+  exit $ExitStatus;
 }
   }
 }


Index: tools/scan-build/bin/scan-build
===
--- tools/scan-build/bin/scan-build
+++ tools/scan-build/bin/scan-build
@@ -1207,7 +1207,7 @@
 
By default, the exit status of scan-build is the same as the executed build
command. Specifying this option causes the exit status of scan-build to be 1
-   if it found potential bugs and 0 otherwise.
+   if it found potential bugs and the exit status of the build itself otherwise.
 
  --exclude 
 
@@ -1908,7 +1908,7 @@
 
 if ($Options{ExitStatusFoundBugs}) {
   exit 1 if ($NumBugs > 0);
-  exit 0;
+  exit $ExitStatus;
 }
   }
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52531: [clangd] clangd-indexer gathers refs and stores them in index files.

[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.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52531

Files:
  clangd/index/IndexAction.cpp
  clangd/index/IndexAction.h
  clangd/index/Serialization.cpp
  clangd/index/Serialization.h
  clangd/index/YAMLSerialization.cpp
  clangd/indexer/IndexerMain.cpp
  unittests/clangd/SerializationTests.cpp

Index: unittests/clangd/SerializationTests.cpp
===
--- unittests/clangd/SerializationTests.cpp
+++ unittests/clangd/SerializationTests.cpp
@@ -13,15 +13,18 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+using testing::_;
 using testing::AllOf;
+using testing::Pair;
 using testing::UnorderedElementsAre;
 using testing::UnorderedElementsAreArray;
 namespace clang {
 namespace clangd {
 namespace {
 
 const char *YAML = R"(
 ---
+!Symbol
 ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF856
 Name:   'Foo1'
 Scope:   'clang::'
@@ -47,6 +50,7 @@
 References:3
 ...
 ---
+!Symbol
 ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF858
 Name:   'Foo2'
 Scope:   'clang::'
@@ -65,6 +69,18 @@
 Signature:'-sig'
 CompletionSnippetSuffix:'-snippet'
 ...
+!Refs
+ID: 057557CEBF6E6B2DD437FBF60CC58F352D1DF856
+References:
+  - Kind: 4
+Location:
+  FileURI:file:///path/foo.cc
+  Start:
+Line: 5
+Column: 3
+  End:
+Line: 5
+Column: 8
 )";
 
 MATCHER_P(ID, I, "") { return arg.ID == cantFail(SymbolID::fromStr(I)); }
@@ -108,32 +124,56 @@
   EXPECT_EQ(Sym2.CanonicalDeclaration.FileURI, "file:///path/bar.h");
   EXPECT_FALSE(Sym2.Flags & Symbol::IndexedForCodeCompletion);
   EXPECT_TRUE(Sym2.Flags & Symbol::Deprecated);
+
+  ASSERT_TRUE(bool(ParsedYAML->Refs));
+  EXPECT_THAT(*ParsedYAML->Refs,
+  UnorderedElementsAre(
+  Pair(cantFail(SymbolID::fromStr(
+   "057557CEBF6E6B2DD437FBF60CC58F352D1DF856")),
+   testing::SizeIs(1;
+  auto Ref1 = ParsedYAML->Refs->begin()->second.front();
+  EXPECT_EQ(Ref1.Kind, RefKind::Reference);
+  EXPECT_EQ(Ref1.Location.FileURI, "file:///path/foo.cc");
 }
 
 std::vector YAMLFromSymbols(const SymbolSlab &Slab) {
   std::vector Result;
   for (const auto &Sym : Slab)
 Result.push_back(toYAML(Sym));
   return Result;
 }
+std::vector YAMLFromRefs(const RefSlab &Slab) {
+  std::vector Result;
+  for (const auto &Sym : Slab)
+Result.push_back(toYAML(Sym));
+  return Result;
+}
+
 
 TEST(SerializationTest, BinaryConversions) {
   auto In = readIndexFile(YAML);
   EXPECT_TRUE(bool(In)) << In.takeError();
 
   // Write to binary format, and parse again.
-  IndexFileOut Out;
-  Out.Symbols = In->Symbols.getPointer();
+  IndexFileOut Out(*In);
   Out.Format = IndexFileFormat::RIFF;
   std::string Serialized = llvm::to_string(Out);
+  {
+std::error_code EC;
+llvm::raw_fd_ostream F("/tmp/foo",EC);
+F << Serialized;
+  }
 
   auto In2 = readIndexFile(Serialized);
   ASSERT_TRUE(bool(In2)) << In.takeError();
-  ASSERT_TRUE(In->Symbols);
+  ASSERT_TRUE(In2->Symbols);
+  ASSERT_TRUE(In2->Refs);
 
   // Assert the YAML serializations match, for nice comparisons and diffs.
   EXPECT_THAT(YAMLFromSymbols(*In2->Symbols),
   UnorderedElementsAreArray(YAMLFromSymbols(*In->Symbols)));
+  EXPECT_THAT(YAMLFromRefs(*In2->Refs),
+  UnorderedElementsAreArray(YAMLFromRefs(*In->Refs)));
 }
 
 } // namespace
Index: clangd/indexer/IndexerMain.cpp
===
--- clangd/indexer/IndexerMain.cpp
+++ clangd/indexer/IndexerMain.cpp
@@ -77,9 +77,10 @@
 
   /// Consume a SymbolSlab build for a file.
   virtual void consumeSymbols(SymbolSlab Symbols) = 0;
+  virtual void consumeRefs(RefSlab Refs) = 0;
   /// Produce a resulting symbol slab, by combining  occurrences of the same
   /// symbols across translation units.
-  virtual SymbolSlab mergeResults() = 0;
+  virtual std::pair mergeResults() = 0;
 };
 
 class SymbolIndexActionFactory : public tooling::FrontendActionFactory {
@@ -91,7 +92,8 @@
 CollectorOpts.FallbackDir = AssumedHeaderDir;
 return createStaticIndexingAction(
CollectorOpts,
-   [&](SymbolSlab S) { Consumer.consumeSymbols(std::move(S)); })
+   [&](SymbolSlab S) { Consumer.consumeSymbols(std::move(S)); },
+   [&](RefSlab S) { Consumer.consumeRefs(std::move(S)); })
 .release();
   }
 
@@ -109,8 +111,9 @@
 for (const auto &Sym : Symbols)
   Executor.getExecutionContext()->reportResult(Sym.ID.str(), toYAML(Sym));
   }
+  void consumeRefs(RefSlab) override {}
 
-  SymbolSlab mergeResults() override {
+  std::pair mergeResults() override {
 SymbolSlab::Builder UniqueSymbols;
 Executor.getToolResults()->forEachResult(
 [&](llvm::StringRef Key, llvm::StringRef Value) {
@@ 

[PATCH] D52492: [AArch64][v8.5A] Test optional Armv8.5-A random number extension

[Sjoerd Meijer via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D52492



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

[PATCH] D52493: [AArch64][v8.5A] Test clang option for the Memory Tagging Extension

[Sjoerd Meijer via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D52493



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

[PATCH] D52503: [clangd] More precise index memory usage estimate

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:251
   Bytes += InvertedIndex.getMemorySize();
-  for (const auto &P : InvertedIndex)
-Bytes += P.second.bytes();
+  for (const auto &TokenToPostingList : InvertedIndex)
+Bytes += TokenToPostingList.first.Data.size() +

ioeric wrote:
> kbobyrev wrote:
> > ioeric wrote:
> > > Would `InvertedIndex.getMemorySize()` be a better estimate?
> > IIUC this is only precise for the objects which do not make any allocations 
> > (e.g. `std::vector` and `std::string` memory estimate would not be 
> > "correct").
> > 
> > From the documentation
> > 
> > > This is just the raw memory used by DenseMap. If entries are pointers to 
> > > objects, the size of the referenced objects are not included.
> > 
> > I also have `Bytes += InvertedIndex.getMemorySize();` above, so the purpose 
> > of this code would be to take into account the size of these "reference 
> > objects".
> > 
> > However, since `PostingList::bytes()` also returns underlying `std::vector` 
> > size, this code will probably add these `std::vector` objects size twice 
> > (the first one was in `InvertedIndex.getMemorySize()`). I should fix that.
> > `If entries are pointers to objects, the size of the referenced objects are 
> > not included.`
> This applies to *pointers* to objects, but the `PostingList`s in InvertedList 
> are not pointerd? So it seems to me that `InvertedIndex.getMemorySize()` 
> covers everything in the `InvertedIndex`. One way to verify is probably check 
> the size calculated with loop against `InvertedIndex.getMemorySize()`.
As discussed offline, pointers and references are an example of objects which 
would be incorrectly measured by `DesneMap::getMemorySize()`, but `std::vector` 
and `std::string` are pointers in a way because they also just store a pointer 
to the underlying storage which is hidden from `DenseMap::getMemorySize()`; 
thus, it would be more precise to use the proposed scheme for memory estimation.


https://reviews.llvm.org/D52503



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

[PATCH] D52503: [clangd] More precise index memory usage estimate

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 167062.
kbobyrev edited the summary of this revision.
kbobyrev added a comment.

- Prevent `sizeof(std::vector)` to be counted twice in the memory 
estimates
- Pull memory usage logging to the caller side: it is currently incorrect 
because `Dex::BackingDataSize` is not set by the time `vlog(..., 
estimateMemoryUsage())` is called and hence the log only includes size of the 
index overhead

I will remove these changes from https://reviews.llvm.org/D52047 and hopefully 
make it easier to review.


https://reviews.llvm.org/D52503

Files:
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/clangd/index/dex/PostingList.h


Index: clang-tools-extra/clangd/index/dex/PostingList.h
===
--- clang-tools-extra/clangd/index/dex/PostingList.h
+++ clang-tools-extra/clangd/index/dex/PostingList.h
@@ -66,10 +66,8 @@
   /// go through the chunks and decompress them on-the-fly when necessary.
   std::unique_ptr iterator() const;
 
-  /// Returns in-memory size.
-  size_t bytes() const {
-return sizeof(Chunk) + Chunks.capacity() * sizeof(Chunk);
-  }
+  /// Returns in-memory size of underlying storage.
+  size_t bytes() const { return Chunks.capacity() * sizeof(Chunk); }
 
 private:
   const std::vector Chunks;
Index: clang-tools-extra/clangd/index/dex/Dex.cpp
===
--- clang-tools-extra/clangd/index/dex/Dex.cpp
+++ clang-tools-extra/clangd/index/dex/Dex.cpp
@@ -130,9 +130,6 @@
   for (const auto &TokenToPostingList : TempInvertedIndex)
 InvertedIndex.insert(
 {TokenToPostingList.first, PostingList(TokenToPostingList.second)});
-
-  vlog("Built Dex with estimated memory usage {0} bytes.",
-   estimateMemoryUsage());
 }
 
 /// Constructs iterators over tokens extracted from the query and exhausts it
@@ -248,8 +245,9 @@
   Bytes += SymbolQuality.size() * sizeof(float);
   Bytes += LookupTable.getMemorySize();
   Bytes += InvertedIndex.getMemorySize();
-  for (const auto &P : InvertedIndex)
-Bytes += P.second.bytes();
+  for (const auto &TokenToPostingList : InvertedIndex)
+Bytes += TokenToPostingList.first.Data.size() +
+ TokenToPostingList.second.bytes();
   return Bytes + BackingDataSize;
 }
 
Index: clang-tools-extra/clangd/index/Serialization.cpp
===
--- clang-tools-extra/clangd/index/Serialization.cpp
+++ clang-tools-extra/clangd/index/Serialization.cpp
@@ -6,8 +6,10 @@
 // License. See LICENSE.TXT for details.
 //
 
//===--===//
+
 #include "Serialization.h"
 #include "Index.h"
+#include "Logger.h"
 #include "RIFF.h"
 #include "Trace.h"
 #include "dex/Dex.h"
@@ -433,8 +435,12 @@
   }
 
   trace::Span Tracer("BuildIndex");
-  return UseDex ? dex::Dex::build(std::move(Symbols), URISchemes)
-: MemIndex::build(std::move(Symbols), std::move(Refs));
+  auto Index = UseDex ? dex::Dex::build(std::move(Symbols), URISchemes)
+  : MemIndex::build(std::move(Symbols), std::move(Refs));
+  vlog("Loaded {0} from {1} with estimated memory usage {2}",
+   UseDex ? "Dex" : "MemIndex", SymbolFilename,
+   Index->estimateMemoryUsage());
+  return Index;
 }
 
 } // namespace clangd


Index: clang-tools-extra/clangd/index/dex/PostingList.h
===
--- clang-tools-extra/clangd/index/dex/PostingList.h
+++ clang-tools-extra/clangd/index/dex/PostingList.h
@@ -66,10 +66,8 @@
   /// go through the chunks and decompress them on-the-fly when necessary.
   std::unique_ptr iterator() const;
 
-  /// Returns in-memory size.
-  size_t bytes() const {
-return sizeof(Chunk) + Chunks.capacity() * sizeof(Chunk);
-  }
+  /// Returns in-memory size of underlying storage.
+  size_t bytes() const { return Chunks.capacity() * sizeof(Chunk); }
 
 private:
   const std::vector Chunks;
Index: clang-tools-extra/clangd/index/dex/Dex.cpp
===
--- clang-tools-extra/clangd/index/dex/Dex.cpp
+++ clang-tools-extra/clangd/index/dex/Dex.cpp
@@ -130,9 +130,6 @@
   for (const auto &TokenToPostingList : TempInvertedIndex)
 InvertedIndex.insert(
 {TokenToPostingList.first, PostingList(TokenToPostingList.second)});
-
-  vlog("Built Dex with estimated memory usage {0} bytes.",
-   estimateMemoryUsage());
 }
 
 /// Constructs iterators over tokens extracted from the query and exhausts it
@@ -248,8 +245,9 @@
   Bytes += SymbolQuality.size() * sizeof(float);
   Bytes += LookupTable.getMemorySize();
   Bytes += InvertedIndex.getMemorySize();
-  for (const auto &P : InvertedIndex)
-Bytes += P.second.bytes();
+  for (const auto &TokenToPostingList : InvertedIndex)
+Bytes += TokenToPostingList.first.Dat

[PATCH] D52296: [Clang] - Add -gsingle-file-split-dwarf option.

[George Rimar via Phabricator via cfe-commits]

grimar added a comment.

In https://reviews.llvm.org/D52296#1243688, @echristo wrote:

> In https://reviews.llvm.org/D52296#1241928, @probinson wrote:
>
> > Do we generate the .dwo file directly these days?  If not, I can imagine 
> > wanting to avoid the overhead of the objcopy hack; as long as the linker is 
> > smart enough not to bother with the .debug_*.dwo sections this seems like a 
> > build-time win.
>
>
> We do generate them generically with no objcopy hack.


Eric, could you elaborate for me your position, please

> As far as the standard text here, IMO it was just there in case people didn't 
> have an objcopy around or don't want to split it.

Yeah, we do not want to split it and I see no other way to say clang to keep 
them in a .o files rather than introducing the new flag.
Am I missing something?

> I'm not sure why we would want the ability. 
>  That said, if we do I'd rather have it as dwarf5 without split-dwarf as an 
> option rather than a -gsingle-file-split-dwarf option.

What do you mean as "dwarf5 without split-dwarf as an option" here? Do you mean 
to do split-dwarf by default? It is orthogonal to what this patch does.


https://reviews.llvm.org/D52296



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

[PATCH] D52533: [test] Use --sysroot instead of -B in print-multi-directory.c

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added reviewers: chrib, dyung.

This avoids finding a similar matching GCC installation outside of the test 
directory tree in the surrounding environment, which would make the test fail.


Repository:
  rC Clang

https://reviews.llvm.org/D52533

Files:
  test/Driver/print-multi-directory.c


Index: test/Driver/print-multi-directory.c
===
--- test/Driver/print-multi-directory.c
+++ test/Driver/print-multi-directory.c
@@ -1,15 +1,15 @@
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>/dev/null \
 // RUN: -target i386-none-linux \
-// RUN: -B%S/Inputs/multilib_64bit_linux_tree/usr \
+// RUN: --sysroot %S/Inputs/multilib_64bit_linux_tree/usr \
 // RUN: -print-multi-directory \
 // RUN:   | FileCheck --match-full-lines --check-prefix=CHECK-X86-MULTILIBS %s
 
 // CHECK-X86-MULTILIBS:  32
 // CHECK-X86-MULTILIBS-NOT:  {{^.+$}}
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>/dev/null \
 // RUN: -target i386-none-linux -m64 \
-// RUN: -B%S/Inputs/multilib_64bit_linux_tree/usr \
+// RUN: --sysroot %S/Inputs/multilib_64bit_linux_tree/usr \
 // RUN: -print-multi-directory \
 // RUN:   | FileCheck --match-full-lines --check-prefix=CHECK-X86_64-MULTILIBS 
%s
 


Index: test/Driver/print-multi-directory.c
===
--- test/Driver/print-multi-directory.c
+++ test/Driver/print-multi-directory.c
@@ -1,15 +1,15 @@
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>/dev/null \
 // RUN: -target i386-none-linux \
-// RUN: -B%S/Inputs/multilib_64bit_linux_tree/usr \
+// RUN: --sysroot %S/Inputs/multilib_64bit_linux_tree/usr \
 // RUN: -print-multi-directory \
 // RUN:   | FileCheck --match-full-lines --check-prefix=CHECK-X86-MULTILIBS %s
 
 // CHECK-X86-MULTILIBS:  32
 // CHECK-X86-MULTILIBS-NOT:  {{^.+$}}
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>/dev/null \
 // RUN: -target i386-none-linux -m64 \
-// RUN: -B%S/Inputs/multilib_64bit_linux_tree/usr \
+// RUN: --sysroot %S/Inputs/multilib_64bit_linux_tree/usr \
 // RUN: -print-multi-directory \
 // RUN:   | FileCheck --match-full-lines --check-prefix=CHECK-X86_64-MULTILIBS %s
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52047: [clangd] Add building benchmark and memory consumption tracking

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 167067.
kbobyrev marked 2 inline comments as done.
kbobyrev added a comment.

Don't include misc changes elsewhere: focus on adding more benchmarks in this 
revision.


https://reviews.llvm.org/D52047

Files:
  clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp

Index: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
===
--- clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
+++ clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
@@ -33,6 +33,19 @@
   return loadIndex(IndexFilename, {}, true);
 }
 
+SymbolSlab loadSlab() {
+  auto IndexFile = readIndexFile(IndexFilename);
+  if (!IndexFile) {
+llvm::errs() << llvm::toString(IndexFile.takeError()) << '\n';
+exit(1);
+  } else if (!IndexFile->Symbols) {
+llvm::errs() << "Couldn't read symbol slab from the index file: "
+ << IndexFilename << '\n';
+exit(1);
+  }
+  return std::move(*IndexFile->Symbols);
+}
+
 // Reads JSON array of serialized FuzzyFindRequest's from user-provided file.
 std::vector extractQueriesFromLogs() {
   std::ifstream InputStream(RequestsFilename);
@@ -73,24 +86,81 @@
 for (const auto &Request : Requests)
   Mem->fuzzyFind(Request, [](const Symbol &S) {});
 }
-BENCHMARK(MemQueries);
+BENCHMARK(MemQueries)->Unit(benchmark::kMillisecond);
 
 static void DexQueries(benchmark::State &State) {
   const auto Dex = buildDex();
   const auto Requests = extractQueriesFromLogs();
   for (auto _ : State)
 for (const auto &Request : Requests)
   Dex->fuzzyFind(Request, [](const Symbol &S) {});
 }
-BENCHMARK(DexQueries);
+BENCHMARK(DexQueries)->Unit(benchmark::kMillisecond);
+
+// This is not a *real* benchmark: it shows size of built MemIndex (in bytes).
+// Same for the next "benchmark".
+// FIXME(kbobyrev): Track memory usage caused by different index parts:
+// SymbolSlab and RefSlabs, InverseIndex, PostingLists of different types for
+// Dex, etc.
+static void MemSize(benchmark::State &State) {
+  const auto Mem = buildMem();
+  for (auto _ : State)
+// Divide size of Mem by 1000 so that it will be correctly displayed in the
+// benchmark report (possible options for time units are ms, ns and us).
+State.SetIterationTime(/*double Seconds=*/Mem->estimateMemoryUsage() /
+   1000.0);
+  State.SetLabel("This tracks in-memory size of MemIndex (SymbolSlab + Index "
+ "overhead) in bytes");
+}
+BENCHMARK(MemSize)
+->UseManualTime()
+->Unit(benchmark::kMillisecond)
+->Iterations(1);
+
+static void DexSize(benchmark::State &State) {
+  const auto Dex = buildDex();
+
+  for (auto _ : State)
+State.SetIterationTime(Dex->estimateMemoryUsage() / 1000.0);
+  State.SetLabel("This tracks in-memory size of Dex (SymbolSlab + Index "
+ "overhead) in bytes");
+}
+BENCHMARK(DexSize)
+->UseManualTime()
+->Unit(benchmark::kMillisecond)
+->Iterations(1);
+
+static void DexOverhead(benchmark::State &State) {
+  const auto Slab = loadSlab();
+  const auto Dex = buildDex();
+  for (auto _ : State)
+State.SetIterationTime((Dex->estimateMemoryUsage() - Slab.bytes()) /
+   1000.0);
+  State.SetLabel("This tracks in-memory size of Dex Index overhead (and "
+ "excludes underlying SymbolSlab size) in bytes");
+}
+BENCHMARK(DexOverhead)
+->UseManualTime()
+->Unit(benchmark::kMillisecond)
+->Iterations(1);
+
+static void SymbolSlabLoading(benchmark::State &State) {
+  for (auto _ : State)
+benchmark::DoNotOptimize(loadSlab());
+}
+BENCHMARK(SymbolSlabLoading)->Unit(benchmark::kMillisecond);
+
+static void DexBuild(benchmark::State &State) {
+  auto Slab = loadSlab();
+  for (auto _ : State)
+benchmark::DoNotOptimize(dex::Dex::build(std::move(Slab), {}));
+}
+BENCHMARK(DexBuild)->Unit(benchmark::kMillisecond);
 
 } // namespace
 } // namespace clangd
 } // namespace clang
 
-// FIXME(kbobyrev): Add index building time benchmarks.
-// FIXME(kbobyrev): Add memory consumption "benchmarks" by manually measuring
-// in-memory index size and reporting it as time.
 // FIXME(kbobyrev): Create a logger wrapper to suppress debugging info printer.
 int main(int argc, char *argv[]) {
   if (argc < 3) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52047: [clangd] Add building benchmark and memory consumption tracking

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:96
 
-static void DexQueries(benchmark::State &State) {
+// This is not a *real* benchmark: it shows size of built MemIndex (in bytes).
+// Same for the next "benchmark".

sammccall wrote:
> This looks like it'll be really stable, why does it need to be a benchmark vs 
> say a dexp subcommand?
As discussed offline, this is meant to make it easier for people to investigate 
memory + performance changes and simplify the development pipeline as opposed 
to remembering multiple binaries and their options and running all these 
binaries after each change.


https://reviews.llvm.org/D52047



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

[PATCH] D52534: [clangd] NFC: Update Serialization routine documentation

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added reviewers: sammccall, ioeric.
kbobyrev added a project: clang-tools-extra.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay.

According to new implementation, `readIndexFile` can also process YAML symbol 
collections. Also, it might make sense to use Doxygen comments here so that the 
Doxygen documentation would include short description of each function.


https://reviews.llvm.org/D52534

Files:
  clang-tools-extra/clangd/index/Serialization.h


Index: clang-tools-extra/clangd/index/Serialization.h
===
--- clang-tools-extra/clangd/index/Serialization.h
+++ clang-tools-extra/clangd/index/Serialization.h
@@ -40,14 +40,15 @@
   YAML, // Human-readable format, suitable for experiments and debugging.
 };
 
-// Holds the contents of an index file that was read.
+/// Holds the contents of an index file that was read.
 struct IndexFileIn {
   llvm::Optional Symbols;
 };
-// Parse an index file. The input must be a RIFF container chunk.
+/// Parse an index file. The input must be a RIFF container chunk or YAML 
symbol
+/// collection.
 llvm::Expected readIndexFile(llvm::StringRef);
 
-// Specifies the contents of an index file to be written.
+/// Specifies the contents of an index file to be written.
 struct IndexFileOut {
   const SymbolSlab *Symbols;
   // TODO: Support serializing symbol occurrences.
@@ -58,16 +59,16 @@
   IndexFileOut(const IndexFileIn &I)
   : Symbols(I.Symbols ? I.Symbols.getPointer() : nullptr) {}
 };
-// Serializes an index file.
+/// Serializes an index file.
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const IndexFileOut &O);
 
 std::string toYAML(const Symbol &);
-// Returned symbol is backed by the YAML input.
+/// Returned symbol is backed by the YAML input.
 // FIXME: this is only needed for IndexerMain, find a better solution.
 llvm::Expected symbolFromYAML(llvm::yaml::Input &);
 
-// Build an in-memory static index from an index file.
-// The size should be relatively small, so data can be managed in memory.
+/// Build an in-memory static index from an index file.
+/// The size should be relatively small, so data can be managed in memory.
 std::unique_ptr loadIndex(llvm::StringRef Filename,
llvm::ArrayRef URISchemes,
bool UseDex = true);


Index: clang-tools-extra/clangd/index/Serialization.h
===
--- clang-tools-extra/clangd/index/Serialization.h
+++ clang-tools-extra/clangd/index/Serialization.h
@@ -40,14 +40,15 @@
   YAML, // Human-readable format, suitable for experiments and debugging.
 };
 
-// Holds the contents of an index file that was read.
+/// Holds the contents of an index file that was read.
 struct IndexFileIn {
   llvm::Optional Symbols;
 };
-// Parse an index file. The input must be a RIFF container chunk.
+/// Parse an index file. The input must be a RIFF container chunk or YAML symbol
+/// collection.
 llvm::Expected readIndexFile(llvm::StringRef);
 
-// Specifies the contents of an index file to be written.
+/// Specifies the contents of an index file to be written.
 struct IndexFileOut {
   const SymbolSlab *Symbols;
   // TODO: Support serializing symbol occurrences.
@@ -58,16 +59,16 @@
   IndexFileOut(const IndexFileIn &I)
   : Symbols(I.Symbols ? I.Symbols.getPointer() : nullptr) {}
 };
-// Serializes an index file.
+/// Serializes an index file.
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const IndexFileOut &O);
 
 std::string toYAML(const Symbol &);
-// Returned symbol is backed by the YAML input.
+/// Returned symbol is backed by the YAML input.
 // FIXME: this is only needed for IndexerMain, find a better solution.
 llvm::Expected symbolFromYAML(llvm::yaml::Input &);
 
-// Build an in-memory static index from an index file.
-// The size should be relatively small, so data can be managed in memory.
+/// Build an in-memory static index from an index file.
+/// The size should be relatively small, so data can be managed in memory.
 std::unique_ptr loadIndex(llvm::StringRef Filename,
llvm::ArrayRef URISchemes,
bool UseDex = true);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52225: [clang] Implement Override Suggestions in Sema.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 167071.
kadircet added a comment.

- Change order of fucntions for better diff.
- Add tests.


Repository:
  rC Clang

https://reviews.llvm.org/D52225

Files:
  include/clang/Sema/CodeCompleteConsumer.h
  lib/Sema/SemaCodeComplete.cpp
  test/CodeCompletion/overrides.cpp

Index: test/CodeCompletion/overrides.cpp
===
--- /dev/null
+++ test/CodeCompletion/overrides.cpp
@@ -0,0 +1,30 @@
+class A {
+ public:
+  virtual void vfunc(bool param);
+  virtual void vfunc(bool param, int p);
+  void func(bool param);
+};
+class B : public A {
+virtual int ttt(bool param) const;
+void vfunc(bool param, int p) override;
+};
+class C : public B {
+ public:
+  void vfunc(bool param) override;
+  void
+};
+
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:3 %s -o - | FileCheck -check-prefix=CHECK-CC1 %s
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:5 %s -o - | FileCheck -check-prefix=CHECK-CC2 %s
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:14:8 %s -o - | FileCheck -check-prefix=CHECK-CC3 %s
+// CHECK-CC1: COMPLETION: Pattern : int ttt(bool param) const override
+// CHECK-CC2-NOT: COMPLETION: Pattern : int ttt(bool param) const override
+// CHECK-CC3-NOT: COMPLETION: Pattern : int ttt(bool param) const override
+//
+// CHECK-CC1: COMPLETION: Pattern : void vfunc(bool param, int p) override
+// CHECK-CC2: COMPLETION: Pattern : void vfunc(bool param, int p) override
+// CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param, int p) override
+//
+// CHECK-CC1-NOT: COMPLETION: Pattern : void vfunc(bool param) override
+// CHECK-CC2-NOT: COMPLETION: Pattern : void vfunc(bool param) override
+// CHECK-CC3-NOT: COMPLETION: Pattern : void vfunc(bool param) override
Index: lib/Sema/SemaCodeComplete.cpp
===
--- lib/Sema/SemaCodeComplete.cpp
+++ lib/Sema/SemaCodeComplete.cpp
@@ -1598,6 +1598,66 @@
   Results.AddResult(CodeCompletionResult(Builder.TakeString()));
 }
 
+static void AddOverrideResults(ResultBuilder &Results,
+   const CodeCompletionContext &CCContext,
+   CodeCompletionBuilder &Builder, Sema &S) {
+  const auto *CR = llvm::dyn_cast(S.CurContext);
+  // If not inside a class/struct/union return empty.
+  if (!CR)
+return;
+  // First store overrides within current class.
+  // These are stored by name to make querying fast in the later step.
+  llvm::StringMap> Overrides;
+  for (auto *Method : CR->methods()) {
+if (!Method->isVirtual() || !Method->getIdentifier())
+  continue;
+Overrides[Method->getName()].push_back(Method);
+  }
+
+  for (const auto &Base : CR->bases()) {
+const auto *BR = Base.getType().getTypePtr()->getAsCXXRecordDecl();
+if (!BR)
+  continue;
+for (auto *Method : BR->methods()) {
+  if (!Method->isVirtual() || !Method->getIdentifier())
+continue;
+  const auto it = Overrides.find(Method->getName());
+  bool IsOverriden = false;
+  if (it != Overrides.end()) {
+for (auto *MD : it->second) {
+  // If the method in current body is not an overload of this virtual
+  // function, then it overrides this one.
+  if (!S.IsOverload(MD, Method, false)) {
+IsOverriden = true;
+break;
+  }
+}
+  }
+  if (!IsOverriden) {
+std::string OverrideSignature;
+llvm::raw_string_ostream OS(OverrideSignature);
+CodeCompletionResult CCR(Method, 0);
+PrintingPolicy Policy =
+getCompletionPrintingPolicy(S.getASTContext(), S.getPreprocessor());
+CCR.createCodeCompletionStringForDecl(
+S.getPreprocessor(), S.getASTContext(), Builder,
+false, CCContext, Policy);
+for(const auto& C : *Builder.TakeString()) {
+  if (C.Kind == CodeCompletionString::CK_Optional)
+continue;
+  OS << C.Text;
+  if (C.Kind == CodeCompletionString::CK_ResultType)
+OS << ' ';
+}
+OS << " override";
+Builder.AddTypedTextChunk(Builder.getAllocator().CopyString(OS.str()));
+Results.AddResult(CodeCompletionResult(Builder.TakeString(), Method,
+   CCP_CodePattern));
+  }
+}
+  }
+}
+
 /// Add language constructs that show up for "ordinary" names.
 static void AddOrdinaryNameResults(Sema::ParserCompletionContext CCC,
Scope *S,
@@ -1706,6 +1766,9 @@
 if (IsNotInheritanceScope && Results.includeCodePatterns())
   Builder.AddChunk(CodeCompletionString::CK_Colon);
 Results.AddResult(Result(Builder.TakeString()));
+
+AddOverrideResults(Results, CodeCompletionContext::CCC_ClassStructUnion,
+   Builder, SemaRef);
   }
 }
 LLVM_FALLTHROUGH;
@@ -2834,7 +2897,16 @

[PATCH] D52535: clang-format: [JS] space after parameter naming.

[Martin Probst via Phabricator via cfe-commits]

mprobst created this revision.
mprobst added a reviewer: mboehme.

Previously:

  foo(/*bar=*/baz);

Now:

  foo(/*bar=*/ baz);

The run-in parameter naming comment is not intended in JS.


Repository:
  rC Clang

https://reviews.llvm.org/D52535

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestJS.cpp


Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -2304,5 +2304,9 @@
  "};"));
 }
 
+TEST_F(FormatTestJS, ParameterNamingComment) {
+  verifyFormat("callFoo(/*spaceAfterParameterNamingComment=*/ 1);");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2517,7 +2517,9 @@
Right.MatchingParen->BlockKind != BK_Block))
 return !Style.Cpp11BracedListStyle;
   if (Left.is(TT_BlockComment))
-return !Left.TokenText.endswith("=*/");
+// No whitespace in x(/*foo=*/1), except for JavaScript.
+return Style.Language == FormatStyle::LK_JavaScript ||
+   !Left.TokenText.endswith("=*/");
   if (Right.is(tok::l_paren)) {
 if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
 (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))


Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -2304,5 +2304,9 @@
  "};"));
 }
 
+TEST_F(FormatTestJS, ParameterNamingComment) {
+  verifyFormat("callFoo(/*spaceAfterParameterNamingComment=*/ 1);");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2517,7 +2517,9 @@
Right.MatchingParen->BlockKind != BK_Block))
 return !Style.Cpp11BracedListStyle;
   if (Left.is(TT_BlockComment))
-return !Left.TokenText.endswith("=*/");
+// No whitespace in x(/*foo=*/1), except for JavaScript.
+return Style.Language == FormatStyle::LK_JavaScript ||
+   !Left.TokenText.endswith("=*/");
   if (Right.is(tok::l_paren)) {
 if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
 (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52534: [clangd] NFC: Update Serialization routine documentation

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

The wording change is in https://reviews.llvm.org/D52531.


https://reviews.llvm.org/D52534



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

[PATCH] D52226: [clangd] Remove override result handling logic from clangd

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 167075.
kadircet added a comment.

- Remove test, since it has been moved into Sema.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52226

Files:
  clangd/CodeComplete.cpp

Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -198,55 +198,6 @@
   return HeaderFile{std::move(*Resolved), /*Verbatim=*/false};
 }
 
-// First traverses all method definitions inside current class/struct/union
-// definition. Than traverses base classes to find virtual methods that haven't
-// been overriden within current context.
-// FIXME(kadircet): Currently we cannot see declarations below completion point.
-// It is because Sema gets run only upto completion point. Need to find a
-// solution to run it for the whole class/struct/union definition.
-static std::vector
-getNonOverridenMethodCompletionResults(const DeclContext *DC, Sema *S) {
-  const auto *CR = llvm::dyn_cast(DC);
-  // If not inside a class/struct/union return empty.
-  if (!CR)
-return {};
-  // First store overrides within current class.
-  // These are stored by name to make querying fast in the later step.
-  llvm::StringMap> Overrides;
-  for (auto *Method : CR->methods()) {
-if (!Method->isVirtual() || !Method->getIdentifier())
-  continue;
-Overrides[Method->getName()].push_back(Method);
-  }
-
-  std::vector Results;
-  for (const auto &Base : CR->bases()) {
-const auto *BR = Base.getType().getTypePtr()->getAsCXXRecordDecl();
-if (!BR)
-  continue;
-for (auto *Method : BR->methods()) {
-  if (!Method->isVirtual() || !Method->getIdentifier())
-continue;
-  const auto it = Overrides.find(Method->getName());
-  bool IsOverriden = false;
-  if (it != Overrides.end()) {
-for (auto *MD : it->second) {
-  // If the method in current body is not an overload of this virtual
-  // function, then it overrides this one.
-  if (!S->IsOverload(MD, Method, false)) {
-IsOverriden = true;
-break;
-  }
-}
-  }
-  if (!IsOverriden)
-Results.emplace_back(Method, 0);
-}
-  }
-
-  return Results;
-}
-
 /// A code completion result, in clang-native form.
 /// It may be promoted to a CompletionItem if it's among the top-ranked results.
 struct CompletionCandidate {
@@ -256,9 +207,6 @@
   const Symbol *IndexResult = nullptr;
   llvm::SmallVector RankedIncludeHeaders;
 
-  // States whether this item is an override suggestion.
-  bool IsOverride = false;
-
   // 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 {
@@ -428,30 +376,21 @@
 Completion.Documentation = getDocComment(ASTCtx, *C.SemaResult,
  /*CommentsFromHeader=*/false);
 }
-if (C.IsOverride)
-  S.OverrideSuffix = true;
   }
 
   CodeCompletion build() {
 Completion.ReturnType = summarizeReturnType();
 Completion.Signature = summarizeSignature();
 Completion.SnippetSuffix = summarizeSnippet();
 Completion.BundleSize = Bundled.size();
-if (summarizeOverride()) {
-  Completion.Name = Completion.ReturnType + ' ' +
-std::move(Completion.Name) +
-std::move(Completion.Signature) + " override";
-  Completion.Signature.clear();
-}
 return std::move(Completion);
   }
 
 private:
   struct BundledEntry {
 std::string SnippetSuffix;
 std::string Signature;
 std::string ReturnType;
-bool OverrideSuffix;
   };
 
   // If all BundledEntrys have the same value for a property, return it.
@@ -499,12 +438,6 @@
 return "(…)";
   }
 
-  bool summarizeOverride() const {
-if (auto *OverrideSuffix = onlyValue<&BundledEntry::OverrideSuffix>())
-  return *OverrideSuffix;
-return false;
-  }
-
   ASTContext &ASTCtx;
   CodeCompletion Completion;
   SmallVector Bundled;
@@ -1363,11 +1296,8 @@
 ? queryIndex()
 : SymbolSlab();
 trace::Span Tracer("Populate CodeCompleteResult");
-// Merge Sema, Index and Override results, score them, and pick the
-// winners.
-const auto Overrides = getNonOverridenMethodCompletionResults(
-Recorder->CCSema->CurContext, Recorder->CCSema);
-auto Top = mergeResults(Recorder->Results, IndexResults, Overrides);
+// Merge Sema and Index results, score them, and pick the winners.
+auto Top = mergeResults(Recorder->Results, IndexResults);
 CodeCompleteResult Output;
 
 // Convert the results to final form, assembling the expensive strings.
@@ -1418,26 +1348,22 @@
 return std::move(ResultsBuilder).build();
   }
 
-  // Merges Sema, Index and Override results where possible, to form
-  // CompletionCandidates. Groups overloads if desired, to 

[PATCH] D52397: [libc++] Remove Fuchsia-specific knowledge to pick the ABI version

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL343079: [libc++] Remove Fuchsia-specific knowledge to pick 
the ABI version (authored by ldionne, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52397?vs=166621&id=167076#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52397

Files:
  libcxx/trunk/CMakeLists.txt
  libcxx/trunk/include/__config


Index: libcxx/trunk/include/__config
===
--- libcxx/trunk/include/__config
+++ libcxx/trunk/include/__config
@@ -36,11 +36,7 @@
 #define _LIBCPP_VERSION 8000
 
 #ifndef _LIBCPP_ABI_VERSION
-#  ifdef __Fuchsia__
-#define _LIBCPP_ABI_VERSION 2
-#  else
-#define _LIBCPP_ABI_VERSION 1
-#  endif
+#  define _LIBCPP_ABI_VERSION 1
 #endif
 
 #ifndef _LIBCPP_STD_VER
Index: libcxx/trunk/CMakeLists.txt
===
--- libcxx/trunk/CMakeLists.txt
+++ libcxx/trunk/CMakeLists.txt
@@ -111,12 +111,7 @@
 "Install libc++fs.a" ON
 "LIBCXX_ENABLE_FILESYSTEM;LIBCXX_INSTALL_LIBRARY" OFF)
 
-if (FUCHSIA)
-  set(DEFAULT_ABI_VERSION 2)
-else()
-  set(DEFAULT_ABI_VERSION 1)
-endif()
-set(LIBCXX_ABI_VERSION ${DEFAULT_ABI_VERSION} CACHE STRING "ABI version of 
libc++.")
+set(LIBCXX_ABI_VERSION "1" CACHE STRING "ABI version of libc++. Can be either 
1 or 2, where 2 is currently not stable. Defaults to 1.")
 option(LIBCXX_ABI_UNSTABLE "Unstable ABI of libc++." OFF)
 option(LIBCXX_ABI_FORCE_ITANIUM "Ignore auto-detection and force use of the 
Itanium ABI.")
 option(LIBCXX_ABI_FORCE_MICROSOFT "Ignore auto-detection and force use of the 
Microsoft ABI.")
@@ -657,7 +652,7 @@
 endif()
 
 # Configuration file flags 
=
-if (NOT LIBCXX_ABI_VERSION EQUAL DEFAULT_ABI_VERSION)
+if (NOT LIBCXX_ABI_VERSION EQUAL 1)
   config_define(${LIBCXX_ABI_VERSION} _LIBCPP_ABI_VERSION)
 endif()
 config_define_if(LIBCXX_ABI_UNSTABLE _LIBCPP_ABI_UNSTABLE)


Index: libcxx/trunk/include/__config
===
--- libcxx/trunk/include/__config
+++ libcxx/trunk/include/__config
@@ -36,11 +36,7 @@
 #define _LIBCPP_VERSION 8000
 
 #ifndef _LIBCPP_ABI_VERSION
-#  ifdef __Fuchsia__
-#define _LIBCPP_ABI_VERSION 2
-#  else
-#define _LIBCPP_ABI_VERSION 1
-#  endif
+#  define _LIBCPP_ABI_VERSION 1
 #endif
 
 #ifndef _LIBCPP_STD_VER
Index: libcxx/trunk/CMakeLists.txt
===
--- libcxx/trunk/CMakeLists.txt
+++ libcxx/trunk/CMakeLists.txt
@@ -111,12 +111,7 @@
 "Install libc++fs.a" ON
 "LIBCXX_ENABLE_FILESYSTEM;LIBCXX_INSTALL_LIBRARY" OFF)
 
-if (FUCHSIA)
-  set(DEFAULT_ABI_VERSION 2)
-else()
-  set(DEFAULT_ABI_VERSION 1)
-endif()
-set(LIBCXX_ABI_VERSION ${DEFAULT_ABI_VERSION} CACHE STRING "ABI version of libc++.")
+set(LIBCXX_ABI_VERSION "1" CACHE STRING "ABI version of libc++. Can be either 1 or 2, where 2 is currently not stable. Defaults to 1.")
 option(LIBCXX_ABI_UNSTABLE "Unstable ABI of libc++." OFF)
 option(LIBCXX_ABI_FORCE_ITANIUM "Ignore auto-detection and force use of the Itanium ABI.")
 option(LIBCXX_ABI_FORCE_MICROSOFT "Ignore auto-detection and force use of the Microsoft ABI.")
@@ -657,7 +652,7 @@
 endif()
 
 # Configuration file flags =
-if (NOT LIBCXX_ABI_VERSION EQUAL DEFAULT_ABI_VERSION)
+if (NOT LIBCXX_ABI_VERSION EQUAL 1)
   config_define(${LIBCXX_ABI_VERSION} _LIBCPP_ABI_VERSION)
 endif()
 config_define_if(LIBCXX_ABI_UNSTABLE _LIBCPP_ABI_UNSTABLE)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r343080 - clang-format: [JS] space after parameter naming.

[Martin Probst via cfe-commits]

Author: mprobst
Date: Wed Sep 26 01:28:33 2018
New Revision: 343080

URL: http://llvm.org/viewvc/llvm-project?rev=343080&view=rev
Log:
clang-format: [JS] space after parameter naming.

Summary:
Previously:
foo(/*bar=*/baz);

Now:
foo(/*bar=*/ baz);

The run-in parameter naming comment is not intended in JS.

Reviewers: mboehme

Subscribers: cfe-commits

Differential Revision: https://reviews.llvm.org/D52535

Modified:
cfe/trunk/lib/Format/TokenAnnotator.cpp
cfe/trunk/unittests/Format/FormatTestJS.cpp

Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=343080&r1=343079&r2=343080&view=diff
==
--- cfe/trunk/lib/Format/TokenAnnotator.cpp (original)
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp Wed Sep 26 01:28:33 2018
@@ -2517,7 +2517,9 @@ bool TokenAnnotator::spaceRequiredBetwee
Right.MatchingParen->BlockKind != BK_Block))
 return !Style.Cpp11BracedListStyle;
   if (Left.is(TT_BlockComment))
-return !Left.TokenText.endswith("=*/");
+// No whitespace in x(/*foo=*/1), except for JavaScript.
+return Style.Language == FormatStyle::LK_JavaScript ||
+   !Left.TokenText.endswith("=*/");
   if (Right.is(tok::l_paren)) {
 if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
 (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))

Modified: cfe/trunk/unittests/Format/FormatTestJS.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTestJS.cpp?rev=343080&r1=343079&r2=343080&view=diff
==
--- cfe/trunk/unittests/Format/FormatTestJS.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTestJS.cpp Wed Sep 26 01:28:33 2018
@@ -2304,5 +2304,9 @@ TEST_F(FormatTestJS, AddsLastLinePenalty
  "};"));
 }
 
+TEST_F(FormatTestJS, ParameterNamingComment) {
+  verifyFormat("callFoo(/*spaceAfterParameterNamingComment=*/ 1);");
+}
+
 } // end namespace tooling
 } // end namespace clang


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

[PATCH] D52535: clang-format: [JS] space after parameter naming.

[Martin Probst via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL343080: clang-format: [JS] space after parameter naming. 
(authored by mprobst, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D52535

Files:
  cfe/trunk/lib/Format/TokenAnnotator.cpp
  cfe/trunk/unittests/Format/FormatTestJS.cpp


Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -2517,7 +2517,9 @@
Right.MatchingParen->BlockKind != BK_Block))
 return !Style.Cpp11BracedListStyle;
   if (Left.is(TT_BlockComment))
-return !Left.TokenText.endswith("=*/");
+// No whitespace in x(/*foo=*/1), except for JavaScript.
+return Style.Language == FormatStyle::LK_JavaScript ||
+   !Left.TokenText.endswith("=*/");
   if (Right.is(tok::l_paren)) {
 if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
 (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))
Index: cfe/trunk/unittests/Format/FormatTestJS.cpp
===
--- cfe/trunk/unittests/Format/FormatTestJS.cpp
+++ cfe/trunk/unittests/Format/FormatTestJS.cpp
@@ -2304,5 +2304,9 @@
  "};"));
 }
 
+TEST_F(FormatTestJS, ParameterNamingComment) {
+  verifyFormat("callFoo(/*spaceAfterParameterNamingComment=*/ 1);");
+}
+
 } // end namespace tooling
 } // end namespace clang


Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -2517,7 +2517,9 @@
Right.MatchingParen->BlockKind != BK_Block))
 return !Style.Cpp11BracedListStyle;
   if (Left.is(TT_BlockComment))
-return !Left.TokenText.endswith("=*/");
+// No whitespace in x(/*foo=*/1), except for JavaScript.
+return Style.Language == FormatStyle::LK_JavaScript ||
+   !Left.TokenText.endswith("=*/");
   if (Right.is(tok::l_paren)) {
 if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
 (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))
Index: cfe/trunk/unittests/Format/FormatTestJS.cpp
===
--- cfe/trunk/unittests/Format/FormatTestJS.cpp
+++ cfe/trunk/unittests/Format/FormatTestJS.cpp
@@ -2304,5 +2304,9 @@
  "};"));
 }
 
+TEST_F(FormatTestJS, ParameterNamingComment) {
+  verifyFormat("callFoo(/*spaceAfterParameterNamingComment=*/ 1);");
+}
+
 } // end namespace tooling
 } // end namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52535: clang-format: [JS] space after parameter naming.

[Martin Probst via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC343080: clang-format: [JS] space after parameter naming. 
(authored by mprobst, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52535?vs=167073&id=167077#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52535

Files:
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestJS.cpp


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2517,7 +2517,9 @@
Right.MatchingParen->BlockKind != BK_Block))
 return !Style.Cpp11BracedListStyle;
   if (Left.is(TT_BlockComment))
-return !Left.TokenText.endswith("=*/");
+// No whitespace in x(/*foo=*/1), except for JavaScript.
+return Style.Language == FormatStyle::LK_JavaScript ||
+   !Left.TokenText.endswith("=*/");
   if (Right.is(tok::l_paren)) {
 if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
 (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))
Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -2304,5 +2304,9 @@
  "};"));
 }
 
+TEST_F(FormatTestJS, ParameterNamingComment) {
+  verifyFormat("callFoo(/*spaceAfterParameterNamingComment=*/ 1);");
+}
+
 } // end namespace tooling
 } // end namespace clang


Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2517,7 +2517,9 @@
Right.MatchingParen->BlockKind != BK_Block))
 return !Style.Cpp11BracedListStyle;
   if (Left.is(TT_BlockComment))
-return !Left.TokenText.endswith("=*/");
+// No whitespace in x(/*foo=*/1), except for JavaScript.
+return Style.Language == FormatStyle::LK_JavaScript ||
+   !Left.TokenText.endswith("=*/");
   if (Right.is(tok::l_paren)) {
 if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
 (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))
Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -2304,5 +2304,9 @@
  "};"));
 }
 
+TEST_F(FormatTestJS, ParameterNamingComment) {
+  verifyFormat("callFoo(/*spaceAfterParameterNamingComment=*/ 1);");
+}
+
 } // end namespace tooling
 } // end namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52503: [clangd] Fix bugs with incorrect memory estimate report

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 167079.
kbobyrev retitled this revision from "[clangd] More precise index memory usage 
estimate" to "[clangd] Fix bugs with incorrect memory estimate report".
kbobyrev edited the summary of this revision.
kbobyrev added a comment.

As discussed offline, using `std::string::size()` would not be a precise 
estimate of `std::string` size, therefore I'm excluding `Token::Data` size 
estimate.


https://reviews.llvm.org/D52503

Files:
  clang-tools-extra/clangd/index/Serialization.cpp
  clang-tools-extra/clangd/index/dex/Dex.cpp
  clang-tools-extra/clangd/index/dex/PostingList.h


Index: clang-tools-extra/clangd/index/dex/PostingList.h
===
--- clang-tools-extra/clangd/index/dex/PostingList.h
+++ clang-tools-extra/clangd/index/dex/PostingList.h
@@ -66,10 +66,8 @@
   /// go through the chunks and decompress them on-the-fly when necessary.
   std::unique_ptr iterator() const;
 
-  /// Returns in-memory size.
-  size_t bytes() const {
-return sizeof(Chunk) + Chunks.capacity() * sizeof(Chunk);
-  }
+  /// Returns in-memory size of external storage.
+  size_t bytes() const { return Chunks.capacity() * sizeof(Chunk); }
 
 private:
   const std::vector Chunks;
Index: clang-tools-extra/clangd/index/dex/Dex.cpp
===
--- clang-tools-extra/clangd/index/dex/Dex.cpp
+++ clang-tools-extra/clangd/index/dex/Dex.cpp
@@ -130,9 +130,6 @@
   for (const auto &TokenToPostingList : TempInvertedIndex)
 InvertedIndex.insert(
 {TokenToPostingList.first, PostingList(TokenToPostingList.second)});
-
-  vlog("Built Dex with estimated memory usage {0} bytes.",
-   estimateMemoryUsage());
 }
 
 /// Constructs iterators over tokens extracted from the query and exhausts it
@@ -248,8 +245,8 @@
   Bytes += SymbolQuality.size() * sizeof(float);
   Bytes += LookupTable.getMemorySize();
   Bytes += InvertedIndex.getMemorySize();
-  for (const auto &P : InvertedIndex)
-Bytes += P.second.bytes();
+  for (const auto &TokenToPostingList : InvertedIndex)
+Bytes += TokenToPostingList.second.bytes();
   return Bytes + BackingDataSize;
 }
 
Index: clang-tools-extra/clangd/index/Serialization.cpp
===
--- clang-tools-extra/clangd/index/Serialization.cpp
+++ clang-tools-extra/clangd/index/Serialization.cpp
@@ -6,8 +6,10 @@
 // License. See LICENSE.TXT for details.
 //
 
//===--===//
+
 #include "Serialization.h"
 #include "Index.h"
+#include "Logger.h"
 #include "RIFF.h"
 #include "Trace.h"
 #include "dex/Dex.h"
@@ -433,8 +435,12 @@
   }
 
   trace::Span Tracer("BuildIndex");
-  return UseDex ? dex::Dex::build(std::move(Symbols), URISchemes)
-: MemIndex::build(std::move(Symbols), std::move(Refs));
+  auto Index = UseDex ? dex::Dex::build(std::move(Symbols), URISchemes)
+  : MemIndex::build(std::move(Symbols), std::move(Refs));
+  vlog("Loaded {0} from {1} with estimated memory usage {2}",
+   UseDex ? "Dex" : "MemIndex", SymbolFilename,
+   Index->estimateMemoryUsage());
+  return Index;
 }
 
 } // namespace clangd


Index: clang-tools-extra/clangd/index/dex/PostingList.h
===
--- clang-tools-extra/clangd/index/dex/PostingList.h
+++ clang-tools-extra/clangd/index/dex/PostingList.h
@@ -66,10 +66,8 @@
   /// go through the chunks and decompress them on-the-fly when necessary.
   std::unique_ptr iterator() const;
 
-  /// Returns in-memory size.
-  size_t bytes() const {
-return sizeof(Chunk) + Chunks.capacity() * sizeof(Chunk);
-  }
+  /// Returns in-memory size of external storage.
+  size_t bytes() const { return Chunks.capacity() * sizeof(Chunk); }
 
 private:
   const std::vector Chunks;
Index: clang-tools-extra/clangd/index/dex/Dex.cpp
===
--- clang-tools-extra/clangd/index/dex/Dex.cpp
+++ clang-tools-extra/clangd/index/dex/Dex.cpp
@@ -130,9 +130,6 @@
   for (const auto &TokenToPostingList : TempInvertedIndex)
 InvertedIndex.insert(
 {TokenToPostingList.first, PostingList(TokenToPostingList.second)});
-
-  vlog("Built Dex with estimated memory usage {0} bytes.",
-   estimateMemoryUsage());
 }
 
 /// Constructs iterators over tokens extracted from the query and exhausts it
@@ -248,8 +245,8 @@
   Bytes += SymbolQuality.size() * sizeof(float);
   Bytes += LookupTable.getMemorySize();
   Bytes += InvertedIndex.getMemorySize();
-  for (const auto &P : InvertedIndex)
-Bytes += P.second.bytes();
+  for (const auto &TokenToPostingList : InvertedIndex)
+Bytes += TokenToPostingList.second.bytes();
   return Bytes + BackingDataSize;
 }
 
Index: clang-tools-extra/clangd/index/Serialization.cpp
=

[PATCH] D52517: [clangd] clangd-indexer: Drop support for MR-via-YAML

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

LGTM


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52517



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

[PATCH] D52536: clang-format: [JS] conditional types.

[Martin Probst via Phabricator via cfe-commits]

mprobst created this revision.
mprobst added a reviewer: krasimir.

This change adds some rudimentary support for conditional types.
Specifically it avoids breaking before `extends` and `infer` keywords,
which are subject to Automatic Semicolon Insertion, so breaking before
them creates incorrect syntax.

The actual formatting of the type expression is odd, but there is as of
yet no clear idea on how to format these.

See 
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-8.html#conditional-types.


Repository:
  rC Clang

https://reviews.llvm.org/D52536

Files:
  lib/Format/FormatToken.h
  lib/Format/TokenAnnotator.cpp
  unittests/Format/FormatTestJS.cpp


Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -2308,5 +2308,12 @@
   verifyFormat("callFoo(/*spaceAfterParameterNamingComment=*/ 1);");
 }
 
+TEST_F(FormatTestJS, ConditionalTypes) {
+  verifyFormat("type UnionToIntersection =\n"
+   "(U extends any ? (k: U) => void :\n"
+   " never) extends((k: infer I) => void) ? I "
+   ": never;");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -3088,6 +3088,12 @@
   return Style.BreakBeforeBinaryOperators != FormatStyle::BOS_None;
 if (Right.is(Keywords.kw_as))
   return false; // must not break before as in 'x as type' casts
+if (Right.isOneOf(Keywords.kw_extends, Keywords.kw_infer)) {
+  // extends and infer can appear as keywords in conditional types:
+  //   
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-8.html#conditional-types
+  // do not break before them, as the expressions are subject to ASI.
+  return false;
+}
 if (Left.is(Keywords.kw_as))
   return true;
 if (Left.is(TT_JsNonNullAssertion))
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -680,6 +680,7 @@
 kw_function = &IdentTable.get("function");
 kw_get = &IdentTable.get("get");
 kw_import = &IdentTable.get("import");
+kw_infer = &IdentTable.get("infer");
 kw_is = &IdentTable.get("is");
 kw_let = &IdentTable.get("let");
 kw_module = &IdentTable.get("module");
@@ -751,6 +752,7 @@
   IdentifierInfo *kw_function;
   IdentifierInfo *kw_get;
   IdentifierInfo *kw_import;
+  IdentifierInfo *kw_infer;
   IdentifierInfo *kw_is;
   IdentifierInfo *kw_let;
   IdentifierInfo *kw_module;


Index: unittests/Format/FormatTestJS.cpp
===
--- unittests/Format/FormatTestJS.cpp
+++ unittests/Format/FormatTestJS.cpp
@@ -2308,5 +2308,12 @@
   verifyFormat("callFoo(/*spaceAfterParameterNamingComment=*/ 1);");
 }
 
+TEST_F(FormatTestJS, ConditionalTypes) {
+  verifyFormat("type UnionToIntersection =\n"
+   "(U extends any ? (k: U) => void :\n"
+   " never) extends((k: infer I) => void) ? I "
+   ": never;");
+}
+
 } // end namespace tooling
 } // end namespace clang
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -3088,6 +3088,12 @@
   return Style.BreakBeforeBinaryOperators != FormatStyle::BOS_None;
 if (Right.is(Keywords.kw_as))
   return false; // must not break before as in 'x as type' casts
+if (Right.isOneOf(Keywords.kw_extends, Keywords.kw_infer)) {
+  // extends and infer can appear as keywords in conditional types:
+  //   https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-8.html#conditional-types
+  // do not break before them, as the expressions are subject to ASI.
+  return false;
+}
 if (Left.is(Keywords.kw_as))
   return true;
 if (Left.is(TT_JsNonNullAssertion))
Index: lib/Format/FormatToken.h
===
--- lib/Format/FormatToken.h
+++ lib/Format/FormatToken.h
@@ -680,6 +680,7 @@
 kw_function = &IdentTable.get("function");
 kw_get = &IdentTable.get("get");
 kw_import = &IdentTable.get("import");
+kw_infer = &IdentTable.get("infer");
 kw_is = &IdentTable.get("is");
 kw_let = &IdentTable.get("let");
 kw_module = &IdentTable.get("module");
@@ -751,6 +752,7 @@
   IdentifierInfo *kw_function;
   IdentifierInfo *kw_get;
   IdentifierInfo *kw_import;
+  IdentifierInfo *kw_infer;
   IdentifierInfo *kw_is;
   IdentifierInfo *kw_let;
   IdentifierInfo *kw_module;
___
cfe-commits mailing list
cfe-commits@lists.l

[clang-tools-extra] r343083 - Removed extra semicolon to fix Wpedantic. (NFCI).

[Simon Pilgrim via cfe-commits]

Author: rksimon
Date: Wed Sep 26 02:02:45 2018
New Revision: 343083

URL: http://llvm.org/viewvc/llvm-project?rev=343083&view=rev
Log:
Removed extra semicolon to fix Wpedantic. (NFCI).

Modified:
clang-tools-extra/trunk/clangd/index/IndexAction.cpp

Modified: clang-tools-extra/trunk/clangd/index/IndexAction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/IndexAction.cpp?rev=343083&r1=343082&r2=343083&view=diff
==
--- clang-tools-extra/trunk/clangd/index/IndexAction.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/IndexAction.cpp Wed Sep 26 02:02:45 
2018
@@ -67,7 +67,7 @@ createStaticIndexingAction(SymbolCollect
   return llvm::make_unique(
   std::make_shared(std::move(Opts)), std::move(Includes),
   IndexOpts, SymbolsCallback);
-};
+}
 
 } // namespace clangd
 } // namespace clang


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

[PATCH] D52290: [driver][mips] Adjust target triple accordingly to provided ABI name

[Simon Atanasyan via Phabricator via cfe-commits]

atanasyan added a comment.

In https://reviews.llvm.org/D52290#1245222, @rnk wrote:

> Should `--target=mipsZZZ -mabi=n64` be meaningful? What does `clang 
> --target=i686-linux-gnu -m64` do? If this matches that, then let's do it.


Thanks. Initially I thought that providing different target triple and `-mabi` 
option explicitly is an error. But now I see that it's better and consistent 
with other targets to adjust a target triple accordingly to `-mabi` options in 
any (implicit/explicit) case. I will update the patch soon.


Repository:
  rC Clang

https://reviews.llvm.org/D52290



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

r343085 - Removed extra semicolon to fix Wpedantic. (NFCI).

[Simon Pilgrim via cfe-commits]

Author: rksimon
Date: Wed Sep 26 02:12:55 2018
New Revision: 343085

URL: http://llvm.org/viewvc/llvm-project?rev=343085&view=rev
Log:
Removed extra semicolon to fix Wpedantic. (NFCI).

Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp?rev=343085&r1=343084&r2=343085&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp Wed Sep 26 
02:12:55 2018
@@ -26,7 +26,7 @@ using namespace clang;
 using namespace ento;
 
 // Associate container objects with a set of raw pointer symbols.
-REGISTER_SET_FACTORY_WITH_PROGRAMSTATE(PtrSet, SymbolRef);
+REGISTER_SET_FACTORY_WITH_PROGRAMSTATE(PtrSet, SymbolRef)
 REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, PtrSet)
 
 


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

[PATCH] D52538: [MinGW] Allow using ASan

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added a reviewer: rnk.

Linking to ASan for MinGW is similar to MSVC, but MinGW always links the CRT 
dynamically, so there is only one of the MSVC cases to consider.

When linking to a shared compiler runtime library on MinGW, the suffix of the 
import library is .dll.a.

The existing case of .dll as suffix for windows in general doesn't seem correct 
(since this is used for linking). As long as callers never actually set the 
Shared flag, the default static suffix of .lib also worked fine for import 
libraries as well.


Repository:
  rC Clang

https://reviews.llvm.org/D52538

Files:
  lib/Driver/ToolChain.cpp
  lib/Driver/ToolChains/MinGW.cpp
  lib/Driver/ToolChains/MinGW.h
  test/Driver/mingw-sanitizers.c

Index: test/Driver/mingw-sanitizers.c
===
--- /dev/null
+++ test/Driver/mingw-sanitizers.c
@@ -0,0 +1,11 @@
+// RUN: %clang -target i686-windows-gnu %s -### -fsanitize=address 2>&1 | FileCheck --check-prefix=ASAN-I686 %s
+// ASAN-I686: "{{.*}}libclang_rt.asan_dynamic-i386.dll.a"
+// ASAN-I686: "{{[^"]*}}libclang_rt.asan_dynamic_runtime_thunk-i386.a"
+// ASAN-I686: "--require-defined" "___asan_seh_interceptor"
+// ASAN-I686: "--whole-archive" "{{.*}}libclang_rt.asan_dynamic_runtime_thunk-i386.a" "--no-whole-archive"
+
+// RUN: %clang -target x86_64-windows-gnu %s -### -fsanitize=address 2>&1 | FileCheck --check-prefix=ASAN-X86_64 %s
+// ASAN-X86_64: "{{.*}}libclang_rt.asan_dynamic-x86_64.dll.a"
+// ASAN-X86_64: "{{[^"]*}}libclang_rt.asan_dynamic_runtime_thunk-x86_64.a"
+// ASAN-X86_64: "--require-defined" "__asan_seh_interceptor"
+// ASAN-X86_64: "--whole-archive" "{{.*}}libclang_rt.asan_dynamic_runtime_thunk-x86_64.a" "--no-whole-archive"
Index: lib/Driver/ToolChains/MinGW.h
===
--- lib/Driver/ToolChains/MinGW.h
+++ lib/Driver/ToolChains/MinGW.h
@@ -65,6 +65,8 @@
   bool isPIEDefault() const override;
   bool isPICDefaultForced() const override;
 
+  SanitizerMask getSupportedSanitizers() const override;
+
   llvm::ExceptionHandling GetExceptionModel(
   const llvm::opt::ArgList &Args) const override;
 
Index: lib/Driver/ToolChains/MinGW.cpp
===
--- lib/Driver/ToolChains/MinGW.cpp
+++ lib/Driver/ToolChains/MinGW.cpp
@@ -14,6 +14,7 @@
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
+#include "clang/Driver/SanitizerArgs.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
@@ -95,7 +96,7 @@
 const char *LinkingOutput) const {
   const ToolChain &TC = getToolChain();
   const Driver &D = TC.getDriver();
-  // const SanitizerArgs &Sanitize = TC.getSanitizerArgs();
+  const SanitizerArgs &Sanitize = TC.getSanitizerArgs();
 
   ArgStringList CmdArgs;
 
@@ -187,8 +188,6 @@
   TC.AddFilePathLibArgs(Args, CmdArgs);
   AddLinkerInputs(TC, Inputs, Args, CmdArgs, JA);
 
-  // TODO: Add ASan stuff here
-
   // TODO: Add profile stuff here
 
   if (TC.ShouldLinkCXXStdlib(Args)) {
@@ -231,6 +230,24 @@
   if (Args.hasArg(options::OPT_pthread))
 CmdArgs.push_back("-lpthread");
 
+  if (Sanitize.needsAsanRt()) {
+// MinGW always links against a shared CRT.
+CmdArgs.push_back(
+TC.getCompilerRTArgString(Args, "asan_dynamic", true));
+CmdArgs.push_back(
+TC.getCompilerRTArgString(Args, "asan_dynamic_runtime_thunk"));
+CmdArgs.push_back(Args.MakeArgString("--require-defined"));
+CmdArgs.push_back(Args.MakeArgString(TC.getArch() == llvm::Triple::x86
+ ? "___asan_seh_interceptor"
+ : "__asan_seh_interceptor"));
+// Make sure the linker consider all object files from the dynamic
+// runtime thunk.
+CmdArgs.push_back(Args.MakeArgString("--whole-archive"));
+CmdArgs.push_back(Args.MakeArgString(
+TC.getCompilerRT(Args, "asan_dynamic_runtime_thunk")));
+CmdArgs.push_back(Args.MakeArgString("--no-whole-archive"));
+  }
+
   if (!HasWindowsApp) {
 // Add system libraries. If linking to libwindowsapp.a, that import
 // library replaces all these and we shouldn't accidentally try to
@@ -407,6 +424,12 @@
   return llvm::ExceptionHandling::DwarfCFI;
 }
 
+SanitizerMask toolchains::MinGW::getSupportedSanitizers() const {
+  SanitizerMask Res = ToolChain::getSupportedSanitizers();
+  Res |= SanitizerKind::Address;
+  return Res;
+}
+
 void toolchains::MinGW::AddCudaIncludeArgs(const ArgList &DriverArgs,
ArgStringList &CC1Args) const {
   CudaInstallation.AddCudaIncludeArgs(DriverArgs, CC1Args);
Index: lib/Driver/ToolChain.cpp
===

[PATCH] D52534: [clangd] NFC: Update Serialization routine documentation

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added a comment.

In https://reviews.llvm.org/D52534#1246190, @sammccall wrote:

> The wording change is in https://reviews.llvm.org/D52531.


Oh, I didn't see that one. Thanks!


https://reviews.llvm.org/D52534



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

[PATCH] D52534: [clangd] NFC: Update Serialization routine documentation

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev abandoned this revision.
kbobyrev added a comment.

This is no longer needed then.


https://reviews.llvm.org/D52534



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

[PATCH] D51689: [clangd] Dense posting lists proof-of-concept

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev resigned from this revision.
kbobyrev added a comment.

For anyone interested in the direction of posting list compression, an 
implementation of Variable length Byte compression (VByte) has landed: 
https://reviews.llvm.org/D52300.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51689



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

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like elements

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Hey @mgrang! Let's see some results on some projects and answer NoQ's comment, 
then I think we can put this in for all the world to see.


https://reviews.llvm.org/D50488



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

[PATCH] D52290: [driver][mips] Adjust target triple accordingly to provided ABI name

[Simon Atanasyan via Phabricator via cfe-commits]

atanasyan updated this revision to Diff 167091.
atanasyan edited the summary of this revision.
atanasyan added a comment.

Always adjust target triple (explicitly and implicitly provided) accordingly to 
ABI name.


Repository:
  rC Clang

https://reviews.llvm.org/D52290

Files:
  lib/Driver/Driver.cpp
  test/Driver/mips-abi.c


Index: test/Driver/mips-abi.c
===
--- test/Driver/mips-abi.c
+++ test/Driver/mips-abi.c
@@ -162,3 +162,28 @@
 // RUN:-march=unknown 2>&1 \
 // RUN:   | FileCheck -check-prefix=MIPS-ARCH-UNKNOWN %s
 // MIPS-ARCH-UNKNOWN: error: unknown target CPU 'unknown'
+
+// Check adjusting of target triple accordingly to `-mabi` option.
+// RUN: %clang -target mips64-linux-gnu -mabi=32 -### %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=TARGET-O32 %s
+// TARGET-O32: "-triple" "mips-unknown-linux-gnu"
+// TARGET-O32: "-target-cpu" "mips32r2"
+// TARGET-O32: "-target-abi" "o32"
+// TARGET-O32: ld{{.*}}"
+// TARGET-O32: "-m" "elf32btsmip"
+
+// RUN: %clang -target mips-linux-gnu -mabi=n32 -### %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=TARGET-N32 %s
+// TARGET-N32: "-triple" "mips64-unknown-linux-gnu"
+// TARGET-N32: "-target-cpu" "mips64r2"
+// TARGET-N32: "-target-abi" "n32"
+// TARGET-N32: ld{{.*}}"
+// TARGET-N32: "-m" "elf32btsmipn32"
+
+// RUN: %clang -target mips-linux-gnu -mabi=64 -### %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=TARGET-N64 %s
+// TARGET-N64: "-triple" "mips64-unknown-linux-gnu"
+// TARGET-N64: "-target-cpu" "mips64r2"
+// TARGET-N64: "-target-abi" "n64"
+// TARGET-N64: ld{{.*}}"
+// TARGET-N64: "-m" "elf64btsmip"
Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -481,6 +481,16 @@
 Target.setVendorName("intel");
   }
 
+  // If target is MIPS adjust the target triple
+  // accordingly to provided ABI name.
+  A = Args.getLastArg(options::OPT_mabi_EQ);
+  if (A && Target.isMIPS())
+Target = llvm::StringSwitch(A->getValue())
+ .Case("32", Target.get32BitArchVariant())
+ .Case("n32", Target.get64BitArchVariant())
+ .Case("64", Target.get64BitArchVariant())
+ .Default(Target);
+
   return Target;
 }
 


Index: test/Driver/mips-abi.c
===
--- test/Driver/mips-abi.c
+++ test/Driver/mips-abi.c
@@ -162,3 +162,28 @@
 // RUN:-march=unknown 2>&1 \
 // RUN:   | FileCheck -check-prefix=MIPS-ARCH-UNKNOWN %s
 // MIPS-ARCH-UNKNOWN: error: unknown target CPU 'unknown'
+
+// Check adjusting of target triple accordingly to `-mabi` option.
+// RUN: %clang -target mips64-linux-gnu -mabi=32 -### %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=TARGET-O32 %s
+// TARGET-O32: "-triple" "mips-unknown-linux-gnu"
+// TARGET-O32: "-target-cpu" "mips32r2"
+// TARGET-O32: "-target-abi" "o32"
+// TARGET-O32: ld{{.*}}"
+// TARGET-O32: "-m" "elf32btsmip"
+
+// RUN: %clang -target mips-linux-gnu -mabi=n32 -### %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=TARGET-N32 %s
+// TARGET-N32: "-triple" "mips64-unknown-linux-gnu"
+// TARGET-N32: "-target-cpu" "mips64r2"
+// TARGET-N32: "-target-abi" "n32"
+// TARGET-N32: ld{{.*}}"
+// TARGET-N32: "-m" "elf32btsmipn32"
+
+// RUN: %clang -target mips-linux-gnu -mabi=64 -### %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=TARGET-N64 %s
+// TARGET-N64: "-triple" "mips64-unknown-linux-gnu"
+// TARGET-N64: "-target-cpu" "mips64r2"
+// TARGET-N64: "-target-abi" "n64"
+// TARGET-N64: ld{{.*}}"
+// TARGET-N64: "-m" "elf64btsmip"
Index: lib/Driver/Driver.cpp
===
--- lib/Driver/Driver.cpp
+++ lib/Driver/Driver.cpp
@@ -481,6 +481,16 @@
 Target.setVendorName("intel");
   }
 
+  // If target is MIPS adjust the target triple
+  // accordingly to provided ABI name.
+  A = Args.getLastArg(options::OPT_mabi_EQ);
+  if (A && Target.isMIPS())
+Target = llvm::StringSwitch(A->getValue())
+ .Case("32", Target.get32BitArchVariant())
+ .Case("n32", Target.get64BitArchVariant())
+ .Case("64", Target.get64BitArchVariant())
+ .Default(Target);
+
   return Target;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52503: [clangd] Fix bugs with incorrect memory estimate report

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:251
   Bytes += InvertedIndex.getMemorySize();
-  for (const auto &P : InvertedIndex)
-Bytes += P.second.bytes();
+  for (const auto &TokenToPostingList : InvertedIndex)
+Bytes += TokenToPostingList.first.Data.size() +

kbobyrev wrote:
> ioeric wrote:
> > kbobyrev wrote:
> > > ioeric wrote:
> > > > Would `InvertedIndex.getMemorySize()` be a better estimate?
> > > IIUC this is only precise for the objects which do not make any 
> > > allocations (e.g. `std::vector` and `std::string` memory estimate would 
> > > not be "correct").
> > > 
> > > From the documentation
> > > 
> > > > This is just the raw memory used by DenseMap. If entries are pointers 
> > > > to objects, the size of the referenced objects are not included.
> > > 
> > > I also have `Bytes += InvertedIndex.getMemorySize();` above, so the 
> > > purpose of this code would be to take into account the size of these 
> > > "reference objects".
> > > 
> > > However, since `PostingList::bytes()` also returns underlying 
> > > `std::vector` size, this code will probably add these `std::vector` 
> > > objects size twice (the first one was in 
> > > `InvertedIndex.getMemorySize()`). I should fix that.
> > > `If entries are pointers to objects, the size of the referenced objects 
> > > are not included.`
> > This applies to *pointers* to objects, but the `PostingList`s in 
> > InvertedList are not pointerd? So it seems to me that 
> > `InvertedIndex.getMemorySize()` covers everything in the `InvertedIndex`. 
> > One way to verify is probably check the size calculated with loop against 
> > `InvertedIndex.getMemorySize()`.
> As discussed offline, pointers and references are an example of objects which 
> would be incorrectly measured by `DesneMap::getMemorySize()`, but 
> `std::vector` and `std::string` are pointers in a way because they also just 
> store a pointer to the underlying storage which is hidden from 
> `DenseMap::getMemorySize()`; thus, it would be more precise to use the 
> proposed scheme for memory estimation.
I see. I was expecting DenseMap to maintain an arena which could provide memory 
usage of all owned data, but it seems to only consider `sizeof(KV-Pair)`...

If the posting lists (vectors) are not covered, shouldn't we also also add the 
size of tokens (string) here?


https://reviews.llvm.org/D52503



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

[PATCH] D52117: Generate llvm.loop.parallel_accesses instead of llvm.mem.parallel_loop_access metadata.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur updated this revision to Diff 167099.
Meinersbur added a comment.
Herald added subscribers: llvm-commits, dexonsmith, steven_wu, eraman, 
mehdi_amini.

- Rebase
- Use call access group if instruction's access group is not set


Repository:
  rL LLVM

https://reviews.llvm.org/D52117

Files:
  docs/LangRef.rst
  include/llvm/IR/LLVMContext.h
  include/llvm/Transforms/Utils/LoopUtils.h
  lib/Analysis/LoopInfo.cpp
  lib/IR/LLVMContext.cpp
  lib/Transforms/InstCombine/InstCombineCalls.cpp
  lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
  lib/Transforms/Scalar/LoopVersioningLICM.cpp
  lib/Transforms/Scalar/SROA.cpp
  lib/Transforms/Scalar/Scalarizer.cpp
  lib/Transforms/Utils/InlineFunction.cpp
  lib/Transforms/Utils/Local.cpp
  lib/Transforms/Utils/LoopUtils.cpp
  lib/Transforms/Utils/SimplifyCFG.cpp
  test/ThinLTO/X86/lazyload_metadata.ll
  test/Transforms/Inline/parallel-loop-md-callee.ll
  test/Transforms/Inline/parallel-loop-md.ll
  test/Transforms/InstCombine/loadstore-metadata.ll
  test/Transforms/InstCombine/mem-par-metadata-memcpy.ll
  test/Transforms/LoopVectorize/X86/force-ifcvt.ll
  test/Transforms/LoopVectorize/X86/illegal-parallel-loop-uniform-write.ll
  test/Transforms/LoopVectorize/X86/parallel-loops-after-reg2mem.ll
  test/Transforms/LoopVectorize/X86/parallel-loops.ll
  test/Transforms/LoopVectorize/X86/pr34438.ll
  test/Transforms/LoopVectorize/X86/vect.omp.force.ll
  test/Transforms/LoopVectorize/X86/vect.omp.force.small-tc.ll
  test/Transforms/LoopVectorize/X86/vector_max_bandwidth.ll
  test/Transforms/SROA/mem-par-metadata-sroa.ll
  test/Transforms/Scalarizer/basic.ll
  test/Transforms/SimplifyCFG/combine-parallel-mem-md.ll

Index: test/Transforms/SimplifyCFG/combine-parallel-mem-md.ll
===
--- test/Transforms/SimplifyCFG/combine-parallel-mem-md.ll
+++ test/Transforms/SimplifyCFG/combine-parallel-mem-md.ll
@@ -8,39 +8,39 @@
   br label %for.body
 
 ; CHECK-LABEL: @Test
-; CHECK: load i32, i32* {{.*}}, align 4, !llvm.mem.parallel_loop_access !0
-; CHECK: load i32, i32* {{.*}}, align 4, !llvm.mem.parallel_loop_access !0
-; CHECK: store i32 {{.*}}, align 4, !llvm.mem.parallel_loop_access !0
+; CHECK: load i32, i32* {{.*}}, align 4, !llvm.access.group !0
+; CHECK: load i32, i32* {{.*}}, align 4, !llvm.access.group !0
+; CHECK: store i32 {{.*}}, align 4, !llvm.access.group !0
 ; CHECK-NOT: load
 ; CHECK-NOT: store
 
 for.body: ; preds = %cond.end, %entry
   %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %cond.end ]
   %arrayidx = getelementptr inbounds i32, i32* %p, i64 %indvars.iv
-  %0 = load i32, i32* %arrayidx, align 4, !llvm.mem.parallel_loop_access !0
+  %0 = load i32, i32* %arrayidx, align 4, !llvm.access.group !0
   %cmp1 = icmp eq i32 %0, 0
   br i1 %cmp1, label %cond.true, label %cond.false
 
 cond.false:   ; preds = %for.body
   %arrayidx3 = getelementptr inbounds i32, i32* %res, i64 %indvars.iv
-  %v = load i32, i32* %arrayidx3, align 4, !llvm.mem.parallel_loop_access !0
+  %v = load i32, i32* %arrayidx3, align 4, !llvm.access.group !0
   %arrayidx7 = getelementptr inbounds i32, i32* %d, i64 %indvars.iv
-  %1 = load i32, i32* %arrayidx7, align 4, !llvm.mem.parallel_loop_access !0
+  %1 = load i32, i32* %arrayidx7, align 4, !llvm.access.group !0
   %add = add nsw i32 %1, %v
   br label %cond.end
 
 cond.true:   ; preds = %for.body
   %arrayidx4 = getelementptr inbounds i32, i32* %res, i64 %indvars.iv
-  %w = load i32, i32* %arrayidx4, align 4, !llvm.mem.parallel_loop_access !0
+  %w = load i32, i32* %arrayidx4, align 4, !llvm.access.group !0
   %arrayidx8 = getelementptr inbounds i32, i32* %d, i64 %indvars.iv
-  %2 = load i32, i32* %arrayidx8, align 4, !llvm.mem.parallel_loop_access !0
+  %2 = load i32, i32* %arrayidx8, align 4, !llvm.access.group !0
   %add2 = add nsw i32 %2, %w
   br label %cond.end
 
 cond.end: ; preds = %for.body, %cond.false
   %cond = phi i32 [ %add, %cond.false ], [ %add2, %cond.true ]
   %arrayidx9 = getelementptr inbounds i32, i32* %res, i64 %indvars.iv
-  store i32 %cond, i32* %arrayidx9, align 4, !llvm.mem.parallel_loop_access !0
+  store i32 %cond, i32* %arrayidx9, align 4, !llvm.access.group !0
   %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
   %exitcond = icmp eq i64 %indvars.iv.next, 16
   br i1 %exitcond, label %for.end, label %for.body, !llvm.loop !0
@@ -51,5 +51,6 @@
 
 attributes #0 = { norecurse nounwind uwtable }
 
-!0 = distinct !{!0, !1}
+!0 = distinct !{!0, !1, !{!"llvm.loop.parallel_accesses", !10}}
 !1 = !{!"llvm.loop.vectorize.enable", i1 true}
+!10 = distinct !{}
Index: test/Transforms/Scalarizer/basic.ll
===
--- test/Transforms/Scalarizer/basic.ll
+++ test/Transforms/Scalarizer/basic.ll
@@ -205,28 +205,28 @@
   ret void
 }
 
-; 

[PATCH] D52334: [clang-tidy] Build it even without static analyzer

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

Can I go ahead and merge this now?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52334



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

[PATCH] D52390: [analyzer] Add StackSizeChecker to StaticAnalyzer

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

One final nit apart from a few in-line comments (most of them are just arguing 
with @Szelethus anyways 😛): KB vs. KiB  
(remember how a 2 TB hard drive appears as 1.8 TB on your machine? Because it's 
TiB!) is one of my pet peeves especially that Windows and most people //still// 
get it wrong nowadays (and they also teach it wrong). Can we change the default 
from `100 000` to `102 400`? That would make it truly `100 KiB` in the proper 
sense.




Comment at: 
include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:16
+//===--===//
+
+#include "llvm/ADT/DenseMap.h"

Szelethus wrote:
> Missing header guard.
Speaking of missing header guards and in general: apart from tests, it might be 
a good idea to run CSA and Clang-Tidy //on your checker//'s code. E.g. Tidy has 
a [[ https://clang.llvm.org/extra/clang-tidy/checks/llvm-header-guard.html | 
`llvm-header-guard` ]] checker -- which might //not// find this particular 
issue, but in general we could find some issues on this code itself.

(For the ultimate version, run your own checker on your own checker's code... 
maybe you yourself used a too large stack somewhere! 😜 )



Comment at: 
include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:102-111
+  bool hasEmptyFlaggedUsageStored(const Stmt *S) const {
+auto iter = StmtSubtreeUsages.find(S);
+return iter != StmtSubtreeUsages.end() &&
+   iter->second == Usage::EmptyFlagged;
+  }
+  bool hasEmptyFlaggedUsageStored(const Decl *D) const {
+auto iter = DeclSubtreeUsages.find(D);

Szelethus wrote:
> Would `isEmptyFlagged` be a better method name?
Then calling this method would mean answering this question: "A Stmt/Decl is 
empty flagged?" This is a "What?" moment.



Comment at: 
include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:117
+
+  int varSize(const VarDecl *Var) const {
+return Var->hasLocalStorage() * sizeInBytes(Var->getType());

If we're talking about sizes, `int` (especially because it's //`signed`// 
`int`) might not be the most appropriate return type for these functions.



Comment at: 
include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:124
+   ? 0
+   : Context.getTypeSize(T) / Context.getCharWidth();
+  }

Szelethus wrote:
> The function name states `sizeInBytes`, but
> * `ASTContext::getTypeSizeInChars` returns the width in `CharUnits`,
> * which you divide with `Context.getCharWidth()`, which is in bits (according 
> to the doxygen comments),
> * and we end up getting the amount of bytes `T` occupies?
> I might make a fool out of myself, but wouldn't the formula look something 
> like this: `(Context.getTypeSize(T) * Context.getCharWidth()) / 8`?
Need to make a distinction whether or not we want to use `byte` in the sense of 
`8 bit` or in the sense of `sizeof(char)`. Also, the code calls `getTypeSize`, 
not `getTypeSize`~~`InChars`~~. This gives the answer in pure bits.

(Because of this, your suggested formula is wrong. `getTypeSize()` is already 
in bits, which you multiply further by `getCharWidth()` -- which is //on some 
systems// 8 -- then you divide it out. In most conventional systems the 
division cancels out the multiplication, and on systems where a 
`sizeof(char)`-byte is not 8, you'll get a non-integer quotient truncated as 
result.) 

So:

 * in case of you want to define `byte` to be `8 bits` forever and always, the 
divisor should be `8`, not `getCharWidth()`.
 * In case you want to define `byte` to be `sizeof(char)`, then the current 
code **is** correct, but you could just use: [[ 
https://clang.llvm.org/doxygen/classclang_1_1ASTContext.html#aa8c44e88d6ce9b3cce885b62d3cd5dff
 | `Context.getTypeSizeInChar()` ]] `.` [[ 
https://clang.llvm.org/doxygen/classclang_1_1CharUnits.html#a968b1a66a449ab256c4dd176bd663c07
 | `getQuantity()` ]].

Just for the record: the C++14 standard says the following in § 1.7 
`intro.memory`:

> The fundamental storage unit in the C++ memory is the **byte**.  A byte is 
> //at least large enough// to contain any member of the basic execution 
> character set (-> § 2.3 `lex.charset`) and the eight-bit code units of the 
> Unicode UTF-8 encoding form and is composed of a contiguous sequence of bits, 
> **the number of which is implementation-defined**.

(`lex.charset` is basically an ASCII subset of 96 cardinality, lowercase, 
uppercase, parens, etc.)

I'm not sure what the convention is for error reports when referring to bytes, 
but maybe it would be better to consider `byte` in the C++ sense and let the 
value change based on what target the analyzer file is being compiled for.



Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:25
+#include "clang/StaticAna

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

[Whisperity via Phabricator via cfe-commits]

whisperity added a subscriber: baloghadamsoftware.
whisperity added inline comments.



Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:1038
+
+  SmallString<128> NewAddNullTermExprStr;
+  NewAddNullTermExprStr = "\n";

aaron.ballman wrote:
> This should be done using a `Twine`.
Can you please give an example of how to well-approach this? We had issues with 
using Twines on the stack and then later on running into weird undefined 
behaviour. The documentation is not clear to a lot of our colleagues on where 
the `Twine`'s usage barriers are... (Asking this not just for @Charusso but 
also for a few more colleagues, @baloghadamsoftware e.g.)


https://reviews.llvm.org/D45050



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

[PATCH] D52034: [Clang] Add options -Xclang -coverage-filter and -Xclang -coverage-exclude to filter the files to instrument with gcov

[calixte via Phabricator via cfe-commits]

calixte updated this revision to Diff 167101.
calixte added a comment.

Fix tests


Repository:
  rC Clang

https://reviews.llvm.org/D52034

Files:
  include/clang/Driver/CC1Options.td
  include/clang/Frontend/CodeGenOptions.h
  lib/CodeGen/BackendUtil.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/Inputs/code-coverage-filter1.h
  test/CodeGen/Inputs/code-coverage-filter2.h
  test/CodeGen/code-coverage-filter.c

Index: test/CodeGen/code-coverage-filter.c
===
--- /dev/null
+++ test/CodeGen/code-coverage-filter.c
@@ -0,0 +1,84 @@
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes %s -o - \
+// RUN:| FileCheck -check-prefix=ALL %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -coverage-exclude=".*\.h$" %s -o - \
+// RUN:| FileCheck -check-prefix=NO-HEADER %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -coverage-filter=".*\.c$" %s -o - \
+// RUN:| FileCheck -check-prefix=NO-HEADER %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -coverage-filter=".*\.c$:.*1\.h$" %s -o - \
+// RUN:| FileCheck -check-prefix=NO-HEADER2 %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -coverage-exclude=".*2\.h$:.*1\.h$" %s -o - \
+// RUN:| FileCheck -check-prefix=JUST-C %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -coverage-exclude=".*code\-coverage\-filter\.c$" %s -o - \
+// RUN:| FileCheck -check-prefix=HEADER %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -coverage-filter=".*\.c$" -coverage-exclude=".*\.c$" %s -o - \
+// RUN:| FileCheck -check-prefix=NONE %s
+// RUN: %clang_cc1 -emit-llvm -femit-coverage-data -femit-coverage-notes -coverage-filter=".*\.c$" -coverage-exclude=".*\.h$" %s -o - \
+// RUN:| FileCheck -check-prefix=JUST-C %s
+
+#include "Inputs/code-coverage-filter1.h"
+#include "Inputs/code-coverage-filter2.h"
+
+void test() {
+test1();
+test2();
+}
+
+// ALL: define void @test1() #0 {{.*}}
+// ALL: {{.*}}__llvm_gcov_ctr{{.*}}
+// ALL: ret void
+// ALL: define void @test2() #0 {{.*}}
+// ALL: {{.*}}__llvm_gcov_ctr{{.*}}
+// ALL: ret void
+// ALL: define void @test() #0 {{.*}}
+// ALL: {{.*}}__llvm_gcov_ctr{{.*}}
+// ALL: ret void
+
+// NO-HEADER: define void @test1() #0 {{.*}}
+// NO-HEADER-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER: ret void
+// NO-HEADER: define void @test2() #0 {{.*}}
+// NO-HEADER-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER: ret void
+// NO-HEADER: define void @test() #0 {{.*}}
+// NO-HEADER: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER: ret void
+
+// NO-HEADER2: define void @test1() #0 {{.*}}
+// NO-HEADER2: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER2: ret void
+// NO-HEADER2: define void @test2() #0 {{.*}}
+// NO-HEADER2-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER2: ret void
+// NO-HEADER2: define void @test() #0 {{.*}}
+// NO-HEADER2: {{.*}}__llvm_gcov_ctr{{.*}}
+// NO-HEADER2: ret void
+
+// JUST-C: define void @test1() #0 {{.*}}
+// JUST-C-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// JUST-C: ret void
+// JUST-C: define void @test2() #0 {{.*}}
+// JUST-C-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// JUST-C: ret void
+// JUST-C: define void @test() #0 {{.*}}
+// JUST-C: {{.*}}__llvm_gcov_ctr{{.*}}
+// JUST-C: ret void
+
+// HEADER: define void @test1() #0 {{.*}}
+// HEADER: {{.*}}__llvm_gcov_ctr{{.*}}
+// HEADER: ret void
+// HEADER: define void @test2() #0 {{.*}}
+// HEADER: {{.*}}__llvm_gcov_ctr{{.*}}
+// HEADER: ret void
+// HEADER: define void @test() #0 {{.*}}
+// HEADER-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// HEADER: ret void
+
+// NONE: define void @test1() #0 {{.*}}
+// NONE-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NONE: ret void
+// NONE: define void @test2() #0 {{.*}}
+// NONE-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NONE: ret void
+// NONE: define void @test() #0 {{.*}}
+// NONE-NOT: {{.*}}__llvm_gcov_ctr{{.*}}
+// NONE: ret void
Index: test/CodeGen/Inputs/code-coverage-filter2.h
===
--- /dev/null
+++ test/CodeGen/Inputs/code-coverage-filter2.h
@@ -0,0 +1,2 @@
+void test2() {
+}
Index: test/CodeGen/Inputs/code-coverage-filter1.h
===
--- /dev/null
+++ test/CodeGen/Inputs/code-coverage-filter1.h
@@ -0,0 +1,2 @@
+void test1() {
+}
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -801,6 +801,8 @@
 Opts.CoverageExtraChecksum = Args.hasArg(OPT_coverage_cfg_checksum);
 Opts.CoverageNoFunctionNamesInData =
 Args.hasArg(OPT_coverage_no_function_names_in_data);
+Opts.CoverageFilter = Args.getLastArgValue(OPT_coverage_filter);
+Opts.CoverageExclude = Args.getLastArgValue(OPT_coverage_exclude);
 Opts.CoverageExi

[PATCH] D52034: [Clang] Add options -Xclang -coverage-filter and -Xclang -coverage-exclude to filter the files to instrument with gcov

[calixte via Phabricator via cfe-commits]

calixte added a comment.

I reported a bug for gcc: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87442
@vsk I'd like to add documentation in Docs/UsersManual.rst, but I've no idea on 
what's a good place for this (I look for option 
-coverage-no-function-names-in-data, but I didn't get anything). So could you 
give a me a clue please ?


Repository:
  rC Clang

https://reviews.llvm.org/D52034



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

[PATCH] D52334: [clang-tidy] Build it even without static analyzer

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D52334#1246356, @steveire wrote:

> Can I go ahead and merge this now?




In https://reviews.llvm.org/D52334#1242813, @lebedev.ri wrote:

> `#ifdef` hell is usually messy and is a source of problems.
>  May i ask what is the motivation for this change?


It seems that ^ comment was lost.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52334



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

[PATCH] D52392: [X86] For lzcnt/tzcnt intrinsics use cttz/ctlz intrinsics with zero_undef flag set to false.

[Simon Pilgrim via Phabricator via cfe-commits]

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

LGTM, thanks for checking.

Please update the *-intrinsics-fast-isel.ll llvm test cases to match the 
*-builtins.c changes.


Repository:
  rC Clang

https://reviews.llvm.org/D52392



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

[PATCH] D52360: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

Config option is a good idea but it must not be empty by default. The checker 
must ignore all pointer and references by default based on their names.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52360



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

[PATCH] D51402: [OpenCL] Adding cl_intel_planar_yuv extension

[Dmitry Sidorov via Phabricator via cfe-commits]

sidorovd added inline comments.



Comment at: lib/Headers/opencl-c.h:26
+#if __OPENCL_C_VERSION__ >= CL_VERSION_1_2
+#ifndef cl_intel_planar_yuv
+#define cl_intel_planar_yuv

Anastasia wrote:
> Anastasia wrote:
> > @yaxunl, do you think we need to add some kind of architecture guard for 
> > such things? Like it should only be added if the architecture supports the 
> > extension? But I guess `-cl-ext=+cl_intel_planar_yuv` trick might not work 
> > here because it's not a Clang known extension?
> > 
> > So may be the right solution here is to introduce a target specific header? 
> > For now it can be explicitly included but we could think of a target hook 
> > to preload a target specific header...
> @sidorovd, I am wondering if we could instead extend 
> '-cl-ext=+cl_intel_planar_yuv' to work with non-builtin extensions? Would 
> that be an acceptable solution for vendor extensions to be added to the 
> common header?
Sorry for a long response.
I have covered a little study on how to enable extensions via adding them in 
clang, common header or without any changes (yeap, that is also a case) and how 
adding -cl-ext+ in the RUN string affects the test's (introduced in this patch, 
but with removed versioning) result. And found out that I don't understand how 
it works or it's supposed to work. Here is a result of my study: {F7312300}
 

|  Changes  | cl-ext+ | cl-ext- | no cl-ext |  
| opencl-c.hver  >=1.2 | defined, known, supported | defined, 
known, supported| defined, known, supported|
| opencl-c.h   ver < 1.2   | undefined, known, supported | undefined, 
unknown  | undefined, unknown  |  
| OpenCLExtensions.def  ver  >=1.2 | defined, known, supported |   
undefined, known, unsupported   |  defined, known, supported  |
| OpenCLExtensions.def  ver < 1.2   | undefined, known, unsupported | 
undefined, known, unsupported |undefined, known, unsupported |
| No changes  ver  >=1.2 | undefined, known, supported| undefined, unknown  
  | undefined, unknown |
| No changes  ver < 1.2| undefined, known, supported   | undefined, unknown 
| undefined, unknown|

So from my perspective there shouldn't be any difference between the result 
depending on an approach to use to enable an extension. But that is not how 
it's happening in reality, as an example see how values put in column #3 
differs for changes in opencl-c.h and OpenCLExtensions.def.
Or am I wrong?






https://reviews.llvm.org/D51402



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

[PATCH] D52360: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

[Danila Malyutin via Phabricator via cfe-commits]

danilaml added a comment.

Would that also skip checks for something like `shared_ptr`?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52360



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

[PATCH] D52544: Improve diagnostics range reporting.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added reviewers: sammccall, ioeric.
Herald added subscribers: cfe-commits, arphaman, jkorous, ilya-biryukov.

If we have some range information coming from clang diagnostic, promote
that one even if it doesn't contain diagnostic location inside.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52544

Files:
  clangd/Diagnostics.cpp
  unittests/clangd/ClangdUnitTests.cpp


Index: unittests/clangd/ClangdUnitTests.cpp
===
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -79,8 +79,9 @@
 int main() {
   $typo[[go\
 o]]();
-  foo()$semicolon[[]]
+  foo()$semicolon[[]]//with comments
   $unk[[unknown]]();
+  double bar = $type[["foo"]];
 }
   )cpp");
   EXPECT_THAT(
@@ -97,7 +98,10 @@
   AllOf(Diag(Test.range("semicolon"), "expected ';' after expression"),
 WithFix(Fix(Test.range("semicolon"), ";", "insert ';'"))),
   // This range isn't provided by clang, we expand to the token.
-  Diag(Test.range("unk"), "use of undeclared identifier 'unknown'")));
+  Diag(Test.range("unk"), "use of undeclared identifier 'unknown'"),
+  Diag(Test.range("type"),
+   "cannot initialize a variable of type 'double' with an lvalue "
+   "of type 'const char [4]'")));
 }
 
 TEST(DiagnosticsTest, FlagsMatter) {
Index: clangd/Diagnostics.cpp
===
--- clangd/Diagnostics.cpp
+++ clangd/Diagnostics.cpp
@@ -52,17 +52,28 @@
   auto &M = D.getSourceManager();
   auto Loc = M.getFileLoc(D.getLocation());
   // Accept the first range that contains the location.
+  llvm::Optional PossibleRange;
   for (const auto &CR : D.getRanges()) {
 auto R = Lexer::makeFileCharRange(CR, M, L);
 if (locationInRange(Loc, R, M))
   return halfOpenToRange(M, R);
+// If there are no ranges that contain the location report the first range.
+if (!PossibleRange)
+  PossibleRange = halfOpenToRange(M, R);
   }
   // The range may be given as a fixit hint instead.
   for (const auto &F : D.getFixItHints()) {
 auto R = Lexer::makeFileCharRange(F.RemoveRange, M, L);
 if (locationInRange(Loc, R, M))
   return halfOpenToRange(M, R);
+// If there's a fixit that performs insertion, it has zero-width. Therefore
+// it can't contain the location of the diag, but it might be possible that
+// this should be reported as range. For example missing semicolon.
+if (!PossibleRange && R.getBegin() == R.getEnd())
+  PossibleRange = halfOpenToRange(M, R);
   }
+  if (PossibleRange)
+return *PossibleRange;
   // If no suitable range is found, just use the token at the location.
   auto R = Lexer::makeFileCharRange(CharSourceRange::getTokenRange(Loc), M, L);
   if (!R.isValid()) // Fall back to location only, let the editor deal with it.


Index: unittests/clangd/ClangdUnitTests.cpp
===
--- unittests/clangd/ClangdUnitTests.cpp
+++ unittests/clangd/ClangdUnitTests.cpp
@@ -79,8 +79,9 @@
 int main() {
   $typo[[go\
 o]]();
-  foo()$semicolon[[]]
+  foo()$semicolon[[]]//with comments
   $unk[[unknown]]();
+  double bar = $type[["foo"]];
 }
   )cpp");
   EXPECT_THAT(
@@ -97,7 +98,10 @@
   AllOf(Diag(Test.range("semicolon"), "expected ';' after expression"),
 WithFix(Fix(Test.range("semicolon"), ";", "insert ';'"))),
   // This range isn't provided by clang, we expand to the token.
-  Diag(Test.range("unk"), "use of undeclared identifier 'unknown'")));
+  Diag(Test.range("unk"), "use of undeclared identifier 'unknown'"),
+  Diag(Test.range("type"),
+   "cannot initialize a variable of type 'double' with an lvalue "
+   "of type 'const char [4]'")));
 }
 
 TEST(DiagnosticsTest, FlagsMatter) {
Index: clangd/Diagnostics.cpp
===
--- clangd/Diagnostics.cpp
+++ clangd/Diagnostics.cpp
@@ -52,17 +52,28 @@
   auto &M = D.getSourceManager();
   auto Loc = M.getFileLoc(D.getLocation());
   // Accept the first range that contains the location.
+  llvm::Optional PossibleRange;
   for (const auto &CR : D.getRanges()) {
 auto R = Lexer::makeFileCharRange(CR, M, L);
 if (locationInRange(Loc, R, M))
   return halfOpenToRange(M, R);
+// If there are no ranges that contain the location report the first range.
+if (!PossibleRange)
+  PossibleRange = halfOpenToRange(M, R);
   }
   // The range may be given as a fixit hint instead.
   for (const auto &F : D.getFixItHints()) {
 auto R = Lexer::makeFileCharRange(F.RemoveRange, M, L);
 if (locationInRange(Loc, R, M))
   return halfOpenToRange(M, R);
+// If there's a fixit that performs insertion, it has zero-width.

[PATCH] D52360: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D52360#1246440, @baloghadamsoftware wrote:

> Config option is a good idea but it must not be empty by default. The checker 
> must ignore all pointer and references by default based on their names.


I disagree, it **must** not have false-negatives by default.
It **may** have false-positives, that are controllable via the options.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52360



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

Re: [clang-tools-extra] r342730 - [clangd] Remember to serialize symbol origin in YAML.

[Eric Liu via cfe-commits]

I think it depends on how you interpret the origin. You could tie the
origin to the index or to the producer of the symbol. The current model
seems to be the later (origin is a configuration in SymbolCollector.
MergeIndex manipulates the origin, but I would rather treat that as a
special case). And I think it's conceptually simpler and more generic
(origin can be more than dynamic vs static). The downside is probably that
all symbols in yaml would have the same origin, which is a bit redundant
but doesn't seem to matter much.

Both models have their pros and cons, but I don't think it's worth spending
much time figuring out which one is better when it seems like a non-issue.

On Tue, Sep 25, 2018 at 6:58 PM Ilya Biryukov  wrote:

>
>
> Eric Liu  schrieb am Di., 25. Sep. 2018, 01:22:
>
>> On Tue, Sep 25, 2018 at 6:34 AM Ilya Biryukov 
>> wrote:
>>
>>> Why would we want to serialize the origin?
>>>
>> We only serialize and deserialize for the static index, it does not seem
>>> to be useful to serialize origin in that scenario.
>>>
>> We serialize Origin because it's a property of Symbol? The origin for the
>> current YAML symbols is defaulted to "unknown".
>>
> My view would be that it's a property of a symbol living in memory, but
> not the serialized one. E.g. all symbols read from the yaml index should
> have the static origin. If we store an origin, we allow loading symbols
> with non-static origin, it's hard to see how that would be useful.
>
>
>
>
>> It's true that we *currently* only serialize symbols from static index,
>> but there is nothing preventing us from doing it for other indexes with
>> different origins. You could probably override origins when loading static
>> index, but that doesn't seem to work in general.
>>
> The origin seems to be exactly the field that is set and manipulated by
> index implementations, but they all have the knowledge on what their origin
> is or how to combine origins of subindexes.
>
> Again, it seems there are two classes hiding in symbol. All other fields
> provide useful information about C++ semantics of the symbol, while origin
> provides some traceability when combining indexes. The former is something
> we need to serialize, the latter is something we can infer when
> deserializing.
>
> If we will have a use case for serializing the latter entity(with origin)
> for any reason, we might add that separately.
>
> Not sure if it's worth the trouble changing it, just wanted to point out
> that storing the  origin for each symbol in the yaml index for the purpose
> of indicating that the symbol is in the yaml index seems to be a bit off.
>
>
>
>> I checked this in without review as I thought this was a trivial fix. The
>> binary serialization also serializes the Origin field.
>>
> LG to be consistent with binary serialization, the same question applies
> to binary serialization.
>
> Again, the change itself seems fine, just nitpicking on whether putting
> origin into a symbol at that level makes sense.
>
>
>
>>> Am I missing something?
>>>
>>
>>> On Fri, Sep 21, 2018 at 3:06 PM Eric Liu via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Author: ioeric
 Date: Fri Sep 21 06:04:57 2018
 New Revision: 342730

 URL: http://llvm.org/viewvc/llvm-project?rev=342730&view=rev
 Log:
 [clangd] Remember to serialize symbol origin in YAML.

 Modified:
 clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
 clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp

 Modified: clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
 URL:
 http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp?rev=342730&r1=342729&r2=342730&view=diff

 ==
 --- clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp (original)
 +++ clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp Fri Sep 21
 06:04:57 2018
 @@ -27,6 +27,7 @@ namespace yaml {

  using clang::clangd::Symbol;
  using clang::clangd::SymbolID;
 +using clang::clangd::SymbolOrigin;
  using clang::clangd::SymbolLocation;
  using clang::index::SymbolInfo;
  using clang::index::SymbolKind;
 @@ -65,6 +66,17 @@ struct NormalizedSymbolFlag {
uint8_t Flag = 0;
  };

 +struct NormalizedSymbolOrigin {
 +  NormalizedSymbolOrigin(IO &) {}
 +  NormalizedSymbolOrigin(IO &, SymbolOrigin O) {
 +Origin = static_cast(O);
 +  }
 +
 +  SymbolOrigin denormalize(IO &) { return
 static_cast(Origin); }
 +
 +  uint8_t Origin = 0;
 +};
 +
  template <> struct MappingTraits {
static void mapping(IO &IO, SymbolLocation::Position &Value) {
  IO.mapRequired("Line", Value.Line);
 @@ -102,6 +114,8 @@ template <> struct MappingTraits
  MappingNormalization NSymbolID(IO,
 Sym.ID);
  MappingNormalization
 NSymbolFlag(
>>

[PATCH] D52499: [clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074)

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Thanks! lgtm.




Comment at: include/clang/Driver/CLCompatOptions.td:94
+def : CLFlag<"Gs">, HelpText<"Use stack probes (default)">,
+  Alias, AliasArgs<["4096"]>;
 def _SLASH_Gs : CLJoined<"Gs">,

https://docs.microsoft.com/en-us/cpp/build/reference/gs-control-stack-checking-calls?view=vs-2017
 still claims that /Gs is /Gs0 though. Is that just wrong? 
https://bugs.llvm.org/show_bug.cgi?id=39074#c2 suggests it is. Should we ask 
bruce to file a doc bug?

And since this flag is supposed to get the default behavior, should it be a 
CLIgnoredFlag instead of duplicating the 4096 somewhat redundantly?


Repository:
  rL LLVM

https://reviews.llvm.org/D52499



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

[PATCH] D52499: [clang-cl] Make /Gs imply default stack probes, not /Gs0 (PR39074)

[Hans Wennborg via Phabricator via cfe-commits]

hans added inline comments.



Comment at: include/clang/Driver/CLCompatOptions.td:94
+def : CLFlag<"Gs">, HelpText<"Use stack probes (default)">,
+  Alias, AliasArgs<["4096"]>;
 def _SLASH_Gs : CLJoined<"Gs">,

thakis wrote:
> https://docs.microsoft.com/en-us/cpp/build/reference/gs-control-stack-checking-calls?view=vs-2017
>  still claims that /Gs is /Gs0 though. Is that just wrong? 
> https://bugs.llvm.org/show_bug.cgi?id=39074#c2 suggests it is. Should we ask 
> bruce to file a doc bug?
> 
> And since this flag is supposed to get the default behavior, should it be a 
> CLIgnoredFlag instead of duplicating the 4096 somewhat redundantly?
I've submitted feedback on the document page: 
https://github.com/MicrosoftDocs/cpp-docs/issues/445

By not ignoring it, we enable using /Gs to override a previous e.g. /Gs0 flag, 
which I think is the correct thing to do.


Repository:
  rL LLVM

https://reviews.llvm.org/D52499



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

[PATCH] D52530: [analyzer] scan-build: if --status-bugs is passed, don't forget about the exit status of the actual build

[Jonathan Roelofs via Phabricator via cfe-commits]

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

LGTM


Repository:
  rC Clang

https://reviews.llvm.org/D52530



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

[PATCH] D52360: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In https://reviews.llvm.org/D52360#1246443, @danilaml wrote:

> Would that also skip checks for something like `shared_ptr`?


Yes, everything ending on `pointer`, `ptr`, `reference` or `ref`, first letter 
case insensitive.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52360



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

[PATCH] D52545: [docs] Update PostingList string representation format

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added reviewers: ioeric, sammccall.
kbobyrev added a project: clang-tools-extra.
Herald added subscribers: kadircet, arphaman, jkorous.

Because `PostingList` objects are compressed, it is now impossible to see 
elements other than the current one and the documentation doesn't match 
implementation anymore.


https://reviews.llvm.org/D52545

Files:
  clang-tools-extra/clangd/index/dex/Iterator.h


Index: clang-tools-extra/clangd/index/dex/Iterator.h
===
--- clang-tools-extra/clangd/index/dex/Iterator.h
+++ clang-tools-extra/clangd/index/dex/Iterator.h
@@ -94,9 +94,8 @@
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
-  /// PostingList entry and the element which is pointed to by the PostingList
-  /// iterator is enclosed in {} braces.
+  /// represented as "[(...)? (IDN | END) (...)?]" where IDN is N-th
+  /// PostingList entry.
   friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const Iterator &Iterator) {
 return Iterator.dump(OS);


Index: clang-tools-extra/clangd/index/dex/Iterator.h
===
--- clang-tools-extra/clangd/index/dex/Iterator.h
+++ clang-tools-extra/clangd/index/dex/Iterator.h
@@ -94,9 +94,8 @@
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
-  /// PostingList entry and the element which is pointed to by the PostingList
-  /// iterator is enclosed in {} braces.
+  /// represented as "[(...)? (IDN | END) (...)?]" where IDN is N-th
+  /// PostingList entry.
   friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const Iterator &Iterator) {
 return Iterator.dump(OS);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52360: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In https://reviews.llvm.org/D52360#1246452, @lebedev.ri wrote:

> I disagree, it **must** not have false-negatives by default.


What kind of false-negatives could be caused by smart pointers or references? 
Sane people do not name a type pointer or reference if they are not. Such types 
must never be passed by reference. Few people will use the check if they have 
to set tons of options for the basic expected behavior. For example in 
`CodeChecker` this check is disabled by default and is only enabled in the 
`Sensitive` profile.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52360



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

[PATCH] D52390: [analyzer] Add StackSizeChecker to StaticAnalyzer

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: 
include/clang/StaticAnalyzer/Checkers/StackUsageMeasuringVisitor.h:102-111
+  bool hasEmptyFlaggedUsageStored(const Stmt *S) const {
+auto iter = StmtSubtreeUsages.find(S);
+return iter != StmtSubtreeUsages.end() &&
+   iter->second == Usage::EmptyFlagged;
+  }
+  bool hasEmptyFlaggedUsageStored(const Decl *D) const {
+auto iter = DeclSubtreeUsages.find(D);

whisperity wrote:
> Szelethus wrote:
> > Would `isEmptyFlagged` be a better method name?
> Then calling this method would mean answering this question: "A Stmt/Decl is 
> empty flagged?" This is a "What?" moment.
Right. And it doesn't actually describe what the function does. 
`hasEmptyFlaggedSubDecl`?



Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:25
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include 
+

whisperity wrote:
> Szelethus wrote:
> > Don't include standard stream libraries: 
> > https://llvm.org/docs/CodingStandards.html#include-iostream-is-forbidden
> > Note that using the other stream headers (`` for example) is not 
> > problematic in this regard 
Oh, right, sorry about that. In either case, every other checker uses LLVM 
streams, so I guess `llvm::raw_*_ostream` are the preferred stream classes 
anyways.



Comment at: lib/StaticAnalyzer/Checkers/StackSizeChecker.cpp:41
+: public Checker {
+  mutable std::unique_ptr StackOverflowBugType;
+

whisperity wrote:
> Szelethus wrote:
> > I think you don't need to make this `mutable` anymore, you can just 
> > initialize it in the constructor.
> `generateError` is const-qualified but writes this.
But I'm not sure that it should. At least, when I wrote my checker, I didn't 
have to: 
https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp#L166


Repository:
  rC Clang

https://reviews.llvm.org/D52390



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

r343105 - [analyzer] scan-build: if --status-bugs is passed, don't forget about the exit status of the actual build

[Roman Lebedev via cfe-commits]

Author: lebedevri
Date: Wed Sep 26 06:08:44 2018
New Revision: 343105

URL: http://llvm.org/viewvc/llvm-project?rev=343105&view=rev
Log:
[analyzer] scan-build: if --status-bugs is passed, don't forget about the exit 
status of the actual build

Summary:
This has been bothering me for a while, but only now i have actually looked 
into this.
I'm using one CI job for static analysis - clang static analyzers as compilers 
+ clang-tidy via cmake.
And i'd like for the build to fail if at least one of those finds issues.
If clang-tidy finds issues, it will fail the build since the warnings-as-errors 
is set.
If static analyzer finds anything, since --status-bugs is set, it will fail the 
build.
But if clang-tidy find anything, but static analyzer does not, the build 
succeeds :/

Reviewers: sylvestre.ledru, alexfh, jroelofs, ygribov, george.karpenkov, 
krememek

Reviewed By: jroelofs

Subscribers: xazax.hun, szepet, a.sidorin, mikhail.ramalho, Szelethus, 
cfe-commits

Differential Revision: https://reviews.llvm.org/D52530

Modified:
cfe/trunk/tools/scan-build/bin/scan-build

Modified: cfe/trunk/tools/scan-build/bin/scan-build
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/scan-build/bin/scan-build?rev=343105&r1=343104&r2=343105&view=diff
==
--- cfe/trunk/tools/scan-build/bin/scan-build (original)
+++ cfe/trunk/tools/scan-build/bin/scan-build Wed Sep 26 06:08:44 2018
@@ -1207,7 +1207,7 @@ OPTIONS:
 
By default, the exit status of scan-build is the same as the executed build
command. Specifying this option causes the exit status of scan-build to be 1
-   if it found potential bugs and 0 otherwise.
+   if it found potential bugs and the exit status of the build itself 
otherwise.
 
  --exclude 
 
@@ -1908,7 +1908,7 @@ if (defined $Options{OutputFormat}) {
 
 if ($Options{ExitStatusFoundBugs}) {
   exit 1 if ($NumBugs > 0);
-  exit 0;
+  exit $ExitStatus;
 }
   }
 }


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

[PATCH] D52530: [analyzer] scan-build: if --status-bugs is passed, don't forget about the exit status of the actual build

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D52530#1246459, @jroelofs wrote:

> LGTM


Thank you for the speedy review!


Repository:
  rC Clang

https://reviews.llvm.org/D52530



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

[PATCH] D52530: [analyzer] scan-build: if --status-bugs is passed, don't forget about the exit status of the actual build

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC343105: [analyzer] scan-build: if --status-bugs is passed, 
don't forget about the exit… (authored by lebedevri, committed by ).

Repository:
  rC Clang

https://reviews.llvm.org/D52530

Files:
  tools/scan-build/bin/scan-build


Index: tools/scan-build/bin/scan-build
===
--- tools/scan-build/bin/scan-build
+++ tools/scan-build/bin/scan-build
@@ -1207,7 +1207,7 @@
 
By default, the exit status of scan-build is the same as the executed build
command. Specifying this option causes the exit status of scan-build to be 1
-   if it found potential bugs and 0 otherwise.
+   if it found potential bugs and the exit status of the build itself 
otherwise.
 
  --exclude 
 
@@ -1908,7 +1908,7 @@
 
 if ($Options{ExitStatusFoundBugs}) {
   exit 1 if ($NumBugs > 0);
-  exit 0;
+  exit $ExitStatus;
 }
   }
 }


Index: tools/scan-build/bin/scan-build
===
--- tools/scan-build/bin/scan-build
+++ tools/scan-build/bin/scan-build
@@ -1207,7 +1207,7 @@
 
By default, the exit status of scan-build is the same as the executed build
command. Specifying this option causes the exit status of scan-build to be 1
-   if it found potential bugs and 0 otherwise.
+   if it found potential bugs and the exit status of the build itself otherwise.
 
  --exclude 
 
@@ -1908,7 +1908,7 @@
 
 if ($Options{ExitStatusFoundBugs}) {
   exit 1 if ($NumBugs > 0);
-  exit 0;
+  exit $ExitStatus;
 }
   }
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52533: [test] Use --sysroot instead of -B in print-multi-directory.c

[Christian Bruel via Phabricator via cfe-commits]

chrib added a reviewer: jroelofs.
chrib added a comment.

Hi Martin,

maybe just a NIT, use --sysroot= rather than just --sysroot for consistency 
with other tests.

Otherwise looks good to me, thanks (adding Jonathan as I'm not sure I can 
accept)


Repository:
  rC Clang

https://reviews.llvm.org/D52533



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

[PATCH] D52547: Tell whether file/folder for include completions.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52547

Files:
  clangd/CodeComplete.cpp
  clangd/Protocol.h
  unittests/clangd/CodeCompleteTests.cpp


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -2073,6 +2073,27 @@
   }
 }
 
+TEST(CompletionTest, IncludedCompletionKinds) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  std::string Subdir = testPath("sub");
+  std::string SearchDirArg = (llvm::Twine("-I") + Subdir).str();
+  CDB.ExtraClangFlags = {SearchDirArg.c_str()};
+  std::string BarHeader = testPath("sub/bar.h");
+  FS.Files[BarHeader] = "";
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+  auto Results = completions(Server,
+  R"cpp(
+#include "^"
+  )cpp"
+  );
+  EXPECT_THAT(Results.Completions,
+  AllOf(Has("sub/", CompletionItemKind::Folder),
+Has("bar.h\"", CompletionItemKind::File)));
+}
+
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -704,6 +704,7 @@
   Color = 16,
   File = 17,
   Reference = 18,
+  Folder = 19,
 };
 
 /// Defines whether the insert text in a completion item should be interpreted
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -349,6 +349,9 @@
   }
   Completion.Kind = toCompletionItemKind(
   C.SemaResult->Kind, C.SemaResult->Declaration, ContextKind);
+  if (Completion.Kind == CompletionItemKind::File &&
+  Completion.Name.back() == '/')
+Completion.Kind = CompletionItemKind::Folder;
   for (const auto &FixIt : C.SemaResult->FixIts) {
 Completion.FixIts.push_back(
 toTextEdit(FixIt, ASTCtx.getSourceManager(), 
ASTCtx.getLangOpts()));


Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -2073,6 +2073,27 @@
   }
 }
 
+TEST(CompletionTest, IncludedCompletionKinds) {
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  std::string Subdir = testPath("sub");
+  std::string SearchDirArg = (llvm::Twine("-I") + Subdir).str();
+  CDB.ExtraClangFlags = {SearchDirArg.c_str()};
+  std::string BarHeader = testPath("sub/bar.h");
+  FS.Files[BarHeader] = "";
+  IgnoreDiagnostics DiagConsumer;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+  auto Results = completions(Server,
+  R"cpp(
+#include "^"
+  )cpp"
+  );
+  EXPECT_THAT(Results.Completions,
+  AllOf(Has("sub/", CompletionItemKind::Folder),
+Has("bar.h\"", CompletionItemKind::File)));
+}
+
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -704,6 +704,7 @@
   Color = 16,
   File = 17,
   Reference = 18,
+  Folder = 19,
 };
 
 /// Defines whether the insert text in a completion item should be interpreted
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -349,6 +349,9 @@
   }
   Completion.Kind = toCompletionItemKind(
   C.SemaResult->Kind, C.SemaResult->Declaration, ContextKind);
+  if (Completion.Kind == CompletionItemKind::File &&
+  Completion.Name.back() == '/')
+Completion.Kind = CompletionItemKind::Folder;
   for (const auto &FixIt : C.SemaResult->FixIts) {
 Completion.FixIts.push_back(
 toTextEdit(FixIt, ASTCtx.getSourceManager(), ASTCtx.getLangOpts()));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51340: Add /Zc:DllexportInlines option to clang-cl

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

The static local stuff still makes me nervous. How much does Chromium break if 
we just ignore the problem? And how does -fvisibility-inlines-hidden handle the 
issue?

Also, Sema already has a check for static locals in C inline functions:

  $ echo "inline void f() { static int x; }" | bin/clang -c -x c -
  :1:19: warning: non-constant static local variable in inline function 
may be different in different files [-Wstatic-local-in-inline]
  inline void f() { static int x; }
^

could we reuse that check somehow for this case with static locals in dllexport 
inline methods?




Comment at: clang/lib/Sema/SemaDecl.cpp:12624
+  isa(D)) {
+CXXMethodDecl *MD = dyn_cast(D);
+CXXRecordDecl *Class = MD->getParent();

Hmm, now we're adding an AST walk over all inline methods which probably slows 
us down a bit. Not sure I have any better ideas though.

In any case, ActOnFinishInlineFunctionDef needs a comment explaining why it's 
doing this.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5702
+!getLangOpts().DllExportInlines &&
+Class->getTemplateSpecializationKind() != 
TSK_ExplicitInstantiationDeclaration &&
+Class->getTemplateSpecializationKind() != 
TSK_ExplicitInstantiationDefinition) {

What worries me is that now we have logic in two different places about 
inheriting the dll attribute.

The new place in ActOnFinishInlineFunctionDef doesn't have the same checks 
(checking for MS ABI and the template specialization kind) which means the 
logic for when to inherit is already a little out of sync...


https://reviews.llvm.org/D51340



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

[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 167124.
ioeric marked 7 inline comments as done.
ioeric added a comment.

- address review comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52419

Files:
  clangd/CMakeLists.txt
  clangd/ClangdServer.cpp
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/FS.cpp
  clangd/FS.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/ClangdTests.cpp
  unittests/clangd/FSTests.cpp
  unittests/clangd/TestFS.cpp

Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -23,6 +23,7 @@
 llvm::StringMap const &Timestamps) {
   IntrusiveRefCntPtr MemFS(
   new vfs::InMemoryFileSystem);
+  MemFS->setCurrentWorkingDirectory(testRoot());
   for (auto &FileAndContents : Files) {
 StringRef File = FileAndContents.first();
 MemFS->addFile(
Index: unittests/clangd/FSTests.cpp
===
--- /dev/null
+++ unittests/clangd/FSTests.cpp
@@ -0,0 +1,46 @@
+//===-- FSTests.cpp - File system related tests -*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "FS.h"
+#include "TestFS.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+TEST(FSTests, PreambleStatusCache) {
+  llvm::StringMap Files;
+  Files["x"] = "";
+  Files["y"] = "";
+  auto FS = buildTestFS(Files);
+  FS->setCurrentWorkingDirectory(testRoot());
+
+  PreambleFileStatusCache StatCache;
+  auto ProduceFS = StatCache.getProducingFS(FS);
+  EXPECT_TRUE(ProduceFS->openFileForRead("x"));
+  EXPECT_TRUE(ProduceFS->status("y"));
+
+  EXPECT_TRUE(StatCache.lookup(testPath("x")).hasValue());
+  EXPECT_TRUE(StatCache.lookup(testPath("y")).hasValue());
+
+  vfs::Status S("fake", llvm::sys::fs::UniqueID(0, 0),
+std::chrono::system_clock::now(), 0, 0, 1024,
+llvm::sys::fs::file_type::regular_file, llvm::sys::fs::all_all);
+  StatCache.update(*FS, S);
+  auto ConsumeFS = StatCache.getConsumingFS(FS);
+  auto Cached = ConsumeFS->status(testPath("fake"));
+  EXPECT_TRUE(Cached);
+  EXPECT_EQ(Cached->getName(), S.getName());
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -963,6 +963,71 @@
Field(&CodeCompletion::Name, "baz")));
 }
 
+// Check that running code completion doesn't stat() a bunch of files from the
+// preamble again. (They should be using the preamble's stat-cache)
+TEST(ClangdTests, PreambleVFSStatCache) {
+  class ListenStatsFSProvider : public FileSystemProvider {
+  public:
+ListenStatsFSProvider(llvm::StringMap &CountStats)
+: CountStats(CountStats) {}
+
+IntrusiveRefCntPtr getFileSystem() override {
+  class ListenStatVFS : public vfs::ProxyFileSystem {
+  public:
+ListenStatVFS(IntrusiveRefCntPtr FS,
+  llvm::StringMap &CountStats)
+: ProxyFileSystem(std::move(FS)), CountStats(CountStats) {}
+
+llvm::ErrorOr>
+openFileForRead(const Twine &Path) override {
+  ++CountStats[llvm::sys::path::filename(Path.str())];
+  return ProxyFileSystem::openFileForRead(Path);
+}
+llvm::ErrorOr status(const Twine &Path) override {
+  ++CountStats[llvm::sys::path::filename(Path.str())];
+  return ProxyFileSystem::status(Path);
+}
+
+  private:
+llvm::StringMap &CountStats;
+  };
+
+  return IntrusiveRefCntPtr(
+  new ListenStatVFS(buildTestFS(Files), CountStats));
+}
+
+// If relative paths are used, they are resolved with testPath().
+llvm::StringMap Files;
+llvm::StringMap &CountStats;
+  };
+
+  llvm::StringMap CountStats;
+  ListenStatsFSProvider FS(CountStats);
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto SourcePath = testPath("foo.cpp");
+  auto HeaderPath = testPath("foo.h");
+  FS.Files[HeaderPath] = "struct TestSym {};";
+  Annotations Code(R"cpp(
+#include "foo.h"
+
+int main() {
+  TestSy^
+})cpp");
+
+  runAddDocument(Server, SourcePath, Code.code());
+
+  EXPECT_EQ(CountStats["foo.h"], 1u);
+  auto Completions = cantFail(runCodeComplete(Server, SourcePath, Code.point(),
+  clangd::CodeCompleteOptions()))
+ .Com

[PATCH] D52419: [clangd] Cache FS stat() calls when building preamble.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/ClangdUnit.cpp:339
+llvm::ErrorOr status(const Twine &Path) override {
+  auto I = StatCache.find(Path.str());
+  return (I != StatCache.end()) ? I->getValue() : FS->status(Path);

ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > We store absolute paths, but Path can be relative here.
> > This is because preamble stores absolute paths. `Path` should be an path 
> > from preamble. Added documentation.
> It's true that preamble stores and checks absolute paths, however clang can 
> later access the same file using a relative path.
> Calling makeAbsolute here shouldn't hurt and would obviously make it work in 
> both cases.
It would "obviously work"... until you have symlinks and tricks to handle 
symlinks ;)



Comment at: clangd/CodeComplete.cpp:1028
 
+  IntrusiveRefCntPtr VFS = Input.VFS;
+  if (Input.PreambleStatCache)

ilya-biryukov wrote:
> sammccall wrote:
> > ioeric wrote:
> > > ilya-biryukov wrote:
> > > > It feels like we could apply this caching one level higher, on the 
> > > > TUScheduler level when creating the filesystem. It makes sense not only 
> > > > for code completion, but also for all other features, including 
> > > > rebuilds of ASTs that were washed out of the cache.
> > > > WDYT?
> > > It sounds like a reasonable thing to do. I took a quick look, and it 
> > > seemed that this would probably require some refactoring/redesign on 
> > > TUScheduler - it doesn't know about the VFS?
> > > 
> > > I think it shouldn't be hard to do this case by case. For example, code 
> > > completion and signature help will be covered in this patch. And it 
> > > should be pretty easy to make AST rebuild use the cache as well?
> > > It sounds like a reasonable thing to do. I took a quick look, and it 
> > > seemed that this would probably require some refactoring/redesign on 
> > > TUScheduler - it doesn't know about the VFS?
> > 
> > Right, this is a bit of a mess...
> > Currently there are features that access the FS "directly". This includes 
> > CodeComplete which runs its own compile action, as well as things like 
> > switchSourceHeader or format. These invoke FSProvider.
> > Then there are features that build an AST (which may need file accesses), 
> > and then may implicitly read files by querying the AST (the FS is attached 
> > to the FileManager or something). These use the FS obtained from the 
> > FSProvider **in the relevant addDocument call**.
> > There's no fundamental reason we can't have both (access FS directly and 
> > use the AST) in which case we'll be using both filesystems together...
> > 
> > In the near term, making the TUScheduler-managed AST rebuild use the cache 
> > stored in the preamble seems like a nice win. (As you alluded to I don't 
> > think this change is in TUScheduler itself, rather buildAST?)
> My idea would be to create and pass the updated **cached** FS into both 
> `buildAST` and `codeComplete` higher up the call chain. This would allow 
> localizing the code that creates caching VFSes in one file (TUScheduler.h or 
> similar).
> 
> The advantage of receiving the patched-up FS in `buildAST` and `codeComplete` 
> is that they don't have to care about this trick, making the implementation 
> simpler.
> The fewer functions across the codebase have to apply the trick the better, 
> e.g. this would make sure we won't accidentally forget it to apply it when 
> implementing a new feature.
Added the cache support to `buildAST` as we thinks it's beneficial (haven't 
profiled this though).

Currently, the trick is applied in two places (`semaCodeComplete` in 
CodeComplete and `buildAST` in ClangdUnit), and I'm not sure how much this 
would grow in the near future. It's also unclear to me whether baking this into 
`TUScheduler` would be less work in the long run. I would suggest revisiting 
this when we have more evidence.



Comment at: clangd/FS.cpp:29
+PreambleFileStatusCache::lookup(llvm::StringRef File) const {
+  auto I = StatCache.find(File);
+  if (I != StatCache.end())

sammccall wrote:
> lock
After a second thought, I'm wondering if the mutex is necessary, if the cache 
is only updated during preamble build in a single thread. The cache would also 
remain the same in preamble reuses.



Comment at: clangd/FS.cpp:48
+return File;
+  if (auto S = File->get()->status())
+StatCache.update(getUnderlyingFS(), std::move(*S));

sammccall wrote:
> I'm not sure I get this: AFAICT (at least on linux) the status is never 
> available on a newly opened file, so this will always be a stat() call, so 
> we're just doing the work eagerly and caching it for later. Isn't this just 
> moving the work around?
> 
> I'm sure you've verified this is important somehow, but a comment explaining 
> how would be much appreciated :-)
Files opened via `openFileFo

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:1038
+
+  SmallString<128> NewAddNullTermExprStr;
+  NewAddNullTermExprStr = "\n";

whisperity wrote:
> aaron.ballman wrote:
> > This should be done using a `Twine`.
> Can you please give an example of how to well-approach this? We had issues 
> with using Twines on the stack and then later on running into weird undefined 
> behaviour. The documentation is not clear to a lot of our colleagues on where 
> the `Twine`'s usage barriers are... (Asking this not just for @Charusso but 
> also for a few more colleagues, @baloghadamsoftware e.g.)
naive approach:

```
std::string New = 
Twine("\n").concat(SpaceBeforeStmtStr).concat(exprToStr(..))...).str();
```
I think retrieving a `SmallString` is not supported. Please note, that `Twine` 
does support taking integer and floats directly without prior conversion.
You can use `operator+` instead of `concat` too.

Did this help or did you have more specific issues? 


https://reviews.llvm.org/D45050



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

[PATCH] D52334: [clang-tidy] Build it even without static analyzer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Please wait for explicit approval from @alexfh

Am 26.09.2018 um 13:05 schrieb Stephen Kelly via Phabricator:

> steveire added a comment.
> 
> Can I go ahead and merge this now?
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D52334


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52334



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

[PATCH] D52421: [Sema] Diagnose parameter names that shadow inherited field names

[Roman Lebedev via Phabricator via cfe-commits]

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

This looks good to me, but you probably want to wait for someone else to review 
this, too.


https://reviews.llvm.org/D52421



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

[PATCH] D52549: [VFS] Add a VFS wrapper which avoids opening nonexistent files by reading dirs.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added reviewers: ioeric, ilya-biryukov.
Herald added subscribers: cfe-commits, jfb, mgorny.

This is intended to reduce the number of syscalls when building large
translation units that have long include paths.

This is a WIP for early feedback:

- obviously it needs tests
- it will not be on by default
- the logging and extra instrumentation is temporary to help me tune it

Particularly interested in how to express this without hundreds of
nested switch statements.


Repository:
  rC Clang

https://reviews.llvm.org/D52549

Files:
  include/clang/Basic/VirtualFileSystem.h
  lib/Basic/AvoidStatsVFS.cpp
  lib/Basic/CMakeLists.txt
  lib/Basic/VirtualFileSystem.cpp

Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -303,11 +303,6 @@
   return llvm::sys::fs::real_path(Path, Output);
 }
 
-IntrusiveRefCntPtr vfs::getRealFileSystem() {
-  static IntrusiveRefCntPtr FS = new RealFileSystem();
-  return FS;
-}
-
 namespace {
 
 class RealFSDirIter : public clang::vfs::detail::DirIterImpl {
@@ -2141,3 +2136,289 @@
 
   return *this;
 }
+
+namespace {
+class StatFS : public FileSystem {
+public:
+  StatFS(const char *Name, llvm::IntrusiveRefCntPtr FS)
+  : Name(Name), FS(std::move(FS)) {
+llvm::errs() << "created statfs " << Name << "\n";
+  }
+
+  llvm::ErrorOr status(const Twine &Path) override {
+auto Ret = FS->status(Path);
+bump(Ret ? StatusOK : StatusErr);
+return Ret;
+  }
+
+  llvm::ErrorOr>
+  openFileForRead(const Twine &Path) override {
+auto Ret = FS->openFileForRead(Path);
+bump(Ret ? OpenOK : OpenErr);
+return Ret;
+  }
+
+  directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override {
+bump(DirBegin);
+return FS->dir_begin(Dir, EC);
+  }
+
+  virtual std::error_code setCurrentWorkingDirectory(const Twine &Path) override {
+bump(SetCWD);
+return FS->setCurrentWorkingDirectory(Path);
+  }
+
+  virtual llvm::ErrorOr getCurrentWorkingDirectory() const override {
+bump(GetCWD);
+return FS->getCurrentWorkingDirectory();
+  }
+
+  virtual std::error_code
+  getRealPath(const Twine &Path, SmallVectorImpl &Output) const override {
+bump(GetRealPath);
+return FS->getRealPath(Path, Output);
+  }
+
+private:
+  void bump(std::atomic &I) const {
+++I;
+if (++All % 1 == 0) {
+  llvm::errs() << "== FILESYSTEM " << Name << " ==\n"
+   << "Status: " << StatusOK << "+" << StatusErr << "\n"
+   << "Open: " << OpenOK << "+" << OpenErr << "\n"
+   << "Dir: " << DirBegin << "\n"
+   << "GetRealPath: " << GetRealPath << "\n"
+   << "===\n";
+}
+  }
+
+  const char* Name;
+  llvm::IntrusiveRefCntPtr FS;
+  mutable std::atomic StatusOK = {0}, StatusErr = {0}, OpenOK = {0},
+OpenErr = {0}, DirBegin = {0}, GetRealPath = {0},
+SetCWD = {0}, GetCWD = {0};
+  mutable std::atomic All= {0};
+};
+
+#if 0
+class StatLessFS : public FileSystem {
+  enum State : uint8_t {
+NoInfo, // Initial state.
+DirKnownChildren,   // Directory, all children's existence is in the cache.
+DirTooBig,  // Directory, known to be too big to enumerate.
+DirUnknownChildren, // Directory, but children are not listed.
+File,   // Known to be a file.
+Missing,// Known to not exist.
+MissingOrDir,   // May be a directory, or may not exist.
+MissingOrFile   // May be a file, or may not exist.
+  };
+  mutable llvm::StringMap Cache;
+  mutable std::mutex Mu;
+public:
+  StatLessFS(llvm::IntrusiveRefCntPtr FS) : FS(std::move(FS)) {
+llvm::errs() << "created statlessfs \n";
+  }
+
+  bool mayBeFile(StringRef Path, bool AllowDir) const {
+llvm::errs() << "Could " << Path << " be a file"
+ << (AllowDir ? " or dir" : "") << "?\n";
+auto Parent = llvm::sys::path::parent_path(Path);
+if (!Parent.empty())
+  fillCache(Parent);
+std::lock_guard Lock(Mu);
+switch (Cache.lookup(Path)) {
+case NoInfo:
+  break;
+case File:
+case MissingOrFile:
+  llvm::errs() << "Cache says maybe\n";
+  return true;
+case Missing:
+  llvm::errs() << "Cache says no\n";
+  return false;
+case DirKnownChildren:
+case DirUnknownChildren:
+case DirTooBig:
+case MissingOrDir:
+  return AllowDir;
+  llvm::errs() << "Cache says " << (AllowDir ? "maybedir" : "no,dir")
+   << "\n";
+  return false;
+}
+if (Parent.empty())
+  return true;
+// We don't know anything about the actual file. Consider the parent...
+llvm::errs() << "Cache says nothing. parent is " << int(Cache.lookup(Parent)) << "\n";
+return Cache.lookup(Parent) == DirTooBig;
+  }
+
+  // Populate

[PATCH] D52549: [VFS] Add a VFS wrapper which avoids opening nonexistent files by reading dirs.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 167126.
sammccall added a comment.

remove commented code


Repository:
  rC Clang

https://reviews.llvm.org/D52549

Files:
  include/clang/Basic/VirtualFileSystem.h
  lib/Basic/AvoidStatsVFS.cpp
  lib/Basic/CMakeLists.txt
  lib/Basic/VirtualFileSystem.cpp

Index: lib/Basic/VirtualFileSystem.cpp
===
--- lib/Basic/VirtualFileSystem.cpp
+++ lib/Basic/VirtualFileSystem.cpp
@@ -303,11 +303,6 @@
   return llvm::sys::fs::real_path(Path, Output);
 }
 
-IntrusiveRefCntPtr vfs::getRealFileSystem() {
-  static IntrusiveRefCntPtr FS = new RealFileSystem();
-  return FS;
-}
-
 namespace {
 
 class RealFSDirIter : public clang::vfs::detail::DirIterImpl {
@@ -2141,3 +2136,75 @@
 
   return *this;
 }
+
+namespace {
+// Log operation counts.
+class StatFS : public FileSystem {
+public:
+  StatFS(const char *Name, llvm::IntrusiveRefCntPtr FS)
+  : Name(Name), FS(std::move(FS)) {
+llvm::errs() << "created statfs " << Name << "\n";
+  }
+
+  llvm::ErrorOr status(const Twine &Path) override {
+auto Ret = FS->status(Path);
+bump(Ret ? StatusOK : StatusErr);
+return Ret;
+  }
+
+  llvm::ErrorOr>
+  openFileForRead(const Twine &Path) override {
+auto Ret = FS->openFileForRead(Path);
+bump(Ret ? OpenOK : OpenErr);
+return Ret;
+  }
+
+  directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override {
+bump(DirBegin);
+return FS->dir_begin(Dir, EC);
+  }
+
+  virtual std::error_code setCurrentWorkingDirectory(const Twine &Path) override {
+bump(SetCWD);
+return FS->setCurrentWorkingDirectory(Path);
+  }
+
+  virtual llvm::ErrorOr getCurrentWorkingDirectory() const override {
+bump(GetCWD);
+return FS->getCurrentWorkingDirectory();
+  }
+
+  virtual std::error_code
+  getRealPath(const Twine &Path, SmallVectorImpl &Output) const override {
+bump(GetRealPath);
+return FS->getRealPath(Path, Output);
+  }
+
+private:
+  void bump(std::atomic &I) const {
+++I;
+if (++All % 1 == 0) {
+  llvm::errs() << "== FILESYSTEM " << Name << " ==\n"
+   << "Status: " << StatusOK << "+" << StatusErr << "\n"
+   << "Open: " << OpenOK << "+" << OpenErr << "\n"
+   << "Dir: " << DirBegin << "\n"
+   << "GetRealPath: " << GetRealPath << "\n"
+   << "===\n";
+}
+  }
+
+  const char* Name;
+  llvm::IntrusiveRefCntPtr FS;
+  mutable std::atomic StatusOK = {0}, StatusErr = {0}, OpenOK = {0},
+OpenErr = {0}, DirBegin = {0}, GetRealPath = {0},
+SetCWD = {0}, GetCWD = {0};
+  mutable std::atomic All= {0};
+};
+
+} // namespace
+
+IntrusiveRefCntPtr vfs::getRealFileSystem() {
+  static IntrusiveRefCntPtr FS = new StatFS(
+  "outer", avoidStats(new StatFS("Real", new RealFileSystem())).release());
+  return FS;
+}
Index: lib/Basic/CMakeLists.txt
===
--- lib/Basic/CMakeLists.txt
+++ lib/Basic/CMakeLists.txt
@@ -46,6 +46,7 @@
 
 add_clang_library(clangBasic
   Attributes.cpp
+  AvoidStatsVFS.cpp
   Builtins.cpp
   CharInfo.cpp
   Cuda.cpp
Index: lib/Basic/AvoidStatsVFS.cpp
===
--- /dev/null
+++ lib/Basic/AvoidStatsVFS.cpp
@@ -0,0 +1,296 @@
+//===- AvoidStatsVFS.cpp - Implements a stat-reducing VFS wrapper -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// We implement a VFS wrapper that uses directory listings to prune status() and
+// open() operations for files that *do not exist*.
+//
+// These operations are common in clang because of include paths.
+// With an include path of {A, B, ...}, an #include  directive results
+// in attempts to open or stat {A/foo/bar, B/foo/bar, ...}.
+//
+// This is expensive because this typically takes one syscall per path, so we
+// have O(NumIncludes * IncludePathLen) syscalls.
+//
+// To optimize this, we can list parent directories such as A/.
+// If A only contains {config.h}, attempts to open A/foo/bar, A/foo/baz, A/qux
+// can all fail instantly.
+// Listing a directory tends to be a single syscall, and in practice most
+// missing files can be recognized by looking at the same few directories.
+// In practice the number of syscalls is O(NumIncludes + IncludePathLen).
+//
+// Our constant factor is higher, but this improves performance for large TUs
+// with many include paths.
+//
+//===--===//
+
+#include "clang/Basic/VirtualFileSystem.h"
+#include "llvm/ADT/StringSet.h"
+#include "llvm/Support/Path.h"
+#include 
+
+namespace clang {
+namespace v

[PATCH] D52549: [VFS] Add a VFS wrapper which avoids opening nonexistent files by reading dirs.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

As noted, this is really early stage, but would appreciate design feedback (I 
think both of you have spent some time fighting VFS by now...)


Repository:
  rC Clang

https://reviews.llvm.org/D52549



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

[PATCH] D52503: [clangd] Fix bugs with incorrect memory estimate report

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:251
   Bytes += InvertedIndex.getMemorySize();
-  for (const auto &P : InvertedIndex)
-Bytes += P.second.bytes();
+  for (const auto &TokenToPostingList : InvertedIndex)
+Bytes += TokenToPostingList.first.Data.size() +

ioeric wrote:
> kbobyrev wrote:
> > ioeric wrote:
> > > kbobyrev wrote:
> > > > ioeric wrote:
> > > > > Would `InvertedIndex.getMemorySize()` be a better estimate?
> > > > IIUC this is only precise for the objects which do not make any 
> > > > allocations (e.g. `std::vector` and `std::string` memory estimate would 
> > > > not be "correct").
> > > > 
> > > > From the documentation
> > > > 
> > > > > This is just the raw memory used by DenseMap. If entries are pointers 
> > > > > to objects, the size of the referenced objects are not included.
> > > > 
> > > > I also have `Bytes += InvertedIndex.getMemorySize();` above, so the 
> > > > purpose of this code would be to take into account the size of these 
> > > > "reference objects".
> > > > 
> > > > However, since `PostingList::bytes()` also returns underlying 
> > > > `std::vector` size, this code will probably add these `std::vector` 
> > > > objects size twice (the first one was in 
> > > > `InvertedIndex.getMemorySize()`). I should fix that.
> > > > `If entries are pointers to objects, the size of the referenced objects 
> > > > are not included.`
> > > This applies to *pointers* to objects, but the `PostingList`s in 
> > > InvertedList are not pointerd? So it seems to me that 
> > > `InvertedIndex.getMemorySize()` covers everything in the `InvertedIndex`. 
> > > One way to verify is probably check the size calculated with loop against 
> > > `InvertedIndex.getMemorySize()`.
> > As discussed offline, pointers and references are an example of objects 
> > which would be incorrectly measured by `DesneMap::getMemorySize()`, but 
> > `std::vector` and `std::string` are pointers in a way because they also 
> > just store a pointer to the underlying storage which is hidden from 
> > `DenseMap::getMemorySize()`; thus, it would be more precise to use the 
> > proposed scheme for memory estimation.
> I see. I was expecting DenseMap to maintain an arena which could provide 
> memory usage of all owned data, but it seems to only consider 
> `sizeof(KV-Pair)`...
> 
> If the posting lists (vectors) are not covered, shouldn't we also also add 
> the size of tokens (string) here?
Yes, I was also adding `TokenToPostingList.first.Data.size()` to the size 
estimate in the previous revision snapshot, but as @sammccall pointed out, 
there are multiple string implementations and one of them (IIUC this one is 
currently pushed in the C++ Standard) utilizes Small Object Optimization (i.e. 
it behaves like `llvm::SmallVector` rather than `std::vector`). Therefore, it 
does not perform external allocations before some limit is reached. I don't 
know whether it is possible to know whether the external allocation happened or 
not and the summary of our discussion with Sam was that we should just omit 
this.


https://reviews.llvm.org/D52503



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

[PATCH] D52360: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D52360#1246474, @baloghadamsoftware wrote:

> In https://reviews.llvm.org/D52360#1246452, @lebedev.ri wrote:
>
> > I disagree, it **must** not have false-negatives by default.
>
>
> What kind of false-negatives could be caused by smart pointers or references? 
> Sane people do not name a type pointer or reference if they are not. Such 
> types must never be passed by reference. Few people will use the check if 
> they have to set tons of options for the basic expected behavior. For example 
> in `CodeChecker` this check is disabled by default and is only enabled in the 
> `Sensitive` profile.


Ok, so let's entertain the current patch.
Now, some time passes, and someone either discovers that the check does not 
warn on some code that one would have thought it should warn.
Shitty code happens. Not everyone is sane in their naming choices. More 
importantly, there isn't a common coding style naming guidelines everyone **has 
to** follow.
Someone somewhere will surely hit this. When he does, how does one get the 
check to warn on that?
Or other side of the coin, how does one get the check //not// to warn on some 
custom type that does not match those hardcoded regexes?

I don't disagree that this has false-positives. But this is **really** 
sensitive to the type names. It is not a good idea to hardcode this.
//Please// follow pre-existing practice of //configurable// type 
white/blacklists.

  $ clang-tidy -checks=\* -dump-config | grep -i "types$" -A1
- key: cert-msc32-c.DisallowedSeedTypes
  value:   'time_t,std::time_t'
- key: cert-msc51-cpp.DisallowedSeedTypes
  value:   'time_t,std::time_t'
  --
- key: google-runtime-references.WhiteListTypes
  value:   ''
  --
- key: hicpp-use-emplace.TupleTypes
  value:   '::std::pair;::std::tuple'
  --
- key: modernize-use-emplace.TupleTypes
  value:   '::std::pair;::std::tuple'
  --
- key: readability-simplify-subscript-expr.Types
  value:   
'::std::basic_string;::std::basic_string_view;::std::vector;::std::array'


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52360



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

[PATCH] D52315: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D52315#1245747, @dblaikie wrote:

> ...


I think this has been attempted to be superseded by 
https://reviews.llvm.org/D52360, although that has the same basic issue.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52315



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

[PATCH] D52360: [clang-tidy] Fix for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

[Felix Berger via Phabricator via cfe-commits]

flx added a comment.

In https://reviews.llvm.org/D52360#1246472, @baloghadamsoftware wrote:

> In https://reviews.llvm.org/D52360#1246443, @danilaml wrote:
>
> > Would that also skip checks for something like `shared_ptr`?
>
>
> Yes, everything ending on `pointer`, `ptr`, `reference` or `ref`, first 
> letter case insensitive.


std::shared_ptr should not be blacklisted. It is not free to copy it: It incurs 
two atomic operations and two branches.

Users should blacklist it if they know they don't care about this or not use 
the check.

While it looks weird for the check to suggest to pass std::shared_ptr by 
reference it is correct. A better change would be to just pass the raw pointer 
in this case:

std::shared_ptr p;
PassByRawPointer(p.get());

But this would require function signature change that breaks callers outside of 
the translation unit.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52360



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

[PATCH] D52286: [Intrinsic] Signed Saturation Intirnsic

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1119
+  case ISD::SSAT:
+// Target legalization checked here?
+Action = TargetLowering::Expand;

leonardchan wrote:
> ebevhan wrote:
> > leonardchan wrote:
> > > ebevhan wrote:
> > > > leonardchan wrote:
> > > > > @ebevhan Do you know if this is where checking if this intrinsic is 
> > > > > supported by the target should be? 
> > > > Yes, this would be it. For nodes like ssat and fixed-point ops, you 
> > > > could ask a custom target hook, maybe 
> > > > `TLI.getScaledOperationAction(, , )` to 
> > > > determine the legality of a given node.
> > > > 
> > > > When type-legalizing you also need to handle things for specific nodes 
> > > > as well, since things behave differently when you legalize 
> > > > operands/results on different nodes.
> > > > 
> > > > (Of course, this all depends on whether we do the legalization here or 
> > > > in IR instead.)
> > > Made the default action `Expand` for this in `TargetLoweringBase` and 
> > > targets can override this themselves.
> > > 
> > > I think ssat may not need a custom hook since I don't think it requires 
> > > anything extra like fixsmul if it has to be expanded. But I see what you 
> > > mean now by how different nodes may require different steps for 
> > > legalization.
> > You don't need an extra hook to determine anything extra for expansion, but 
> > you do need the hook to determine if it's legal in the first place. As I 
> > mentioned in another comment, an i16 ssat with a saturating width of 8 
> > might be legal, but an i16 ssat with saturating width 12 might not. The 
> > former must not be expanded, but the latter must.
> Got it. Just to clarify though, legalizations regarding the scale would only 
> apply for our mul/div intrinsics right? Operations like saturation and sat 
> add/sub which don't care about the scale do not need this hook right, since 
> it would also be useful for these operations to work on regular integers?
Saturating add/sub won't need anything, but I still think saturation requires 
some extra check to determine if an operation with a specific saturating 
bitwidth should be legal or expanded. As I mentioned, a saturation with a type 
might be legal for one saturating bitwidth but not for another.


Repository:
  rL LLVM

https://reviews.llvm.org/D52286



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

r343111 - [ARM/AArch64][v8.5A] Add Armv8.5-A target

[Oliver Stannard via cfe-commits]

Author: olista01
Date: Wed Sep 26 07:20:29 2018
New Revision: 343111

URL: http://llvm.org/viewvc/llvm-project?rev=343111&view=rev
Log:
[ARM/AArch64][v8.5A] Add Armv8.5-A target

This patch allows targetting Armv8.5-A from Clang. Most of the
implementation is in TargetParser, so this is mostly just adding tests.

Patch by Pablo Barrio!

Differential revision: https://reviews.llvm.org/D52491


Modified:
cfe/trunk/lib/Basic/Targets/ARM.cpp
cfe/trunk/test/Driver/aarch64-cpus.c
cfe/trunk/test/Driver/arm-cortex-cpus.c
cfe/trunk/test/Preprocessor/arm-target-features.c

Modified: cfe/trunk/lib/Basic/Targets/ARM.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/ARM.cpp?rev=343111&r1=343110&r2=343111&view=diff
==
--- cfe/trunk/lib/Basic/Targets/ARM.cpp (original)
+++ cfe/trunk/lib/Basic/Targets/ARM.cpp Wed Sep 26 07:20:29 2018
@@ -189,6 +189,8 @@ StringRef ARMTargetInfo::getCPUAttr() co
 return "8_3A";
   case llvm::ARM::ArchKind::ARMV8_4A:
 return "8_4A";
+  case llvm::ARM::ArchKind::ARMV8_5A:
+return "8_5A";
   case llvm::ARM::ArchKind::ARMV8MBaseline:
 return "8M_BASE";
   case llvm::ARM::ArchKind::ARMV8MMainline:

Modified: cfe/trunk/test/Driver/aarch64-cpus.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/aarch64-cpus.c?rev=343111&r1=343110&r2=343111&view=diff
==
--- cfe/trunk/test/Driver/aarch64-cpus.c (original)
+++ cfe/trunk/test/Driver/aarch64-cpus.c Wed Sep 26 07:20:29 2018
@@ -542,6 +542,25 @@
 // RUN: %clang -target aarch64 -march=armv8.4-a+nofp16+fp16fml -### -c %s 2>&1 
| FileCheck -check-prefix=GENERICV84A-NO-FP16-FP16FML %s
 // GENERICV84A-NO-FP16-FP16FML: "-target-feature" "+fp16fml" "-target-feature" 
"+fullfp16"
 
+// RUN: %clang -target aarch64 -march=armv8.5a -### -c %s 2>&1 | FileCheck 
-check-prefix=GENERICV85A %s
+// RUN: %clang -target aarch64 -march=armv8.5-a -### -c %s 2>&1 | FileCheck 
-check-prefix=GENERICV85A %s
+// RUN: %clang -target aarch64 -mlittle-endian -march=armv8.5a -### -c %s 2>&1 
| FileCheck -check-prefix=GENERICV85A %s
+// RUN: %clang -target aarch64 -mlittle-endian -march=armv8.5-a -### -c %s 
2>&1 | FileCheck -check-prefix=GENERICV85A %s
+// RUN: %clang -target aarch64_be -mlittle-endian -march=armv8.5a -### -c %s 
2>&1 | FileCheck -check-prefix=GENERICV85A %s
+// RUN: %clang -target aarch64_be -mlittle-endian -march=armv8.5-a -### -c %s 
2>&1 | FileCheck -check-prefix=GENERICV85A %s
+// GENERICV85A: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-cpu" "generic" 
"-target-feature" "+neon" "-target-feature" "+v8.5a"
+
+// RUN: %clang -target aarch64_be -march=armv8.5a -### -c %s 2>&1 | FileCheck 
-check-prefix=GENERICV85A-BE %s
+// RUN: %clang -target aarch64_be -march=armv8.5-a -### -c %s 2>&1 | FileCheck 
-check-prefix=GENERICV85A-BE %s
+// RUN: %clang -target aarch64 -mbig-endian -march=armv8.5a -### -c %s 2>&1 | 
FileCheck -check-prefix=GENERICV85A-BE %s
+// RUN: %clang -target aarch64 -mbig-endian -march=armv8.5-a -### -c %s 2>&1 | 
FileCheck -check-prefix=GENERICV85A-BE %s
+// RUN: %clang -target aarch64_be -mbig-endian -march=armv8.5a -### -c %s 2>&1 
| FileCheck -check-prefix=GENERICV85A-BE %s
+// RUN: %clang -target aarch64_be -mbig-endian -march=armv8.5-a -### -c %s 
2>&1 | FileCheck -check-prefix=GENERICV85A-BE %s
+// GENERICV85A-BE: "-cc1"{{.*}} "-triple" "aarch64_be{{.*}}" "-target-cpu" 
"generic" "-target-feature" "+neon" "-target-feature" "+v8.5a"
+
+// RUN: %clang -target aarch64 -march=armv8.5-a+fp16 -### -c %s 2>&1 | 
FileCheck -check-prefix=GENERICV85A-FP16 %s
+// GENERICV85A-FP16: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-cpu" 
"generic" "-target-feature" "+neon" "-target-feature" "+v8.5a" 
"-target-feature" "+fullfp16"
+
 // fullfp16 is off by default for v8a, feature must not be mentioned
 // RUN: %clang -target aarch64 -march=armv8a  -### -c %s 2>&1 | FileCheck 
-check-prefix=V82ANOFP16 -check-prefix=GENERIC %s
 // RUN: %clang -target aarch64 -march=armv8-a -### -c %s 2>&1 | FileCheck 
-check-prefix=V82ANOFP16 -check-prefix=GENERIC %s

Modified: cfe/trunk/test/Driver/arm-cortex-cpus.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/arm-cortex-cpus.c?rev=343111&r1=343110&r2=343111&view=diff
==
--- cfe/trunk/test/Driver/arm-cortex-cpus.c (original)
+++ cfe/trunk/test/Driver/arm-cortex-cpus.c Wed Sep 26 07:20:29 2018
@@ -318,6 +318,23 @@
 // RUN: %clang -target arm -march=armebv8.4-a -mbig-endian -### -c %s 2>&1 | 
FileCheck -check-prefix=CHECK-BE-V84A %s
 // CHECK-BE-V84A: "-cc1"{{.*}} "-triple" "armebv8.4{{.*}}" "-target-cpu" 
"generic"
 
+// RUN: %clang -target armv8.5a -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V85A %s
+// RUN: %clang -target arm -march=armv8.5a -### -c %s 2>&1 | FileCheck 
-check-prefix=CHECK-V85A %s
+// RUN

[PATCH] D52491: [ARM/AArch64][v8.5A] Add Armv8.5-A target

[Oliver Stannard via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC343111: [ARM/AArch64][v8.5A] Add Armv8.5-A target (authored 
by olista01, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52491?vs=166908&id=167130#toc

Repository:
  rC Clang

https://reviews.llvm.org/D52491

Files:
  lib/Basic/Targets/ARM.cpp
  test/Driver/aarch64-cpus.c
  test/Driver/arm-cortex-cpus.c
  test/Preprocessor/arm-target-features.c

Index: test/Driver/aarch64-cpus.c
===
--- test/Driver/aarch64-cpus.c
+++ test/Driver/aarch64-cpus.c
@@ -542,6 +542,25 @@
 // RUN: %clang -target aarch64 -march=armv8.4-a+nofp16+fp16fml -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV84A-NO-FP16-FP16FML %s
 // GENERICV84A-NO-FP16-FP16FML: "-target-feature" "+fp16fml" "-target-feature" "+fullfp16"
 
+// RUN: %clang -target aarch64 -march=armv8.5a -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV85A %s
+// RUN: %clang -target aarch64 -march=armv8.5-a -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV85A %s
+// RUN: %clang -target aarch64 -mlittle-endian -march=armv8.5a -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV85A %s
+// RUN: %clang -target aarch64 -mlittle-endian -march=armv8.5-a -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV85A %s
+// RUN: %clang -target aarch64_be -mlittle-endian -march=armv8.5a -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV85A %s
+// RUN: %clang -target aarch64_be -mlittle-endian -march=armv8.5-a -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV85A %s
+// GENERICV85A: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-cpu" "generic" "-target-feature" "+neon" "-target-feature" "+v8.5a"
+
+// RUN: %clang -target aarch64_be -march=armv8.5a -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV85A-BE %s
+// RUN: %clang -target aarch64_be -march=armv8.5-a -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV85A-BE %s
+// RUN: %clang -target aarch64 -mbig-endian -march=armv8.5a -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV85A-BE %s
+// RUN: %clang -target aarch64 -mbig-endian -march=armv8.5-a -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV85A-BE %s
+// RUN: %clang -target aarch64_be -mbig-endian -march=armv8.5a -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV85A-BE %s
+// RUN: %clang -target aarch64_be -mbig-endian -march=armv8.5-a -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV85A-BE %s
+// GENERICV85A-BE: "-cc1"{{.*}} "-triple" "aarch64_be{{.*}}" "-target-cpu" "generic" "-target-feature" "+neon" "-target-feature" "+v8.5a"
+
+// RUN: %clang -target aarch64 -march=armv8.5-a+fp16 -### -c %s 2>&1 | FileCheck -check-prefix=GENERICV85A-FP16 %s
+// GENERICV85A-FP16: "-cc1"{{.*}} "-triple" "aarch64{{.*}}" "-target-cpu" "generic" "-target-feature" "+neon" "-target-feature" "+v8.5a" "-target-feature" "+fullfp16"
+
 // fullfp16 is off by default for v8a, feature must not be mentioned
 // RUN: %clang -target aarch64 -march=armv8a  -### -c %s 2>&1 | FileCheck -check-prefix=V82ANOFP16 -check-prefix=GENERIC %s
 // RUN: %clang -target aarch64 -march=armv8-a -### -c %s 2>&1 | FileCheck -check-prefix=V82ANOFP16 -check-prefix=GENERIC %s
Index: test/Driver/arm-cortex-cpus.c
===
--- test/Driver/arm-cortex-cpus.c
+++ test/Driver/arm-cortex-cpus.c
@@ -318,6 +318,23 @@
 // RUN: %clang -target arm -march=armebv8.4-a -mbig-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V84A %s
 // CHECK-BE-V84A: "-cc1"{{.*}} "-triple" "armebv8.4{{.*}}" "-target-cpu" "generic"
 
+// RUN: %clang -target armv8.5a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V85A %s
+// RUN: %clang -target arm -march=armv8.5a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V85A %s
+// RUN: %clang -target arm -march=armv8.5-a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V85A %s
+// RUN: %clang -target arm -march=armv8.5a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V85A %s
+// RUN: %clang -target armv8.5a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V85A %s
+// RUN: %clang -target arm -march=armv8.5a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V85A %s
+// RUN: %clang -target arm -mlittle-endian -march=armv8.5-a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V85A %s
+// CHECK-V85A: "-cc1"{{.*}} "-triple" "armv8.5{{.*}}" "-target-cpu" "generic"
+
+// RUN: %clang -target armebv8.5a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V85A %s
+// RUN: %clang -target armv8.5a -mbig-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V85A %s
+// RUN: %clang -target armeb -march=armebv8.5a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V85A %s
+// RUN: %clang -target armeb -march=armebv8.5-a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V85A %s
+// RUN: %clang -target arm -march=armebv8.5a -mbig-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V85A %s
+// RUN: %clang -target arm -march=armebv8.5-a 

[PATCH] D52552: [clang-tidy] Flag Classes Inheriting From Structs

[Dennis Melamed via Phabricator via cfe-commits]

DennisMelamed created this revision.
DennisMelamed added reviewers: alexfh, aaron.ballman, hokein, JonasToth.
Herald added subscribers: xazax.hun, mgorny.

[clang-tidy] Flag Classes Inheriting From Structs

Added a check which flags cases of a class inheriting from a struct, which can 
cause unintended scope issues since struct members are public by default.

Patch by Dennis Melamed


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52552

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/ClassInheritFromStructCheck.cpp
  clang-tidy/misc/ClassInheritFromStructCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-class-inherit-from-struct.rst
  test/clang-tidy/misc-class-inherit-from-struct.cpp

Index: test/clang-tidy/misc-class-inherit-from-struct.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-class-inherit-from-struct.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s misc-class-inherit-from-struct %t
+
+struct A
+{
+int a;
+};
+
+class B:A
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: classes should not inherit from structs [misc-class-inherit-from-struct]
+{
+int b;
+};
+
+class C
+{
+int c;
+};
+
+class D:C
+{
+int d;
+};
Index: docs/clang-tidy/checks/misc-class-inherit-from-struct.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-class-inherit-from-struct.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - misc-class-inherit-from-struct
+
+misc-class-inherit-from-struct
+==
+
+Finds instances of classes inheriting from structs. Structs are meant for 
+storing data and are often used to maintain C compatibility. Having a class
+inherit from them can lead to confusion with what type of object is being 
+dealt with. Additionally, the default public nature of struct members can 
+lead to unintentional exposure of members.
+
+.. code-block:: c++
+  
+  struct A {};
+  class B: public A {}; //throws an error, members of A might be
+//unintentionally exposed
+  class C {};
+  class D: public C {}; //fine
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -159,6 +159,7 @@
llvm-include-order
llvm-namespace-comment
llvm-twine-local
+   misc-class-inherit-from-struct
misc-definitions-in-headers
misc-misplaced-const
misc-new-delete-overloads
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -106,6 +106,13 @@
   Detects usage of magic numbers, numbers that are used as literals instead of
   introduced via constants or symbols.
 
+- New :doc:`misc-class-inherit-from-struct
+  ` check.
+
+  Detects instances of classes inheriting from structs as that might lead to
+  unintended member access issues.
+
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/misc/MiscTidyModule.cpp
===
--- clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tidy/misc/MiscTidyModule.cpp
@@ -10,6 +10,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "ClassInheritFromStructCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "MisplacedConstCheck.h"
 #include "NewDeleteOverloadsCheck.h"
@@ -30,6 +31,8 @@
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+CheckFactories.registerCheck(
+"misc-class-inherit-from-struct");
 CheckFactories.registerCheck(
 "misc-definitions-in-headers");
 CheckFactories.registerCheck("misc-misplaced-const");
Index: clang-tidy/misc/ClassInheritFromStructCheck.h
===
--- /dev/null
+++ clang-tidy/misc/ClassInheritFromStructCheck.h
@@ -0,0 +1,35 @@
+//===--- ClassInheritFromStructCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CLASSINHERITFROMSTRUCTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CLASSINHERITFROMSTRUCTCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Matches non-implicit classes which inherit from structs
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-class-inherit-from-struct.html
+class ClassInheritFromSt

[PATCH] D52364: [clangd] Initial supoprt for cross-namespace global code completion.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 167135.
ioeric marked 14 inline comments as done.
ioeric added a comment.

- address review comments


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52364

Files:
  clangd/CodeComplete.cpp
  clangd/CodeComplete.h
  clangd/index/Index.h
  clangd/index/MemIndex.cpp
  clangd/index/dex/Dex.cpp
  clangd/tool/ClangdMain.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/DexTests.cpp

Index: unittests/clangd/DexTests.cpp
===
--- unittests/clangd/DexTests.cpp
+++ unittests/clangd/DexTests.cpp
@@ -554,6 +554,17 @@
   EXPECT_THAT(match(*I, Req), UnorderedElementsAre("a::y1"));
 }
 
+TEST(DexTest, WildcardScope) {
+  auto I =
+  Dex::build(generateSymbols({"a::y1", "a::b::y2", "c::y3"}), URISchemes);
+  FuzzyFindRequest Req;
+  Req.Query = "y";
+  Req.Scopes = {"a::"};
+  Req.AnyScope = true;
+  EXPECT_THAT(match(*I, Req),
+  UnorderedElementsAre("a::y1", "a::b::y2", "c::y3"));
+}
+
 TEST(DexTest, IgnoreCases) {
   auto I = Dex::build(generateSymbols({"ns::ABC", "ns::abc"}), URISchemes);
   FuzzyFindRequest Req;
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -2040,6 +2040,58 @@
   }
 }
 
+TEST(CompletionTest, NoAllScopesCompletionWhenQualified) {
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
+  auto Results = completions(
+  R"cpp(
+void f() { na::Clangd^ }
+  )cpp",
+  {cls("na::ClangdA"), cls("nx::ClangdX"), cls("Clangd3")}, Opts);
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(
+  AllOf(Qualifier(""), Scope("na::"), Named("ClangdA";
+}
+
+TEST(CompletionTest, AllScopesCompletion) {
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
+  auto Results = completions(
+  R"cpp(
+namespace na {
+void f() { Clangd^ }
+}
+  )cpp",
+  {cls("nx::Clangd1"), cls("ny::Clangd2"), cls("Clangd3"),
+   cls("na::nb::Clangd4")},
+  Opts);
+  EXPECT_THAT(
+  Results.Completions,
+  UnorderedElementsAre(AllOf(Qualifier("nx::"), Named("Clangd1")),
+   AllOf(Qualifier("ny::"), Named("Clangd2")),
+   AllOf(Qualifier(""), Scope(""), Named("Clangd3")),
+   AllOf(Qualifier("nb::"), Named("Clangd4";
+}
+
+TEST(CompletionTest, NoQualifierIfShadowed) {
+  clangd::CodeCompleteOptions Opts = {};
+  Opts.AllScopes = true;
+
+  auto Results = completions(R"cpp(
+namespace nx { class Clangd1 {}; }
+using nx::Clangd1;
+void f() { Clangd^ }
+  )cpp",
+ {cls("nx::Clangd1"), cls("nx::Clangd2")}, Opts);
+  // Although Clangd1 is from another namespace, Sema tells us it's in-scope and
+  // needs no qualifier.
+  EXPECT_THAT(Results.Completions,
+  UnorderedElementsAre(AllOf(Qualifier(""), Named("Clangd1")),
+   AllOf(Qualifier("nx::"), Named("Clangd2";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -136,6 +136,15 @@
 "enabled separatedly."),
 llvm::cl::init(true), llvm::cl::Hidden);
 
+static llvm::cl::opt AllScopesCompletion(
+"all-scopes-completion",
+llvm::cl::desc(
+"If set to true, code completion will include index symbols that are "
+"not defined in the scopes (e.g. "
+"namespaces) visible from the code completion point. Such completions "
+"can insert scope qualifiers."),
+llvm::cl::init(false), llvm::cl::Hidden);
+
 static llvm::cl::opt
 ShowOrigins("debug-origin",
 llvm::cl::desc("Show origins of completion items"),
@@ -304,6 +313,7 @@
   }
   CCOpts.SpeculativeIndexRequest = Opts.StaticIndex;
   CCOpts.EnableFunctionArgSnippets = EnableFunctionArgSnippets;
+  CCOpts.AllScopes = AllScopesCompletion;
 
   // Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(
Index: clangd/index/dex/Dex.cpp
===
--- clangd/index/dex/Dex.cpp
+++ clangd/index/dex/Dex.cpp
@@ -12,6 +12,8 @@
 #include "FuzzyMatch.h"
 #include "Logger.h"
 #include "Quality.h"
+#include "index/Index.h"
+#include "index/dex/Iterator.h"
 #include "llvm/ADT/StringSet.h"
 #include 
 #include 
@@ -166,6 +168,9 @@
 if (It != InvertedIndex.end())
   ScopeIterators.push_back(It->second.iterator());
   }
+  if (Req.AnyScope)
+ScopeIterators.push_back(createBoost(createTrue(Symbols.size()), 0.2));
+
   // Add OR iterator for scopes if there are any Scope Iterators.
   if (!ScopeIterators.empty())
 TopLevelChildren.push_back(createOr(move(ScopeIterators)))

[PATCH] D52364: [clangd] Initial supoprt for cross-namespace global code completion.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/index/Index.h:430
   ///
-  /// The global scope is "", a top level scope is "foo::", etc.
+  /// The global scope is "", a top level scope is "foo::", etc. "*" is
+  /// wildcard.

sammccall wrote:
> I'm not a big fan of this magic value, it seems too easy to forget to handle.
> Especially since we have a lot of existing code that touches this interface, 
> and we may forget to check some of it.
> 
> I suggest making this a separate boolean field `AnyScope`, with a comment 
> that scopes explicitly listed will be ranked higher.
> We can probably also remove the "If this is empty" special case from `Scopes` 
> now too, as the old behavior can be achieved by setting `Scopes = {}; 
> AnyScope = true;`
sounds good.

> We can probably also remove the "If this is empty" special case from Scopes 
> now too, as the old behavior can be achieved by setting Scopes = {}; AnyScope 
> = true;
I think this is a good idea. Unfortunately, there seem to be many tests that 
rely on the old behavior. I'll add a FIXME and do it in followup patch.



Comment at: clangd/index/Index.h:432
+  /// wildcard.
+  /// FIXME: support assigning different weight to each scope.
   std::vector Scopes;

sammccall wrote:
> May not want a heavyweight API with explicit weights...
> 
> I think what we really **need** here is the ability to weight 
> `clang::clangd::` > `clang::` > `` when you're in the scope of namespace 
> clangd.
> 
> We could just say something like "the primary scope should come first" and do 
> some FileDistance-like penalizing of the others...
Changed the wording of `FIXME`.

> I think what we really need here is the ability to weight clang::clangd:: > 
> clang:: > `` when you're in the scope of namespace clangd.
It's unclear what this would mean for scopes from `using-namespace` directives. 
But we can revisit when we get there.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52364



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

[PATCH] D52552: [clang-tidy] Flag Classes Inheriting From Structs

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Please upload patches with full context.
Please do follow coding style, clang-format your patches.

Would it make sense to not catch-all all the `class : struct`, but consider the 
explicitly specified inheritance visibility?




Comment at: clang-tidy/misc/ClassInheritFromStructCheck.cpp:21
+void ClassInheritFromStructCheck::registerMatchers(MatchFinder *Finder) {
+  
Finder->addMatcher(cxxRecordDecl(isClass(),unless(isImplicit()),isDerivedFrom(cxxRecordDecl(isStruct(.bind("class"),
 this);
+}

Please linewrap to 80 chars.



Comment at: test/clang-tidy/misc-class-inherit-from-struct.cpp:13
+};
+
+class C

Missing cases:
* struct inheriting from struct
* Different inheritance visibility:
  * You only check the default visibility
  * What if class explicitly-`public`ly inherits from `struct`?
  * What if class explicitly-`private`ly inherits from `struct`?
  * What if class explicitly-`protected`ly inherits from `struct`?
  * Same for `struct` inheriting from struct?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52552



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

[PATCH] D52545: [docs] Update PostingList string representation format

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clang-tools-extra/clangd/index/dex/Iterator.h:97
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
-  /// PostingList entry and the element which is pointed to by the PostingList
-  /// iterator is enclosed in {} braces.
+  /// represented as "[(...)? (IDN | END) (...)?]" where IDN is N-th
+  /// PostingList entry.

nit: `[(...)? (IDN | END) (...)?]` is accurate but can be really confusing... I 
think it's okay to be a bit inaccurate with just `[ ... CurID ... ]`.


https://reviews.llvm.org/D52545



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

[PATCH] D52545: [docs] Update PostingList string representation format

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 167138.
kbobyrev added a comment.

Simplify the documentation format.


https://reviews.llvm.org/D52545

Files:
  clang-tools-extra/clangd/index/dex/Iterator.h


Index: clang-tools-extra/clangd/index/dex/Iterator.h
===
--- clang-tools-extra/clangd/index/dex/Iterator.h
+++ clang-tools-extra/clangd/index/dex/Iterator.h
@@ -94,9 +94,8 @@
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
-  /// PostingList entry and the element which is pointed to by the PostingList
-  /// iterator is enclosed in {} braces.
+  /// represented as "[... CurID ...]" where CurID is the current PostingList
+  /// entry being inspected.
   friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const Iterator &Iterator) {
 return Iterator.dump(OS);


Index: clang-tools-extra/clangd/index/dex/Iterator.h
===
--- clang-tools-extra/clangd/index/dex/Iterator.h
+++ clang-tools-extra/clangd/index/dex/Iterator.h
@@ -94,9 +94,8 @@
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
-  /// PostingList entry and the element which is pointed to by the PostingList
-  /// iterator is enclosed in {} braces.
+  /// represented as "[... CurID ...]" where CurID is the current PostingList
+  /// entry being inspected.
   friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const Iterator &Iterator) {
 return Iterator.dump(OS);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r343116 - [docs] Update PostingList string representation format

[Kirill Bobyrev via cfe-commits]

Author: omtcyfz
Date: Wed Sep 26 07:59:49 2018
New Revision: 343116

URL: http://llvm.org/viewvc/llvm-project?rev=343116&view=rev
Log:
[docs] Update PostingList string representation format

Because `PostingList` objects are compressed, it is now impossible to
see elements other than the current one and the documentation doesn't
match implementation anymore.

Reviewed By: ioeric

Differential Revision: https://reviews.llvm.org/D52545

Modified:
clang-tools-extra/trunk/clangd/index/dex/Iterator.h

Modified: clang-tools-extra/trunk/clangd/index/dex/Iterator.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/dex/Iterator.h?rev=343116&r1=343115&r2=343116&view=diff
==
--- clang-tools-extra/trunk/clangd/index/dex/Iterator.h (original)
+++ clang-tools-extra/trunk/clangd/index/dex/Iterator.h Wed Sep 26 07:59:49 2018
@@ -94,9 +94,8 @@ public:
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
-  /// PostingList entry and the element which is pointed to by the PostingList
-  /// iterator is enclosed in {} braces.
+  /// represented as "[... CurID ...]" where CurID is the current PostingList
+  /// entry being inspected.
   friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const Iterator &Iterator) {
 return Iterator.dump(OS);


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

[PATCH] D52545: [docs] Update PostingList string representation format

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL343116: [docs] Update PostingList string representation 
format (authored by omtcyfz, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D52545?vs=167138&id=167141#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52545

Files:
  clang-tools-extra/trunk/clangd/index/dex/Iterator.h


Index: clang-tools-extra/trunk/clangd/index/dex/Iterator.h
===
--- clang-tools-extra/trunk/clangd/index/dex/Iterator.h
+++ clang-tools-extra/trunk/clangd/index/dex/Iterator.h
@@ -94,9 +94,8 @@
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
-  /// PostingList entry and the element which is pointed to by the PostingList
-  /// iterator is enclosed in {} braces.
+  /// represented as "[... CurID ...]" where CurID is the current PostingList
+  /// entry being inspected.
   friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const Iterator &Iterator) {
 return Iterator.dump(OS);


Index: clang-tools-extra/trunk/clangd/index/dex/Iterator.h
===
--- clang-tools-extra/trunk/clangd/index/dex/Iterator.h
+++ clang-tools-extra/trunk/clangd/index/dex/Iterator.h
@@ -94,9 +94,8 @@
   ///
   /// Where Type is the iterator type representation: "&" for And, "|" for Or,
   /// ChildN is N-th iterator child. Raw iterators over PostingList are
-  /// represented as "[ID1, ID2, ..., {IDN}, ... END]" where IDN is N-th
-  /// PostingList entry and the element which is pointed to by the PostingList
-  /// iterator is enclosed in {} braces.
+  /// represented as "[... CurID ...]" where CurID is the current PostingList
+  /// entry being inspected.
   friend llvm::raw_ostream &operator<<(llvm::raw_ostream &OS,
const Iterator &Iterator) {
 return Iterator.dump(OS);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52554: [WIP] [clangd] Tests for special methods code-completion

[Jan Korous via Phabricator via cfe-commits]

jkorous created this revision.
jkorous added reviewers: sammccall, ilya-biryukov.
jkorous added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, kadircet, arphaman, dexonsmith, MaskRay, 
ioeric.

Created in order to check we agree on what are the requirements and would later 
write patches against these.

Currently these are failing:
[  FAILED  ] CompletionTestNoExplicitMembers.Struct
[  FAILED  ] CompletionTestNoExplicitMembers.StructTemplate
[  FAILED  ] 
CompletionTestNoExplicitMembers.ExplicitStructTemplateSpecialization
[  FAILED  ] CompletionTestMethodDeclared.ExplicitStructTemplateSpecialization
[  FAILED  ] CompletionTestSpecialMethodsDeclared.Struct
[  FAILED  ] CompletionTestSpecialMethodsDeclared.StructTemplate
[  FAILED  ] 
CompletionTestSpecialMethodsDeclared.ExplicitStructTemplateSpecialization


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52554

Files:
  clangd/CodeCompleteTests.cpp

Index: clangd/CodeCompleteTests.cpp
===
--- clangd/CodeCompleteTests.cpp
+++ clangd/CodeCompleteTests.cpp
@@ -2040,6 +2040,336 @@
   }
 }
 
+TEST(CompletionTestNoExplicitMembers, Struct) {
+  clangd::CodeCompleteOptions Opts;
+  Opts.Limit = 1;
+  auto ResultsDot = completions(R"cpp(
+  struct foo {};
+  void bar() {
+foo a;
+a.^
+)cpp",
+/*IndexSymbols=*/{}, Opts);
+
+  EXPECT_TRUE(ResultsDot.Completions.empty());
+
+  auto ResultsArrow = completions(R"cpp(
+  struct foo {};
+  void bar() {
+foo* b;
+b->^
+)cpp",
+/*IndexSymbols=*/{}, Opts);
+
+  EXPECT_TRUE(ResultsArrow.Completions.empty());
+
+  auto ResultsQualified = completions(R"cpp(
+  struct foo {};
+  foo::^
+)cpp",
+/*IndexSymbols=*/{}, Opts);
+
+  EXPECT_TRUE(ResultsQualified.Completions.empty());
+}
+
+TEST(CompletionTestNoExplicitMembers, StructTemplate) {
+  clangd::CodeCompleteOptions Opts;
+  Opts.Limit = 1;
+  auto ResultsDot = completions(R"cpp(
+  template struct foo {};
+  void bar() {
+foo a;
+a.^
+)cpp",
+/*IndexSymbols=*/{}, Opts);
+
+  EXPECT_TRUE(ResultsDot.Completions.empty());
+
+  auto ResultsArrow = completions(R"cpp(
+  template struct foo {};
+  void bar() {
+foo* b;
+b->^
+)cpp",
+/*IndexSymbols=*/{}, Opts);
+
+  EXPECT_TRUE(ResultsArrow.Completions.empty());
+
+  auto ResultsQualified = completions(R"cpp(
+  template struct foo {};
+  foo::^
+)cpp",
+/*IndexSymbols=*/{}, Opts);
+
+  EXPECT_TRUE(ResultsQualified.Completions.empty());
+}
+
+TEST(CompletionTestNoExplicitMembers, ExplicitStructTemplateSpecialization) {
+  clangd::CodeCompleteOptions Opts;
+  Opts.Limit = 1;
+  auto ResultsDot = completions(R"cpp(
+  template struct foo {}; template<> struct foo {};
+  void bar() {
+foo a;
+a.^
+)cpp",
+/*IndexSymbols=*/{}, Opts);
+
+  EXPECT_TRUE(ResultsDot.Completions.empty());
+
+  auto ResultsArrow = completions(R"cpp(
+  template struct foo {}; template<> struct foo {};
+  void bar() {
+foo* b;
+b->^
+)cpp",
+/*IndexSymbols=*/{}, Opts);
+
+  EXPECT_TRUE(ResultsArrow.Completions.empty());
+
+  auto ResultsQualified = completions(R"cpp(
+  template struct foo {}; template<> struct foo {};
+  foo::^
+)cpp",
+/*IndexSymbols=*/{}, Opts);
+
+  EXPECT_TRUE(ResultsQualified.Completions.empty());
+}
+
+TEST(CompletionTestMethodDeclared, Struct) {
+  clangd::CodeCompleteOptions Opts;
+  Opts.Limit = 1;
+  auto ResultsDot = completions(R"cpp(
+  struct foo { void foomethod(); };
+  void bar() {
+foo a;
+a.^
+)cpp",
+/*IndexSymbols=*/{}, Opts);
+
+  ASSERT_TRUE(ResultsDot.Completions.size() == 1);
+  EXPECT_TRUE(ResultsDot.Completions.front().Name == "foomethod");
+
+  auto ResultsArrow = completions(R"cpp(
+  struct foo { void foomethod(); };
+  void bar() {
+foo* b;
+b->^
+)cpp",
+/*IndexSymbols=*/{}, Opts);
+
+  ASSERT_TRUE(ResultsArrow.Completions.size() == 1);
+  EXPECT_TRUE(ResultsArrow.Completions.front().Name == "foomethod");
+
+  auto ResultsQualified = completions(R"cpp(
+  struct foo { void foomethod(); };
+  foo::^
+)cpp",
+/*IndexSymbols=*/{}, Opts);
+
+  ASSERT_TRUE(ResultsQualified.Completions.size() == 1);
+  EXPECT_TRUE(ResultsQualified.Completions.front().Name == "foomethod");
+
+  auto ResultsQualifiedStatic = completions(R"cpp(
+  struct foo { static void foomethod(); };
+  void bar() {
+foo::^
+)cpp",
+/*IndexSymbols=*/{}, Opts);
+
+  ASSERT_TRUE(ResultsQualifiedStatic.Completions.size() == 1);
+  EXPECT_TRUE(ResultsQualifiedStatic.Completions.front().Name == "foomethod");
+}
+
+TEST(CompletionTestMethodDeclared, StructTemplate) {
+  clangd::CodeCompleteOptions Opts;
+  Opts.Limit = 1;
+  auto ResultsDot = completions(R"cpp(
+  template struct foo { void foomethod(); };
+ 

[clang-tools-extra] r343117 - [clangd] Fix bugs with incorrect memory estimate report

[Kirill Bobyrev via cfe-commits]

Author: omtcyfz
Date: Wed Sep 26 08:06:23 2018
New Revision: 343117

URL: http://llvm.org/viewvc/llvm-project?rev=343117&view=rev
Log:
[clangd] Fix bugs with incorrect memory estimate report

* With the current implementation, `sizeof(std::vector)` is added
twice to the `Dex` memory estimate which is incorrect
* `Dex` logs memory usage estimation before `BackingDataSize` is set and
hence the log report excludes size of the external `SymbolSlab` which is
coupled with `Dex` instance

Reviewed By: ioeric

Differential Revision: https://reviews.llvm.org/D52503

Modified:
clang-tools-extra/trunk/clangd/index/Serialization.cpp
clang-tools-extra/trunk/clangd/index/dex/Dex.cpp
clang-tools-extra/trunk/clangd/index/dex/PostingList.h

Modified: clang-tools-extra/trunk/clangd/index/Serialization.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Serialization.cpp?rev=343117&r1=343116&r2=343117&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Serialization.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Serialization.cpp Wed Sep 26 08:06:23 
2018
@@ -6,8 +6,10 @@
 // License. See LICENSE.TXT for details.
 //
 
//===--===//
+
 #include "Serialization.h"
 #include "Index.h"
+#include "Logger.h"
 #include "RIFF.h"
 #include "Trace.h"
 #include "dex/Dex.h"
@@ -433,8 +435,12 @@ std::unique_ptr loadIndex(l
   }
 
   trace::Span Tracer("BuildIndex");
-  return UseDex ? dex::Dex::build(std::move(Symbols), URISchemes)
-: MemIndex::build(std::move(Symbols), std::move(Refs));
+  auto Index = UseDex ? dex::Dex::build(std::move(Symbols), URISchemes)
+  : MemIndex::build(std::move(Symbols), std::move(Refs));
+  vlog("Loaded {0} from {1} with estimated memory usage {2}",
+   UseDex ? "Dex" : "MemIndex", SymbolFilename,
+   Index->estimateMemoryUsage());
+  return Index;
 }
 
 } // namespace clangd

Modified: clang-tools-extra/trunk/clangd/index/dex/Dex.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/dex/Dex.cpp?rev=343117&r1=343116&r2=343117&view=diff
==
--- clang-tools-extra/trunk/clangd/index/dex/Dex.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/dex/Dex.cpp Wed Sep 26 08:06:23 2018
@@ -130,9 +130,6 @@ void Dex::buildIndex() {
   for (const auto &TokenToPostingList : TempInvertedIndex)
 InvertedIndex.insert(
 {TokenToPostingList.first, PostingList(TokenToPostingList.second)});
-
-  vlog("Built Dex with estimated memory usage {0} bytes.",
-   estimateMemoryUsage());
 }
 
 /// Constructs iterators over tokens extracted from the query and exhausts it
@@ -248,8 +245,8 @@ size_t Dex::estimateMemoryUsage() const
   Bytes += SymbolQuality.size() * sizeof(float);
   Bytes += LookupTable.getMemorySize();
   Bytes += InvertedIndex.getMemorySize();
-  for (const auto &P : InvertedIndex)
-Bytes += P.second.bytes();
+  for (const auto &TokenToPostingList : InvertedIndex)
+Bytes += TokenToPostingList.second.bytes();
   return Bytes + BackingDataSize;
 }
 

Modified: clang-tools-extra/trunk/clangd/index/dex/PostingList.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/dex/PostingList.h?rev=343117&r1=343116&r2=343117&view=diff
==
--- clang-tools-extra/trunk/clangd/index/dex/PostingList.h (original)
+++ clang-tools-extra/trunk/clangd/index/dex/PostingList.h Wed Sep 26 08:06:23 
2018
@@ -66,10 +66,8 @@ public:
   /// go through the chunks and decompress them on-the-fly when necessary.
   std::unique_ptr iterator() const;
 
-  /// Returns in-memory size.
-  size_t bytes() const {
-return sizeof(Chunk) + Chunks.capacity() * sizeof(Chunk);
-  }
+  /// Returns in-memory size of external storage.
+  size_t bytes() const { return Chunks.capacity() * sizeof(Chunk); }
 
 private:
   const std::vector Chunks;


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

[PATCH] D52503: [clangd] Fix bugs with incorrect memory estimate report

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE343117: [clangd] Fix bugs with incorrect memory estimate 
report (authored by omtcyfz, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52503?vs=167079&id=167143#toc

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52503

Files:
  clangd/index/Serialization.cpp
  clangd/index/dex/Dex.cpp
  clangd/index/dex/PostingList.h


Index: clangd/index/Serialization.cpp
===
--- clangd/index/Serialization.cpp
+++ clangd/index/Serialization.cpp
@@ -6,8 +6,10 @@
 // License. See LICENSE.TXT for details.
 //
 
//===--===//
+
 #include "Serialization.h"
 #include "Index.h"
+#include "Logger.h"
 #include "RIFF.h"
 #include "Trace.h"
 #include "dex/Dex.h"
@@ -433,8 +435,12 @@
   }
 
   trace::Span Tracer("BuildIndex");
-  return UseDex ? dex::Dex::build(std::move(Symbols), URISchemes)
-: MemIndex::build(std::move(Symbols), std::move(Refs));
+  auto Index = UseDex ? dex::Dex::build(std::move(Symbols), URISchemes)
+  : MemIndex::build(std::move(Symbols), std::move(Refs));
+  vlog("Loaded {0} from {1} with estimated memory usage {2}",
+   UseDex ? "Dex" : "MemIndex", SymbolFilename,
+   Index->estimateMemoryUsage());
+  return Index;
 }
 
 } // namespace clangd
Index: clangd/index/dex/Dex.cpp
===
--- clangd/index/dex/Dex.cpp
+++ clangd/index/dex/Dex.cpp
@@ -130,9 +130,6 @@
   for (const auto &TokenToPostingList : TempInvertedIndex)
 InvertedIndex.insert(
 {TokenToPostingList.first, PostingList(TokenToPostingList.second)});
-
-  vlog("Built Dex with estimated memory usage {0} bytes.",
-   estimateMemoryUsage());
 }
 
 /// Constructs iterators over tokens extracted from the query and exhausts it
@@ -248,8 +245,8 @@
   Bytes += SymbolQuality.size() * sizeof(float);
   Bytes += LookupTable.getMemorySize();
   Bytes += InvertedIndex.getMemorySize();
-  for (const auto &P : InvertedIndex)
-Bytes += P.second.bytes();
+  for (const auto &TokenToPostingList : InvertedIndex)
+Bytes += TokenToPostingList.second.bytes();
   return Bytes + BackingDataSize;
 }
 
Index: clangd/index/dex/PostingList.h
===
--- clangd/index/dex/PostingList.h
+++ clangd/index/dex/PostingList.h
@@ -66,10 +66,8 @@
   /// go through the chunks and decompress them on-the-fly when necessary.
   std::unique_ptr iterator() const;
 
-  /// Returns in-memory size.
-  size_t bytes() const {
-return sizeof(Chunk) + Chunks.capacity() * sizeof(Chunk);
-  }
+  /// Returns in-memory size of external storage.
+  size_t bytes() const { return Chunks.capacity() * sizeof(Chunk); }
 
 private:
   const std::vector Chunks;


Index: clangd/index/Serialization.cpp
===
--- clangd/index/Serialization.cpp
+++ clangd/index/Serialization.cpp
@@ -6,8 +6,10 @@
 // License. See LICENSE.TXT for details.
 //
 //===--===//
+
 #include "Serialization.h"
 #include "Index.h"
+#include "Logger.h"
 #include "RIFF.h"
 #include "Trace.h"
 #include "dex/Dex.h"
@@ -433,8 +435,12 @@
   }
 
   trace::Span Tracer("BuildIndex");
-  return UseDex ? dex::Dex::build(std::move(Symbols), URISchemes)
-: MemIndex::build(std::move(Symbols), std::move(Refs));
+  auto Index = UseDex ? dex::Dex::build(std::move(Symbols), URISchemes)
+  : MemIndex::build(std::move(Symbols), std::move(Refs));
+  vlog("Loaded {0} from {1} with estimated memory usage {2}",
+   UseDex ? "Dex" : "MemIndex", SymbolFilename,
+   Index->estimateMemoryUsage());
+  return Index;
 }
 
 } // namespace clangd
Index: clangd/index/dex/Dex.cpp
===
--- clangd/index/dex/Dex.cpp
+++ clangd/index/dex/Dex.cpp
@@ -130,9 +130,6 @@
   for (const auto &TokenToPostingList : TempInvertedIndex)
 InvertedIndex.insert(
 {TokenToPostingList.first, PostingList(TokenToPostingList.second)});
-
-  vlog("Built Dex with estimated memory usage {0} bytes.",
-   estimateMemoryUsage());
 }
 
 /// Constructs iterators over tokens extracted from the query and exhausts it
@@ -248,8 +245,8 @@
   Bytes += SymbolQuality.size() * sizeof(float);
   Bytes += LookupTable.getMemorySize();
   Bytes += InvertedIndex.getMemorySize();
-  for (const auto &P : InvertedIndex)
-Bytes += P.second.bytes();
+  for (const auto &TokenToPostingList : InvertedIndex)
+Bytes += TokenToPostingList.second.bytes();
   return Bytes + BackingDataSize;
 }
 
Index: clangd/index/dex/PostingList.h
===
--- clangd/index/dex/PostingList.h

Re: [clang-tools-extra] r342730 - [clangd] Remember to serialize symbol origin in YAML.

[Ilya Biryukov via cfe-commits]

> The downside is probably that all symbols in yaml would have the same
origin, which is a bit redundant but doesn't seem to matter much.
All my comments boil down to this one, but the argument is not only
redundant - it does not make sense design-wise to store an origin in the
serialized static index: the origin is static because the symbol is in the
static index, any other value means we did something wrong.
Including origin would make sense if our serialization was used as wire
transfer format, but that's not the case - it's only used for serializing
into the file index.

Also don't think it's not that important, so feel free to keep as is.

On Wed, Sep 26, 2018 at 2:25 PM Eric Liu  wrote:

> I think it depends on how you interpret the origin. You could tie the
> origin to the index or to the producer of the symbol. The current model
> seems to be the later (origin is a configuration in SymbolCollector.
> MergeIndex manipulates the origin, but I would rather treat that as a
> special case). And I think it's conceptually simpler and more generic
> (origin can be more than dynamic vs static). The downside is probably that
> all symbols in yaml would have the same origin, which is a bit redundant
> but doesn't seem to matter much.
>
> Both models have their pros and cons, but I don't think it's worth
> spending much time figuring out which one is better when it seems like a
> non-issue.
>
> On Tue, Sep 25, 2018 at 6:58 PM Ilya Biryukov 
> wrote:
>
>>
>>
>> Eric Liu  schrieb am Di., 25. Sep. 2018, 01:22:
>>
>>> On Tue, Sep 25, 2018 at 6:34 AM Ilya Biryukov 
>>> wrote:
>>>
 Why would we want to serialize the origin?

>>> We only serialize and deserialize for the static index, it does not seem
 to be useful to serialize origin in that scenario.

>>> We serialize Origin because it's a property of Symbol? The origin for
>>> the current YAML symbols is defaulted to "unknown".
>>>
>> My view would be that it's a property of a symbol living in memory, but
>> not the serialized one. E.g. all symbols read from the yaml index should
>> have the static origin. If we store an origin, we allow loading symbols
>> with non-static origin, it's hard to see how that would be useful.
>>
>>
>>
>>
>>> It's true that we *currently* only serialize symbols from static index,
>>> but there is nothing preventing us from doing it for other indexes with
>>> different origins. You could probably override origins when loading static
>>> index, but that doesn't seem to work in general.
>>>
>> The origin seems to be exactly the field that is set and manipulated by
>> index implementations, but they all have the knowledge on what their origin
>> is or how to combine origins of subindexes.
>>
>> Again, it seems there are two classes hiding in symbol. All other fields
>> provide useful information about C++ semantics of the symbol, while origin
>> provides some traceability when combining indexes. The former is something
>> we need to serialize, the latter is something we can infer when
>> deserializing.
>>
>> If we will have a use case for serializing the latter entity(with origin)
>> for any reason, we might add that separately.
>>
>> Not sure if it's worth the trouble changing it, just wanted to point out
>> that storing the  origin for each symbol in the yaml index for the purpose
>> of indicating that the symbol is in the yaml index seems to be a bit off.
>>
>>
>>
>>> I checked this in without review as I thought this was a trivial fix.
>>> The binary serialization also serializes the Origin field.
>>>
>> LG to be consistent with binary serialization, the same question applies
>> to binary serialization.
>>
>> Again, the change itself seems fine, just nitpicking on whether putting
>> origin into a symbol at that level makes sense.
>>
>>
>>
 Am I missing something?

>>>
 On Fri, Sep 21, 2018 at 3:06 PM Eric Liu via cfe-commits <
 cfe-commits@lists.llvm.org> wrote:

> Author: ioeric
> Date: Fri Sep 21 06:04:57 2018
> New Revision: 342730
>
> URL: http://llvm.org/viewvc/llvm-project?rev=342730&view=rev
> Log:
> [clangd] Remember to serialize symbol origin in YAML.
>
> Modified:
> clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
> clang-tools-extra/trunk/unittests/clangd/SerializationTests.cpp
>
> Modified: clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp?rev=342730&r1=342729&r2=342730&view=diff
>
> ==
> --- clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp (original)
> +++ clang-tools-extra/trunk/clangd/index/SymbolYAML.cpp Fri Sep 21
> 06:04:57 2018
> @@ -27,6 +27,7 @@ namespace yaml {
>
>  using clang::clangd::Symbol;
>  using clang::clangd::SymbolID;
> +using clang::clangd::SymbolOrigin;
>  using clang::clangd::Sy

[PATCH] D51741: [coro]Pass rvalue reference for named local variable to return_value

[Brian Gesiak via Phabricator via cfe-commits]

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

This is great, thanks! Sorry for letting it languish. I defer to @GorNishanov, 
but I don't see why this couldn't go in now and if there're any edge cases I'm 
missing we can address those in another patch. I pulled this down and played 
around with it, and everything seemed OK to me.

I had one nit about formatting, but other than that this is good to go. Do you 
have commit access? If not, let me know if I can land this for you.




Comment at: lib/Sema/SemaCoroutine.cpp:851
+  ExprResult MoveResult =
+  this->PerformMoveOrCopyInitialization(Entity, NRVOCandidate, 
E->getType(), E);
+  if (MoveResult.get())

nit: When I run clang-format on this patch, it suggests the following here:

```
  ExprResult MoveResult = this->PerformMoveOrCopyInitialization(
  Entity, NRVOCandidate, E->getType(), E);
```

Splitting it up as above makes sure this line stays within the 
80-character-per-line limit.


Repository:
  rC Clang

https://reviews.llvm.org/D51741



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

[PATCH] D52547: Tell whether file/folder for include completions.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

A drive-by comment.
Would it be cleaner to pass this information from clang? Relying on completion 
label seems shaky.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52547



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

[PATCH] D52281: [clang-tidy] Add modernize check to use std::invoke in generic code

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:70
+const auto *Paren = dyn_cast(MFunctor->getCallee());
+const auto *BinOp = dyn_cast(Paren->getSubExpr());
+

How do you know `Paren` won't be null? If it cannot be null, please use 
`cast<>` instead, otherwise, you should be checking for null before 
dereferencing.



Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:85
+
+const std::string Param = ObjName + ", " + FuncName;
+diagnose(MFunctor, Param, OriginalParams);

Perhaps use a `Twine` in the `diagnose()` call for this.



Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:94-96
+  const Twine &Replace = Twine("::std::invoke(") + Param +
+ (Functor->getNumArgs() == 0 ? "" : ", ") +
+ OriginalParams;

This is dangerous (the intermediary temps will be destroyed and you will be 
referencing dangling memory) -- you should lower it into the call to 
`CreateReplacement()`.



Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:98
+  diag(Functor->getExprLoc(),
+   "Use ::std::invoke to invoke type dependent callable objects.")
+  << FixItHint::CreateReplacement(Functor->getSourceRange(), 
Replace.str());

Diagnostics should not be complete sentences, so Use -> use and drop the full 
stop.



Comment at: test/clang-tidy/modernize-replace-generic-functor-call.cpp:23-25
+template 
+void func2(T func) {
+  func(1);

Can you add a test case that demonstrates this works well in the presence of 
std::forward? This is a common pattern:
```
template 
void f(Callable&& C, Args&&... As) {
  std::forward(C)(std::forward(As)...);
}
```
This could be converted into:
```
template 
void f(Callable&& C, Args&&... As) {
  std::invoke(C, As...);
}
```


https://reviews.llvm.org/D52281



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

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:1038
+
+  SmallString<128> NewAddNullTermExprStr;
+  NewAddNullTermExprStr = "\n";

JonasToth wrote:
> whisperity wrote:
> > aaron.ballman wrote:
> > > This should be done using a `Twine`.
> > Can you please give an example of how to well-approach this? We had issues 
> > with using Twines on the stack and then later on running into weird 
> > undefined behaviour. The documentation is not clear to a lot of our 
> > colleagues on where the `Twine`'s usage barriers are... (Asking this not 
> > just for @Charusso but also for a few more colleagues, @baloghadamsoftware 
> > e.g.)
> naive approach:
> 
> ```
> std::string New = 
> Twine("\n").concat(SpaceBeforeStmtStr).concat(exprToStr(..))...).str();
> ```
> I think retrieving a `SmallString` is not supported. Please note, that 
> `Twine` does support taking integer and floats directly without prior 
> conversion.
> You can use `operator+` instead of `concat` too.
> 
> Did this help or did you have more specific issues? 
The important bit is -- don't try to store a Twine (locally or otherwise). It's 
fine as a parameter in a function, but otherwise you should try to use it only 
as part of an expression. e.g., `(Twine("foo") + "bar" + baz).str()` is a good 
pattern to get used to.


https://reviews.llvm.org/D45050



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

[PATCH] D52552: [clang-tidy] Flag Classes Inheriting From Structs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This strikes me as likely being a very chatty diagnostic. For instance, it will 
trigger on harmless code that uses empty base class optimization (in addition 
to other concerns pointed out by @lebedev.ri). It seems like the real concern 
here is unintentional information disclosure, but I don't see a good heuristic 
for determining that something is "unintentional" or not. I'd be curious to 
know how frequently this triggers on some large C++ code bases.




Comment at: docs/clang-tidy/checks/misc-class-inherit-from-struct.rst:6-10
+Finds instances of classes inheriting from structs. Structs are meant for 
+storing data and are often used to maintain C compatibility. Having a class
+inherit from them can lead to confusion with what type of object is being 
+dealt with. Additionally, the default public nature of struct members can 
+lead to unintentional exposure of members.

I don't agree with the predicate here -- structs aren't just used for C 
compatibility (look at  as an example). They're also useful when 
all members need to be public, which is exactly the situation you claim may be 
unintentional.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D52552



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