[PATCH] D53231: [Sema] Fix PR38987: keep end location of a direct initializer list

2018-10-14 Thread Orivej Desh via Phabricator via cfe-commits 
orivej added a comment.

I don't have the complete picture yet. (This is why the summary is so short.)
I have looked up that `IK_DirectList` `Kind` was known not to carry a valid 
`ParenRange` since 
https://github.com/llvm-mirror/clang/commit/188158db29f50443b6e412f2a40c800b2669c957,
 and that `PerformConstructorInitialization` acquired `BraceLoc` arguments 
specifically to support `CXXTemporaryObjectExpr`: 
https://github.com/llvm-mirror/clang/commit/1245a54ca6e9c5b14196461dc3f84b24ea6594b1#diff-d7cc8293491a9fdddee7ba857c028256R5921
 , but then it appears that `CXXTemporaryObjectExpr` `Kind` has acquired a 
valid `ParenRange`, except when it is instantiated from a template…


Repository:
  rC Clang

https://reviews.llvm.org/D53231



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

[PATCH] D53250: [ToolChain] Use default linker if the toolchain uses a custom one

2018-10-14 Thread Jonas Hahnfeld via Phabricator via cfe-commits 
Hahnfeld added a comment.

If I read that patch correctly, this will render `-fuse-ld` with non-absolute 
paths useless if a toolchain has `DefaultLinker != "ld"`. I don't think that's 
what we want to do if the user explicitly sets a different linker.

In https://reviews.llvm.org/D53250#1264626, @phosek wrote:

> This should avoid workarounds such as https://reviews.llvm.org/D49899 or 
> https://reviews.llvm.org/D53249.


I agree that these changes are ugly, but we do the same with `-stdlib=platform` 
and `-rtlib=platform` for tests that check Clang's choices for a particular 
platform. For this case it's what you did in https://reviews.llvm.org/D53249: 
Adding `-fuse-ld=ld` to tests that check the default linker for a platform. 
(because there is a case for `UseLinker == "ld"`)


Repository:
  rC Clang

https://reviews.llvm.org/D53250



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

[PATCH] D53249: Force Hexagon to use default (hexagon-link) linker

2018-10-14 Thread Jonas Hahnfeld via Phabricator via cfe-commits 
Hahnfeld added a comment.

As said on https://reviews.llvm.org/D53250 I think this is the right way to fix 
these tests: We already do the same for `-stdlib=platform` and 
`-rtlib=platform`.


Repository:
  rC Clang

https://reviews.llvm.org/D53249



___
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

2018-10-14 Thread Jonas Toth via Phabricator via cfe-commits 
JonasToth added a comment.

I am in contact with the guy that actually discovered this breakage. It seems 
to be a weird and hard to reproduce error, but happens on x86 as well. So it is 
a compile/header something combination that results in the error. I am pretty 
sure that will take a while until we figured it out :/


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] D53219: Update hexagon driver tests

2018-10-14 Thread Sid Manning via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL344482: [Hexagon] Update tests account for non-hardcoded 
linker name. (authored by sidneym, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53219?vs=169491&id=169608#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53219

Files:
  cfe/trunk/test/Driver/hexagon-toolchain-elf.c
  cfe/trunk/test/Driver/linux-ld.c

Index: cfe/trunk/test/Driver/hexagon-toolchain-elf.c
===
--- cfe/trunk/test/Driver/hexagon-toolchain-elf.c
+++ cfe/trunk/test/Driver/hexagon-toolchain-elf.c
@@ -63,63 +63,63 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK020 %s
 // CHECK020: "-cc1" {{.*}} "-target-cpu" "hexagonv4"
-// CHECK020: hexagon-link{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v4/crt0
+// CHECK020: {{hexagon-link|ld}}{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v4/crt0
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
 // RUN:   -mcpu=hexagonv5 \
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK021 %s
 // CHECK021: "-cc1" {{.*}} "-target-cpu" "hexagonv5"
-// CHECK021: hexagon-link{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v5/crt0
+// CHECK021: {{hexagon-link|ld}}{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v5/crt0
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
 // RUN:   -mcpu=hexagonv55 \
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK022 %s
 // CHECK022: "-cc1" {{.*}} "-target-cpu" "hexagonv55"
-// CHECK022: hexagon-link{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v55/crt0
+// CHECK022: {{hexagon-link|ld}}{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v55/crt0
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
 // RUN:   -mcpu=hexagonv60 \
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK023 %s
 // CHECK023: "-cc1" {{.*}} "-target-cpu" "hexagonv60"
-// CHECK023: hexagon-link{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v60/crt0
+// CHECK023: {{hexagon-link|ld}}{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v60/crt0
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
 // RUN:   -mcpu=hexagonv62 \
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK024 %s
 // CHECK024: "-cc1" {{.*}} "-target-cpu" "hexagonv62"
-// CHECK024: hexagon-link{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v62/crt0
+// CHECK024: {{hexagon-link|ld}}{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v62/crt0
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
 // RUN:   -mcpu=hexagonv65 \
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK025 %s
 // CHECK025: "-cc1" {{.*}} "-target-cpu" "hexagonv65"
-// CHECK025: hexagon-link{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v65/crt0
+// CHECK025: {{hexagon-link|ld}}{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v65/crt0
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
 // RUN:   -O3 \
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK026 %s
 // CHECK026-NOT: "-ffp-contract=fast"
-// CHECK026: hexagon-link
+// CHECK026: {{hexagon-link|ld}}
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
 // RUN:   -O3 -ffp-contract=off \
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK027 %s
 // CHECK027-NOT: "-ffp-contract=fast"
-// CHECK027: hexagon-link
+// CHECK027: {{hexagon-link|ld}}
 
 // -
 // Test Linker related args
@@ -134,7 +134,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK030 %s
 // CHECK030: "-cc1"
-// CHECK030-NEXT: hexagon-link
+// CHECK030: {{hexagon-link|ld}}
 // CHECK030-NOT: "-static"
 // CHECK030-NOT: "-shared"
 // CHECK030: "{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v60/crt0_standalone.o"
@@ -155,7 +155,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK031 %s
 // CHECK031: "-cc1"
-// CHECK031-NEXT: hexagon-link
+// CHECK031: {{hexagon-link|ld}}
 // CHECK031-NOT: "-static"
 // CHECK031-NOT: "-shared"
 // CHECK031: "{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v60/crt0_standalone.o"
@@ -178,7 +178,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK032 %s
 // CHECK032: "-cc1"
-// CHECK032-NEXT: hexagon-link
+// CHECK032: {{hexagon-link|ld}}
 // CHECK032-NOT: "-static"
 // CHECK032-NOT: "-shared"
 // CHECK032: "{{.*}}/Inputs/hexagon_tree/Tools/bin

r344482 - [Hexagon] Update tests account for non-hardcoded linker name.

2018-10-14 Thread Sid Manning via cfe-commits 
Author: sidneym
Date: Sun Oct 14 10:51:36 2018
New Revision: 344482

URL: http://llvm.org/viewvc/llvm-project?rev=344482&view=rev
Log:
[Hexagon] Update tests account for non-hardcoded linker name.

Tests should not assume the linker's name, CLANG_DEFAULT_LINKER could
change it.

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

Modified:
cfe/trunk/test/Driver/hexagon-toolchain-elf.c
cfe/trunk/test/Driver/linux-ld.c

Modified: cfe/trunk/test/Driver/hexagon-toolchain-elf.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/hexagon-toolchain-elf.c?rev=344482&r1=344481&r2=344482&view=diff
==
--- cfe/trunk/test/Driver/hexagon-toolchain-elf.c (original)
+++ cfe/trunk/test/Driver/hexagon-toolchain-elf.c Sun Oct 14 10:51:36 2018
@@ -63,7 +63,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK020 %s
 // CHECK020: "-cc1" {{.*}} "-target-cpu" "hexagonv4"
-// CHECK020: 
hexagon-link{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v4/crt0
+// CHECK020: 
{{hexagon-link|ld}}{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v4/crt0
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
@@ -71,7 +71,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK021 %s
 // CHECK021: "-cc1" {{.*}} "-target-cpu" "hexagonv5"
-// CHECK021: 
hexagon-link{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v5/crt0
+// CHECK021: 
{{hexagon-link|ld}}{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v5/crt0
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
@@ -79,7 +79,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK022 %s
 // CHECK022: "-cc1" {{.*}} "-target-cpu" "hexagonv55"
-// CHECK022: 
hexagon-link{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v55/crt0
+// CHECK022: 
{{hexagon-link|ld}}{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v55/crt0
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
@@ -87,7 +87,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK023 %s
 // CHECK023: "-cc1" {{.*}} "-target-cpu" "hexagonv60"
-// CHECK023: 
hexagon-link{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v60/crt0
+// CHECK023: 
{{hexagon-link|ld}}{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v60/crt0
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
@@ -95,7 +95,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK024 %s
 // CHECK024: "-cc1" {{.*}} "-target-cpu" "hexagonv62"
-// CHECK024: 
hexagon-link{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v62/crt0
+// CHECK024: 
{{hexagon-link|ld}}{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v62/crt0
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
@@ -103,7 +103,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK025 %s
 // CHECK025: "-cc1" {{.*}} "-target-cpu" "hexagonv65"
-// CHECK025: 
hexagon-link{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v65/crt0
+// CHECK025: 
{{hexagon-link|ld}}{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v65/crt0
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
@@ -111,7 +111,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK026 %s
 // CHECK026-NOT: "-ffp-contract=fast"
-// CHECK026: hexagon-link
+// CHECK026: {{hexagon-link|ld}}
 
 // RUN: %clang -### -target hexagon-unknown-elf \
 // RUN:   -ccc-install-dir %S/Inputs/hexagon_tree/Tools/bin \
@@ -119,7 +119,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK027 %s
 // CHECK027-NOT: "-ffp-contract=fast"
-// CHECK027: hexagon-link
+// CHECK027: {{hexagon-link|ld}}
 
 // 
-
 // Test Linker related args
@@ -134,7 +134,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK030 %s
 // CHECK030: "-cc1"
-// CHECK030-NEXT: hexagon-link
+// CHECK030: {{hexagon-link|ld}}
 // CHECK030-NOT: "-static"
 // CHECK030-NOT: "-shared"
 // CHECK030: 
"{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v60/crt0_standalone.o"
@@ -155,7 +155,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK031 %s
 // CHECK031: "-cc1"
-// CHECK031-NEXT: hexagon-link
+// CHECK031: {{hexagon-link|ld}}
 // CHECK031-NOT: "-static"
 // CHECK031-NOT: "-shared"
 // CHECK031: 
"{{.*}}/Inputs/hexagon_tree/Tools/bin/../target/hexagon/lib/v60/crt0_standalone.o"
@@ -178,7 +178,7 @@
 // RUN:   %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK032 %s
 // CHECK032: "-cc1"
-// CHECK032-NEXT: hexagon-link
+// CHECK032: {{he

[PATCH] D53250: [ToolChain] Use default linker if the toolchain uses a custom one

2018-10-14 Thread Petr Hosek via Phabricator via cfe-commits 
phosek added a comment.

In https://reviews.llvm.org/D53250#1264708, @Hahnfeld wrote:

> If I read that patch correctly, this will render `-fuse-ld` with non-absolute 
> paths useless if a toolchain has `DefaultLinker != "ld"`. I don't think 
> that's what we want to do if the user explicitly sets a different linker.


You're right, I missed that case, I'll try to address it.

> I agree that these changes are ugly, but we do the same with 
> `-stdlib=platform` and `-rtlib=platform` for tests that check Clang's choices 
> for a particular platform. For this case it's what you did in 
> https://reviews.llvm.org/D53249: Adding `-fuse-ld=ld` to tests that check the 
> default linker for a platform. (because there is a case for `UseLinker == 
> "ld"`)

I agree about the test case but I think the current logic is broken even for 
regular use. If the platform specifies a custom linker (e.g. `hegaxon-link`), 
`CLANG_DEFAULT_LINKER` should not override it because it's unlikely that it'll 
be usable on that platform. Today we treat `CLANG_DEFAULT_LINKER` as a default 
value for `-fuse-ld=` but I think we should instead treat it as a default value 
for `ld` (i.e. default system linker) which will be used if `-fuse-ld=` is 
unspecified or the platform doesn't specify its own linker. Would you agree 
with that?

The reason why this is not a problem of `-stdlib=` and `-rtlib=` is because 
many toolchains override `ToolChain::GetRuntimeLibType` and 
`ToolChain::GetCXXStdlibType` (e.g. Fuchsia 

 or WebAssembly 
)
 completely bypassing the default logic. The reason why this is not being used 
for linker selection is because `-fuse-ld=/path/to/linker` is generally useful 
even for these targets that use custom linkers (e.g. 
https://reviews.llvm.org/D53038 explicitly mentions that) so I think we either 
need to make `ToolChain::GetLinkerPath` smarter about handling 
platform-specific linkers or we need to extract the logic for handling linker 
specified via absolute path into a helper method so toolchains can override 
`ToolChain::GetLinkerPath` without re-implementing chunk of its logic.


Repository:
  rC Clang

https://reviews.llvm.org/D53250



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

[PATCH] D53219: Update hexagon driver tests

2018-10-14 Thread Petr Hosek via Phabricator via cfe-commits 
phosek added a comment.

The way we addressed this for other targets in the past is to explicitly set 
`-fuse-ld=ld` for all tests, I made that change in 
https://reviews.llvm.org/D53249.


Repository:
  rL LLVM

https://reviews.llvm.org/D53219



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

[PATCH] D53263: [CodeGen] Fix places where the return type of a FunctionDecl was being used in place of the function type

2018-10-14 Thread Ben via Phabricator via cfe-commits 
bobsayshilol created this revision.
bobsayshilol added reviewers: sberg, ahatanak.
Herald added subscribers: cfe-commits, jfb.

`FunctionDecl::Create()` takes as its `T` parameter the type of function that 
should be created, not the return type. Passing in the return type looks to 
have been copypasta'd around a bit, but the number of correct usages outweighs 
the incorrect ones so I've opted for keeping what `T` is the same and fixing up 
the call sites instead.

This fixes a crash in Clang when attempting to compile the following snippet of 
code with `-fblocks -fsanitize=function -x objective-c++` (the original repro 
case):

  void g(void(^)());
  void f()
  {
__block int a = 0;
g(^(){ a++; });
  }

as well as the following which only requires `-fsanitize=function -x c++`:

  void f(char * buf)
  {
__builtin_os_log_format(buf, "");
  }

I haven't added the above as tests or checked that all of the current tests 
build so I don't expect this to get merged straight away, but I wanted to check 
that this is matching guidelines and potentially avoiding any duplicate work if 
someone else also starts trying to track down this issue too.


Repository:
  rC Clang

https://reviews.llvm.org/D53263

Files:
  lib/AST/Decl.cpp
  lib/CodeGen/CGBlocks.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGNonTrivialStruct.cpp
  lib/CodeGen/CGObjC.cpp
  lib/CodeGen/CGStmtOpenMP.cpp
  lib/CodeGen/ItaniumCXXABI.cpp

Index: lib/CodeGen/ItaniumCXXABI.cpp
===
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -2315,11 +2315,13 @@
 FTy, GlobalInitFnName, getTypes().arrangeNullaryFunction(),
 SourceLocation());
 ASTContext &Ctx = getContext();
+QualType ReturnTy = Ctx.VoidTy;
+QualType FunctionTy = Ctx.getFunctionType(ReturnTy, llvm::None, {});
 FunctionDecl *FD = FunctionDecl::Create(
 Ctx, Ctx.getTranslationUnitDecl(), SourceLocation(), SourceLocation(),
-&Ctx.Idents.get(GlobalInitFnName), Ctx.VoidTy, nullptr, SC_Static,
+&Ctx.Idents.get(GlobalInitFnName), FunctionTy, nullptr, SC_Static,
 false, false);
-CGF.StartFunction(GlobalDecl(FD), getContext().VoidTy, GlobalInitFn,
+CGF.StartFunction(GlobalDecl(FD), ReturnTy, GlobalInitFn,
   getTypes().arrangeNullaryFunction(), FunctionArgList(),
   SourceLocation(), SourceLocation());
 
Index: lib/CodeGen/CGStmtOpenMP.cpp
===
--- lib/CodeGen/CGStmtOpenMP.cpp
+++ lib/CodeGen/CGStmtOpenMP.cpp
@@ -385,11 +385,11 @@
   FunctionDecl *DebugFunctionDecl = nullptr;
   if (!FO.UIntPtrCastRequired) {
 FunctionProtoType::ExtProtoInfo EPI;
+QualType FunctionTy = Ctx.getFunctionType(Ctx.VoidTy, llvm::None, EPI);
 DebugFunctionDecl = FunctionDecl::Create(
 Ctx, Ctx.getTranslationUnitDecl(), FO.S->getBeginLoc(),
-SourceLocation(), DeclarationName(), Ctx.VoidTy,
-Ctx.getTrivialTypeSourceInfo(
-Ctx.getFunctionType(Ctx.VoidTy, llvm::None, EPI)),
+SourceLocation(), DeclarationName(), FunctionTy,
+Ctx.getTrivialTypeSourceInfo(FunctionTy),
 SC_Static, /*isInlineSpecified=*/false, /*hasWrittenPrototype=*/false);
   }
   for (const FieldDecl *FD : RD->fields()) {
Index: lib/CodeGen/CGObjC.cpp
===
--- lib/CodeGen/CGObjC.cpp
+++ lib/CodeGen/CGObjC.cpp
@@ -3229,29 +3229,36 @@
   ASTContext &C = getContext();
   IdentifierInfo *II
 = &CGM.getContext().Idents.get("__assign_helper_atomic_property_");
+
+  QualType ReturnTy = C.VoidTy;
+  QualType DestTy = C.getPointerType(Ty);
+  QualType SrcTy = Ty;
+  SrcTy.addConst();
+  SrcTy = C.getPointerType(SrcTy);
+
+  SmallVector ArgTys;
+  ArgTys.push_back(DestTy);
+  ArgTys.push_back(SrcTy);
+  QualType FunctionTy = C.getFunctionType(ReturnTy, ArgTys, {});
+
   FunctionDecl *FD = FunctionDecl::Create(C,
   C.getTranslationUnitDecl(),
   SourceLocation(),
-  SourceLocation(), II, C.VoidTy,
+  SourceLocation(), II, FunctionTy,
   nullptr, SC_Static,
   false,
   false);
 
-  QualType DestTy = C.getPointerType(Ty);
-  QualType SrcTy = Ty;
-  SrcTy.addConst();
-  SrcTy = C.getPointerType(SrcTy);
-
   FunctionArgList args;
-  ImplicitParamDecl DstDecl(getContext(), FD, SourceLocation(), /*Id=*/nullptr,
+  ImplicitParamDecl DstDecl(C, FD, SourceLocation(), /*Id=*/nullptr,
 DestTy, ImplicitParamDecl::Other);
   args.push_back(&DstDecl);
-  ImplicitParamDecl SrcDecl(getContext(), FD, SourceLocation(), /*Id=*/nullptr,
+  ImplicitParamDecl SrcDecl(C, FD, SourceLocation(),

[PATCH] D51633: [ASTImporter] Added error handling for AST import.

2018-10-14 Thread Aleksei Sidorin via Phabricator via cfe-commits 
a_sidorin added a comment.

Hi Balázs,

Finally, I have completed the review. Despite the fact that this patch is huge, 
I found only a few issues.




Comment at: lib/AST/ASTImporter.cpp:4604
+  if (Error Err = ImportDeclContext(D))
+// FIXME: Really ignore the error?
+consumeError(std::move(Err));

I think we can just `return std::move(Err);` as it was done below.



Comment at: lib/AST/ASTImporter.cpp:5657
+  if (!ToSemiLocOrErr)
+return nullptr;
+  return new (Importer.getToContext()) NullStmt(

Shouldn't we return an error here?



Comment at: lib/AST/ASTImporter.cpp:7339
+  // FIXME: Why is this copy needed?
+  // Why not used in the CXXOperatorCallExpr case?
   auto **ToArgs_Copied = new (Importer.getToContext()) Expr*[NumArgs];

That's true, this copying can be removed.


Repository:
  rC Clang

https://reviews.llvm.org/D51633



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

[PATCH] D53249: Force Hexagon to use default (hexagon-link) linker

2018-10-14 Thread Sid Manning via Phabricator via cfe-commits 
sidneym added a comment.

https://reviews.llvm.org/D53219 added a check for either hexagon-ld or ld.  I 
merged that change earlier today.  I can change the test to pass -fuse-ld, let 
me know.  Thanks,


Repository:
  rC Clang

https://reviews.llvm.org/D53249



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

[PATCH] D53244: [Coverage] Fix PR39258: support coverage regions that start deeper than they end

2018-10-14 Thread Vedant Kumar via Phabricator via cfe-commits 
vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM.




Comment at: test/CoverageMapping/macros.c:60
 
+// CHECK-NEXT: func6
+void func6(unsigned count) { // CHECK-NEXT: File 0, [[@LINE]]:28 -> 
[[@LINE+4]]:2 = #0

Please use CHECK-LABEL here.


Repository:
  rC Clang

https://reviews.llvm.org/D53244



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

[PATCH] D53082: [clang-doc] Add unit tests for bitcode

2018-10-14 Thread Petr Hosek via Phabricator via cfe-commits 
phosek added inline comments.



Comment at: clang-tools-extra/unittests/clang-doc/BitcodeTest.cpp:2
+//===-- clang-doc/BitcodeTest.cpp
+//--===//
+//

Nit: I think this should be on the previous line.


https://reviews.llvm.org/D53082



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

[PATCH] D52939: ExprConstant: Propagate correct return values from constant evaluation.

2018-10-14 Thread James Y Knight via Phabricator via cfe-commits 
jyknight updated this revision to Diff 169641.
jyknight marked 11 inline comments as done.
jyknight added a comment.

Address most comments.


https://reviews.llvm.org/D52939

Files:
  clang/include/clang/AST/Expr.h
  clang/lib/AST/ExprConstant.cpp

Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -412,52 +412,10 @@
   MostDerivedArraySize = 2;
   MostDerivedPathLength = Entries.size();
 }
-void diagnoseUnsizedArrayPointerArithmetic(EvalInfo &Info, const Expr *E);
-void diagnosePointerArithmetic(EvalInfo &Info, const Expr *E,
+bool diagnosePointerArithmetic(EvalInfo &Info, const Expr *E,
const APSInt &N);
 /// Add N to the address of this subobject.
-void adjustIndex(EvalInfo &Info, const Expr *E, APSInt N) {
-  if (Invalid || !N) return;
-  uint64_t TruncatedN = N.extOrTrunc(64).getZExtValue();
-  if (isMostDerivedAnUnsizedArray()) {
-diagnoseUnsizedArrayPointerArithmetic(Info, E);
-// Can't verify -- trust that the user is doing the right thing (or if
-// not, trust that the caller will catch the bad behavior).
-// FIXME: Should we reject if this overflows, at least?
-Entries.back().ArrayIndex += TruncatedN;
-return;
-  }
-
-  // [expr.add]p4: For the purposes of these operators, a pointer to a
-  // nonarray object behaves the same as a pointer to the first element of
-  // an array of length one with the type of the object as its element type.
-  bool IsArray = MostDerivedPathLength == Entries.size() &&
- MostDerivedIsArrayElement;
-  uint64_t ArrayIndex =
-  IsArray ? Entries.back().ArrayIndex : (uint64_t)IsOnePastTheEnd;
-  uint64_t ArraySize =
-  IsArray ? getMostDerivedArraySize() : (uint64_t)1;
-
-  if (N < -(int64_t)ArrayIndex || N > ArraySize - ArrayIndex) {
-// Calculate the actual index in a wide enough type, so we can include
-// it in the note.
-N = N.extend(std::max(N.getBitWidth() + 1, 65));
-(llvm::APInt&)N += ArrayIndex;
-assert(N.ugt(ArraySize) && "bounds check failed for in-bounds index");
-diagnosePointerArithmetic(Info, E, N);
-setInvalid();
-return;
-  }
-
-  ArrayIndex += TruncatedN;
-  assert(ArrayIndex <= ArraySize &&
- "bounds check succeeded for out-of-bounds index");
-
-  if (IsArray)
-Entries.back().ArrayIndex = ArrayIndex;
-  else
-IsOnePastTheEnd = (ArrayIndex != 0);
-}
+bool adjustIndex(EvalInfo &Info, const Expr *E, APSInt N);
   };
 
   /// A stack frame in the constexpr call stack.
@@ -721,43 +679,75 @@
 bool IsSpeculativelyEvaluating;
 
 enum EvaluationMode {
-  /// Evaluate as a constant expression. Stop if we find that the expression
-  /// is not a constant expression.
+  // The various EvaluationMode kinds vary in terms of what sorts of
+  // expressions they accept.
+  //
+  // Key for the following table:
+  // * N = Expressions which are evaluable, but are not a constant
+  //   expression (according to the language rules) are OK
+  //   (keepEvaluatingAfterNonConstexpr)
+  // * S = Failure to evaluate which only occurs in expressions used for
+  //   side-effect are okay.  (keepEvaluatingAfterSideEffect)
+  // * F = Failure to evaluate is okay. (keepEvaluatingAfterFailure)
+  // * E = Eagerly evaluate certain builtins to a value which would normally
+  //   be deferred until after optimizations.
+  //
+  // +-+---+---+---+---+
+  // | | N | S | F | E |
+  // +-+---+---+---+---+
+  // |EM_ConstantExpression| . | . | . | . |
+  // |EM_ConstantFold  | Y | . | . | . |
+  // |EM_ConstantFoldUnevaluated   | Y | . | . | Y |
+  // |EM_IgnoreSideEffects | Y | Y | . | . |
+  // |EM_PotentialConstantExpression   | . | Y | Y | . |
+  // |EM_PotentialConstantExpressionUnevaluated| . | Y | Y | Y |
+  // |EM_EvaluateForOverflow   | Y | Y | Y | . |
+  // +-+---+---+---+---+
+  //
+  // TODO: Fix EM_ConstantExpression and EM_PotentialConstantExpression to
+  // also eagerly evaluate. (and then delete
+  // EM_PotentialConstantExpressionUnevaluated as a duplicate).  This will
+  // be somewhat tricky to change, because LLVM currently verifies that an
+  // expression is a constant expression (possibly strictly with
+  // EM_ConstantExpression) in Sema/*, while it evaluates the value
+  // separately in CodeGen/* with EM_ConstantF

[PATCH] D52939: ExprConstant: Propagate correct return values from constant evaluation.

2018-10-14 Thread James Y Knight via Phabricator via cfe-commits 
jyknight added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:12-32
 // Constant expression evaluation produces four main results:
 //
 //  * A success/failure flag indicating whether constant folding was 
successful.
 //This is the 'bool' return value used by most of the code in this file. A
 //'false' return value indicates that constant folding has failed, and any
 //appropriate diagnostic has already been produced.
 //

rsmith wrote:
> Is this comment still correct?
Yes -- the "potential" constant expression mode still has this semantic.



Comment at: clang/lib/AST/ExprConstant.cpp:682
 enum EvaluationMode {
-  /// Evaluate as a constant expression. Stop if we find that the 
expression
-  /// is not a constant expression.
+  /* The various EvaluationMode kinds vary in terms of what sorts of
+   * expressions they accept.

rsmith wrote:
> Nit: LLVM style avoids `/*...*/` comments even for long comment blocks like 
> this (https://llvm.org/docs/CodingStandards.html#comment-formatting).
Done.



Comment at: clang/lib/AST/ExprConstant.cpp:686-687
+ Key for the following table:
+ * D = Diagnostic notes (about non-constexpr, but still evaluable
+   constructs) are OK (keepEvaluatingAfterNote)
+ * S = Failure to evaluate which only occurs in expressions used for

rsmith wrote:
> I think talking about diagnostic notes here distracts from the purpose, which 
> is that this column indicates that we accept constructs that are not 
> permitted by the language mode's constant expression rules.
Reworded.



Comment at: clang/lib/AST/ExprConstant.cpp:688-690
+ * S = Failure to evaluate which only occurs in expressions used for
+   side-effect are okay.  (keepEvaluatingAfterSideEffect)
+ * F = Failure to evaluate is okay. (keepEvaluatingAfterFailure)

rsmith wrote:
> "is okay" here is misleading, I think. The point of 
> `keepEvaluatingAfter[...]` is that it's safe to stop evaluating if they 
> return false, because later evaluations can't affect the overall result.
The overall return value of the evaluation indicates whether the expression was 
evaluable under a specified set of conditions. The different modes cause 
failure under different conditions -- so I think "is okay" really _is_ what is 
meant here. In this particular case, "failure to evaluate" can still return 
success!

Not sure if some rewording would communicate that better?



Comment at: clang/lib/AST/ExprConstant.cpp:692
+ * E = Eagerly evaluate certain builtins to a value which would 
normally
+   defer until after optimizations.
+

rsmith wrote:
> defer -> be deferred?
Done.



Comment at: clang/lib/AST/ExprConstant.cpp:706-707
+
+ TODO: Fix EM_ConstantExpression and EM_PotentialConstantExpression to
+ also eagerly evaluate. (and then delete
+ EM_PotentialConstantExpressionUnevaluated as a duplicate)

rsmith wrote:
> Allowing `EM_ConstantExpression` to evaluate expressions that 
> `EM_ConstantFold` cannot evaluate would violate our invariants. We rely on 
> being able to check up front that an expression is a constant expression and 
> then later just ask for its value, and for the later query to always succeed 
> and return the same value as the earlier one. (This is one of the reasons why 
> I suggested in D52854 that we add an explicit AST representation for constant 
> expressions.)
I agree, it would right now. But I do think it should be fixed not to in the 
future. Let me reword the TODO, noting that it's not trivial.



Comment at: clang/lib/AST/ExprConstant.cpp:711
+
+  /// Evaluate as a constant expression, as per C++11-and-later constexpr
+  /// rules. Stop if we find that the expression is not a constant

rsmith wrote:
> This is not the intent. The evaluator uses the rules of the current language 
> mode. However, it doesn't enforce the C and C++98 syntactic restrictions on 
> what can appear in a constant expression, because it turns out to be cleaner 
> to check those separately rather than to interleave them with evaluation.
> 
> We should probably document this as evaluating following the evaluation (but 
> not syntactic) constant expression rules of the current language mode.
I'm not really sure what you mean to distinguish between C/C++98 "evaluation 
constant expression rules" and the definition of a C/C++98 ICE?

Perhaps you mean that if evaluation succeeds, it will have produced a value 
consistent with the rules of the current language, but that it may not produce 
the diagnostics in all cases it ought to?

AFAICT, the evaluator is designed to emit diagnostics only for the C++11 (and 
later) constant expression rules, as well as for inability to e

[PATCH] D53081: [clang-doc] Add unit tests for serialization

2018-10-14 Thread Petr Hosek via Phabricator via cfe-commits 
phosek added inline comments.



Comment at: clang-tools-extra/unittests/clang-doc/SerializeTest.cpp:27
+comments::FullComment *
+getComment(const NamedDecl *D) const {
+  if (RawComment *Comment = D->getASTContext().getRawCommentForDeclNoCache(D)) 
{

Nit: this function seems to be wrongly indented.



Comment at: clang-tools-extra/unittests/clang-doc/SerializeTest.cpp:40
+  bool VisitNamespaceDecl(const NamespaceDecl *D) {
+auto I = serialize::emitInfo(D, getComment(D), /*Line*/ 0,
+ /*File=*/"test.cpp", Public);

Nit: this should be `/*Line=*/` for consistency, the same below.


https://reviews.llvm.org/D53081



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

[PATCH] D53085: [clang-doc] Add unit tests for Markdown

2018-10-14 Thread Petr Hosek via Phabricator via cfe-commits 
phosek added inline comments.



Comment at: clang-tools-extra/unittests/clang-doc/MDGeneratorTest.cpp:362
+} // namespace clang
\ No newline at end of file


Nit: this is relevant.


https://reviews.llvm.org/D53085



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

[PATCH] D53150: [clang-doc] Limit integration tests

2018-10-14 Thread 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


https://reviews.llvm.org/D53150



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

[PATCH] D32905: [Analyzer] Iterator Checker - Part 9: Evaluation of std::find-like calls

2018-10-14 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.
Herald added a subscriber: donat.nagy.



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1756-1760
+  auto stateFound = state->BindExpr(CE, LCtx, RetVal);
+  auto stateNotFound = state->BindExpr(CE, LCtx, SecondParam);
+
+  C.addTransition(stateFound);
+  C.addTransition(stateNotFound);

Right now i don't think we're ready for finding a good generalized way of 
describing C++ method summaries. Even the mechanism in the C library checker is 
vry limited. In C++ i'd put a good generalized model for dealing with 
opaque symbolic structures as an important pre-requisite. But if you have 
anything specific in mind, i'd be excited to hear what you propose.

Also i still believe that state split is incorrect and leads to inevitable 
false positives even when all the information that's necessary to prevent these 
false positives is in fact available to the Analyzer. And for that reason i 
hesitate to put this logic into a checker that most people will need to enable 
by default.

On the other hand, because you support `==` and `!=`, these false positives 
might be suppress-able with `assert`s. So if they're rare, they're probably not 
that bad. But i'd still put it under a flag.

Actually, yeah, emptiness of the container might be pretty easy to model. It'll 
let you return `.end()` when `.begin()` is called, etc. You can declare that 
the container is empty when it's freshly default-constructed or a fresh return 
value of `.empty()` is assumed to be true. That's not enough to fix `.find()` 
modeling, just random thoughts.


https://reviews.llvm.org/D32905



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

[PATCH] D32905: [Analyzer] Iterator Checker - Part 9: Evaluation of std::find-like calls

2018-10-14 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

In https://reviews.llvm.org/D32906#1264452, @baloghadamsoftware wrote:

> Unfortunately, we are at the beginning of a long road. I will post several 
> new patches that we already test internally. However the only checker with 
> acceptable false-positive rate is the `invalidated-iterator` checker. The 
> `mismatched-iterator` still has high false-positive rate for 
> iterator-iterator mismatches. For iterator-container mismatches it is 
> acceptable. The `iterator-range` checker also has still too many false 
> positives.


These 1.5 checkers are a lot, in my opinion. They're preventing undefined 
behavior that can be very easy to introduce accidentally. I think they're very 
valuable. Non-powerusers would love to have these checkers turned on by 
default, and while this is entirely up to you to decide, my vague idea of the 
greater good suggests that focusing on converging to something 
not-feature-rich-but-deliverable and delivering that something to the users is 
a good thing to do, especially given that it seems almost ready, while there's 
a long road ahead to finish other checkers.

Like, if you have 3 tasks and spend 1/3 of your time on each of them, all of 
them will be done in 3 units of time, but if you first spend 1 unit of time on 
the first task, then 1 unit of time on the second task, then 1 unit of time on 
the third task, then even though time to complete all three tasks remains at 3 
units you have the benefit of the first and the second task being completed and 
starting to bring benefit much earlier.


https://reviews.llvm.org/D32905



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

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

2018-10-14 Thread John McCall via Phabricator via cfe-commits 
rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D50616



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

[PATCH] D53266: [clangd] Simplify client capabilities parsing.

2018-10-14 Thread Sam McCall via Phabricator via cfe-commits 
sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: cfe-commits, arphaman, jkorous, MaskRay, ioeric, 
ilya-biryukov.

Instead of parsing into structs that mirror LSP, simply parse into a flat struct
that contains the info we need.
This is an exception to our strategy with Protocol.h, which seems justified:

- the structure here is very large and deeply nested
- we care about almost none of it
- we should never have to serialize client capabilities


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53266

Files:
  clangd/ClangdLSPServer.cpp
  clangd/Protocol.cpp
  clangd/Protocol.h

Index: clangd/Protocol.h
===
--- clangd/Protocol.h
+++ clangd/Protocol.h
@@ -221,18 +221,6 @@
   Incremental = 2,
 };
 
-struct CompletionItemClientCapabilities {
-  /// Client supports snippets as insert text.
-  bool snippetSupport = false;
-  /// Client supports commit characters on a completion item.
-  bool commitCharacterSupport = false;
-  // Client supports the follow content formats for the documentation property.
-  // The order describes the preferred format of the client.
-  // NOTE: not used by clangd at the moment.
-  // std::vector documentationFormat;
-};
-bool fromJSON(const llvm::json::Value &, CompletionItemClientCapabilities &);
-
 /// The kind of a completion entry.
 enum class CompletionItemKind {
   Missing = 0,
@@ -263,58 +251,16 @@
   TypeParameter = 25,
 };
 bool fromJSON(const llvm::json::Value &, CompletionItemKind &);
-
-struct CompletionItemKindCapabilities {
-  /// The CompletionItemKinds that the client supports. If not set, the client
-  /// only supports <= CompletionItemKind::Reference and will not fall back to a
-  /// valid default value.
-  llvm::Optional> valueSet;
-};
-// Discards unknown CompletionItemKinds.
-bool fromJSON(const llvm::json::Value &, std::vector &);
-bool fromJSON(const llvm::json::Value &, CompletionItemKindCapabilities &);
-
 constexpr auto CompletionItemKindMin =
 static_cast(CompletionItemKind::Text);
 constexpr auto CompletionItemKindMax =
 static_cast(CompletionItemKind::TypeParameter);
 using CompletionItemKindBitset = std::bitset;
+bool fromJSON(const llvm::json::Value &, CompletionItemKindBitset &);
 CompletionItemKind
 adjustKindToCapability(CompletionItemKind Kind,
CompletionItemKindBitset &supportedCompletionItemKinds);
 
-struct CompletionClientCapabilities {
-  /// Whether completion supports dynamic registration.
-  bool dynamicRegistration = false;
-  /// The client supports the following `CompletionItem` specific capabilities.
-  CompletionItemClientCapabilities completionItem;
-  /// The CompletionItemKinds that the client supports. If not set, the client
-  /// only supports <= CompletionItemKind::Reference and will not fall back to a
-  /// valid default value.
-  llvm::Optional completionItemKind;
-
-  /// The client supports to send additional context information for a
-  /// `textDocument/completion` request.
-  bool contextSupport = false;
-};
-bool fromJSON(const llvm::json::Value &, CompletionClientCapabilities &);
-
-struct PublishDiagnosticsClientCapabilities {
-  // Whether the client accepts diagnostics with related information.
-  // NOTE: not used by clangd at the moment.
-  // bool relatedInformation;
-
-  /// Whether the client accepts diagnostics with fixes attached using the
-  /// "clangd_fixes" extension.
-  bool clangdFixSupport = false;
-
-  /// Whether the client accepts diagnostics with category attached to it
-  /// using the "category" extension.
-  bool categorySupport = false;
-};
-bool fromJSON(const llvm::json::Value &,
-  PublishDiagnosticsClientCapabilities &);
-
 /// A symbol kind.
 enum class SymbolKind {
   File = 1,
@@ -344,58 +290,40 @@
   Operator = 25,
   TypeParameter = 26
 };
-
+bool fromJSON(const llvm::json::Value &, SymbolKind &);
 constexpr auto SymbolKindMin = static_cast(SymbolKind::File);
 constexpr auto SymbolKindMax = static_cast(SymbolKind::TypeParameter);
 using SymbolKindBitset = std::bitset;
-
-bool fromJSON(const llvm::json::Value &, SymbolKind &);
-
-struct SymbolKindCapabilities {
-  /// The SymbolKinds that the client supports. If not set, the client only
-  /// supports <= SymbolKind::Array and will not fall back to a valid default
-  /// value.
-  llvm::Optional> valueSet;
-};
-// Discards unknown SymbolKinds.
-bool fromJSON(const llvm::json::Value &, std::vector &);
-bool fromJSON(const llvm::json::Value &, SymbolKindCapabilities &);
+bool fromJSON(const llvm::json::Value &, SymbolKindBitset &);
 SymbolKind adjustKindToCapability(SymbolKind Kind,
   SymbolKindBitset &supportedSymbolKinds);
 
-struct WorkspaceSymbolCapabilities {
-  /// Capabilities SymbolKind.
-  llvm::Optional symbolKind;
-};
-bool fromJSON(const llvm::json::Value &, WorkspaceSymbolCapabilities &);
-
-// FIXME: most of the capabili

[PATCH] D53213: [clangd] Send CodeAction responses to textDocument/codeAction (LSP 3.8)

2018-10-14 Thread Sam McCall via Phabricator via cfe-commits 
sammccall added inline comments.



Comment at: clangd/ClangdLSPServer.cpp:338
+  Command Cmd;
+  if (Action.command && Action.edit)
+return llvm::None;

kadircet wrote:
> What would you think about emitting two commands in this case? First the edit 
> and then the command. I believe LSP doesn't specify any ordering on how the 
> commands returned should be executed by the client, so I am OK with current 
> state as well. Just wanted to know if there were any other concerns.
That doesn't have the right semantics. Multiple commands returned from 
`textDocument/codeAction` are alternatives that the user should select between, 
whereas having both an edit and a command means they should be performed 
atomically as one action.

(This never actually happens as we don't emit such actions, added a comment)



Comment at: clangd/ClangdLSPServer.cpp:350
+  if (Action.kind && *Action.kind == CodeAction::QUICKFIX_KIND)
+Cmd.title = "Apply fix: " + Cmd.title;
+  return Cmd;

kadircet wrote:
> It seems we only prepend title with Apply fix when we fallback, I believe it 
> would be better to add them in CodeAction instead?
The `CodeAction` has a slot to describe the type of action: if the client wants 
to prepend "Quick Fix: " or so it can.
(Personally this just seems like noise to me, so I'd rather the client omit it, 
but...)



Comment at: clangd/ClangdLSPServer.cpp:355
 void ClangdLSPServer::onCodeAction(CodeActionParams &Params) {
   // We provide a code action for each diagnostic at the requested location
   // which has FixIts available.

kadircet wrote:
> I believe this comment is misleading, do we perform any location check? Maybe 
> change that to say "requested file"?
Updated the comment.



Comment at: clangd/Protocol.h:390
+  /// Flattened from codeAction.codeActionLiteralSupport.
+  // FIXME: flatten other properties in this way.
+  bool codeActionLiteralSupport = false;

kadircet wrote:
> What is the reason behind this one? Is it because clients must handle unknown 
> items on their own and fallback to a default one?
> 
> Since that default is client specific, behavior might change from client to 
> client. I agree that clients should be up-to-date with the specs and handle 
> all kinds of items but these might still create confusions during the 
> transition period.
> 
> For example, ycmd decided to fallback to None instead of Text when they don't 
> know about a symbolkind of a completion item, so users will get to see "File" 
> for the include insertions on both folders and files but when they update to 
> a newer clangd, they will start to see "File" for files and "None" for 
> directory elements. Which I believe might create confusion, but we could 
> still fallback to File for those elements(if we handled them within clangd) 
> and user experience would neither worsen or improve.
> 
> (Currently ycmd's symbolkindcapabilities are actually up-to-date with specs, 
> so this issue wouldn't happen. Just wanted to make my point clearer). 
Sorry, I don't really understand the question.

Are you talking about the default for `codeActionLiteralSupport`? The protocol 
says servers must send `Command`s unless the client indicates support for 
`CodeAction`s. There's no room for a different default here.

Or flattening of other properties? That will have no effect on logic, it just 
simplifies the code (see D53266).



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53213



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

[PATCH] D53213: [clangd] Send CodeAction responses to textDocument/codeAction (LSP 3.8)

2018-10-14 Thread Sam McCall via Phabricator via cfe-commits 
sammccall updated this revision to Diff 169650.
sammccall marked an inline comment as done.
sammccall added a comment.

update comment


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53213

Files:
  clangd/ClangdLSPServer.cpp
  clangd/ClangdLSPServer.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/fixits-codeaction.test
  test/clangd/fixits-command.test
  test/clangd/fixits.test
  test/include-fixer/merge.test

Index: test/include-fixer/merge.test
===
--- test/include-fixer/merge.test
+++ test/include-fixer/merge.test
@@ -6,7 +6,7 @@
 Contexts:
   - ContextType: Namespace
 ContextName: a
-FilePath:../include/bar.h
+FilePath:'../include/bar.h'
 Type:Class
 Seen:1
 Used:1
@@ -16,7 +16,7 @@
 Contexts:
   - ContextType: Namespace
 ContextName: a
-FilePath:../include/barbar.h
+FilePath:'../include/barbar.h'
 Type:Class
 Seen:1
 Used:0
Index: test/clangd/fixits.test
===
--- /dev/null
+++ test/clangd/fixits.test
@@ -1,210 +0,0 @@
-# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
-{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}

-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","version":1,"text":"int main(int i, char **a) { if (i = 2) {}}"}}}
-#  CHECK:"method": "textDocument/publishDiagnostics",
-# CHECK-NEXT:  "params": {
-# CHECK-NEXT:"diagnostics": [
-# CHECK-NEXT:  {
-# CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses",
-# CHECK-NEXT:"range": {
-# CHECK-NEXT:  "end": {
-# CHECK-NEXT:"character": 37,
-# CHECK-NEXT:"line": 0
-# CHECK-NEXT:  },
-# CHECK-NEXT:  "start": {
-# CHECK-NEXT:"character": 32,
-# CHECK-NEXT:"line": 0
-# CHECK-NEXT:  }
-# CHECK-NEXT:},
-# CHECK-NEXT:"severity": 2
-# CHECK-NEXT:  }
-# CHECK-NEXT:],
-# CHECK-NEXT:"uri": "file://{{.*}}/foo.c"
-# CHECK-NEXT:  }

-{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":104,"character":13},"end":{"line":0,"character":35}},"context":{"diagnostics":[{"range":{"start": {"line": 0, "character": 32}, "end": {"line": 0, "character": 37}},"severity":2,"message":"Using the result of an assignment as a condition without parentheses"}]}}}
-#  CHECK:  "id": 2,
-# CHECK-NEXT:  "jsonrpc": "2.0",
-# CHECK-NEXT:  "result": [
-# CHECK-NEXT:{
-# CHECK-NEXT:  "arguments": [
-# CHECK-NEXT:{
-# CHECK-NEXT:  "changes": {
-# CHECK-NEXT:"file://{{.*}}/foo.c": [
-# CHECK-NEXT:  {
-# CHECK-NEXT:"newText": "(",
-# CHECK-NEXT:"range": {
-# CHECK-NEXT:  "end": {
-# CHECK-NEXT:"character": 32,
-# CHECK-NEXT:"line": 0
-# CHECK-NEXT:  },
-# CHECK-NEXT:  "start": {
-# CHECK-NEXT:"character": 32,
-# CHECK-NEXT:"line": 0
-# CHECK-NEXT:  }
-# CHECK-NEXT:}
-# CHECK-NEXT:  },
-# CHECK-NEXT:  {
-# CHECK-NEXT:"newText": ")",
-# CHECK-NEXT:"range": {
-# CHECK-NEXT:  "end": {
-# CHECK-NEXT:"character": 37,
-# CHECK-NEXT:"line": 0
-# CHECK-NEXT:  },
-# CHECK-NEXT:  "start": {
-# CHECK-NEXT:"character": 37,
-# CHECK-NEXT:"line": 0
-# CHECK-NEXT:  }
-# CHECK-NEXT:}
-# CHECK-NEXT:  }
-# CHECK-NEXT:]
-# CHECK-NEXT:  }
-# CHECK-NEXT:}
-# CHECK-NEXT:  ],
-# CHECK-NEXT:  "command": "clangd.applyFix",
-# CHECK-NEXT:  "title": "Apply fix: place parentheses around the assignment to silence this warning"
-# CHECK-NEXT:},
-# CHECK-NEXT:{
-# CHECK-NEXT:  "arguments": [
-# CHECK-NEXT:{
-# CHECK-NEXT:  "changes": {
-# CHECK-NEXT:"file://{{.*}}/foo.c": [
-# CHECK-NEXT:  {
-# CHECK-NEXT:"newText": "==",
-# CHECK-NEXT:"range": {
-# CHECK-NEXT:  "end": {
-# CHECK-NEXT:"character": 35,
-# CHECK-NEXT:"line": 0
-# CHECK-NEXT:  },
-# CHECK-NEXT:  "start": {
-# CHECK-NEXT:"character": 34,
-# CHECK-NEXT:"line": 0
-# CHECK-NEXT:  }
-# CHECK-NEXT:}
-# CHECK-N

[PATCH] D53220: Remove possibility to change compile database path at runtime

2018-10-14 Thread Sam McCall via Phabricator via cfe-commits 
sammccall added a comment.

Thanks for cleaning this up!




Comment at: clangd/ClangdLSPServer.cpp:433
 
 reparseOpenedFiles();
   }

This isn't needed, the compilation database can only be set during 
initialization.



Comment at: clangd/ClangdLSPServer.h:90
   void reparseOpenedFiles();
+  void applyConfiguration(const ClangdInitializationOptions &Settings);
   void applyConfiguration(const ClangdConfigurationParamsChange &Settings);

Prefer a different name for this function - an overload set should have similar 
semantics and these are quite different (pseudo-constructor vs mutation allowed 
at any time).



Comment at: clangd/Protocol.h:422
+struct ClangdInitializationOptions : public ClangdConfigurationParamsChange {
+  llvm::Optional compilationDatabasePath;
+};

Can we just move this to InitializeParams as a clangd extension?
Doing tricks with inheritance here makes the protocol harder to understand.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53220



___
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

2018-10-14 Thread Takuto Ikuta via Phabricator via cfe-commits 
takuto.ikuta updated this revision to Diff 169652.
takuto.ikuta retitled this revision from "[WIP] Add /Zc:DllexportInlines option 
to clang-cl" to "Add /Zc:DllexportInlines option to clang-cl".
takuto.ikuta added a comment.

Export function inside explicit template instantiation definition


https://reviews.llvm.org/D51340

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/CLCompatOptions.td
  clang/lib/AST/ASTContext.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp

Index: clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/dllexport-no-dllexport-inlines.cpp
@@ -0,0 +1,167 @@
+// RUN: %clang_cc1 %s -fms-extensions -triple x86_64-windows-msvc   \
+// RUN: -fno-dllexport-inlines -emit-llvm -O0 -o - |\
+// RUN: FileCheck --check-prefix=DEFAULT --check-prefix=NOINLINE %s
+
+// RUN: %clang_cc1 %s -fms-extensions -triple x86_64-windows-msvc   \
+// RUN: -emit-llvm -O0 -o - |   \
+// RUN: FileCheck --check-prefix=DEFAULT --check-prefix=INLINE %s
+
+// Function
+
+// DEFAULT-DAG: define dso_local dllexport void @"?NormalFunction@@YAXXZ"()
+void __declspec(dllexport) NormalFunction() {}
+
+
+// DEFAULT-DAG: define weak_odr dso_local dllexport void @"?AlwaysInlineFunction@@YAXXZ"
+__forceinline void __declspec(dllexport) AlwaysInlineFunction() {}
+
+// DEFAULT-DAG: @"?static_variable@?1??AlwaysInlineWithStaticVariableExported@@YAHXZ@4HA" = weak_odr dso_local dllexport global i32 0, comdat, align 4
+__forceinline int __declspec(dllexport) AlwaysInlineWithStaticVariableExported() {
+  static int static_variable = 0;
+  ++static_variable;
+  return static_variable;
+}
+
+// DEFAULT-DAG: @"?static_variable@?1??AlwaysInlineWithStaticVariableImported@@YAHXZ@4HA" = available_externally dllimport global i32 0, align 4
+__forceinline int __declspec(dllimport) AlwaysInlineWithStaticVariableImported() {
+  static int static_variable = 0;
+  ++static_variable;
+  return static_variable;
+}
+
+int ImportedFunctionUser() {
+  return AlwaysInlineWithStaticVariableImported();
+}
+
+// Class member function
+
+// check for local static variables
+// NOINLINE-DAG: @"?static_variable@?1??InclassDefFuncWithStaticVariable@NoTemplateExportedClass@@QEAAHXZ@4HA" = weak_odr dso_local dllexport global i32 0, comdat, align 4
+
+// INLINE-DAG: @"?static_variable@?1??InclassDefFuncWithStaticVariable@NoTemplateExportedClass@@QEAAHXZ@4HA" = weak_odr dso_local dllexport global i32 0, comdat, align 4
+
+// NOINLINE-DAG: @"?static_variable@?1??InClassDefFuncWithStaticVariable@ImportedClass@@QEAAHXZ@4HA" = available_externally dllimport global i32 0, align 4
+
+class __declspec(dllexport) NoTemplateExportedClass {
+ public:
+  // DEFAULT-NOT: NoTemplateExportedClass@NoTemplateExportedClass@@
+  NoTemplateExportedClass() = default;
+
+  // NOINLINE-NOT: InclassDefFunc@NoTemplateExportedClass
+  // INLINE-DAG: define weak_odr dso_local dllexport void @"?InclassDefFunc@NoTemplateExportedClass@@
+  void InclassDefFunc() {}
+
+  int f();
+
+  // DEFAULT-DAG: define weak_odr dso_local dllexport i32 @"?InclassDefFuncWithStaticVariable@NoTemplateExportedClass@@QEAAHXZ"
+  int InclassDefFuncWithStaticVariable() {
+static int static_variable = 0;
+++static_variable;
+return static_variable;
+  }
+
+  // DEFAULT-DAG: define weak_odr dso_local dllexport i32 @"?InclassDefFunctWithLambdaStaticVariable@NoTemplateExportedClass@@QEAAHXZ"
+  int InclassDefFunctWithLambdaStaticVariable() {
+return ([]() { static int static_x; return ++static_x; })();
+  }
+
+  // DEFAULT-NOT: InlineOutclassDefFuncWihtoutDefinition
+  __forceinline void InlineOutclassDefFuncWihtoutDefinition();
+
+  // DEFAULT-NOT: InlineOutclassDefFunc@NoTemplateExportedClass@@
+  __forceinline void InlineOutclassDefFunc();
+
+  // DEFAULT-NOT: InlineOutclassDefFuncWithStaticVariable@NoTemplateExportedClass@@
+  __forceinline int InlineOutclassDefFuncWithStaticVariable();
+
+  // DEFAULT-DAG: define dso_local dllexport void @"?OutclassDefFunc@NoTemplateExportedClass@@QEAAXXZ"
+  void OutclassDefFunc();
+};
+
+void NoTemplateExportedClass::OutclassDefFunc() {}
+
+__forceinline void NoTemplateExportedClass::InlineOutclassDefFunc() {}
+
+__forceinline int NoTemplateExportedClass::InlineOutclassDefFuncWithStaticVariable() {
+  static int static_variable = 0;
+  return ++static_variable;
+}
+
+void __declspec(dllexport) NoTemplateExportedClassUser() {
+  NoTemplateExportedClass a;
+  a.InlineOutclassDefFunc();
+}
+
+template
+class __declspec(dllexport) TemplateExportedClass {
+  void InclassDefFunc() {}
+  void OutclassDefFunc();
+
+  T tem

[PATCH] D53192: [clangd] Do not query index for new name completions.

2018-10-14 Thread Sam McCall via Phabricator via cfe-commits 
sammccall added a comment.

(This looks good, of course the Sema patch needs to land first!)




Comment at: clangd/CodeComplete.cpp:643
   case CodeCompletionContext::CCC_Recovery:
+  // TODO: Provide identifier based completions for the following two contexts:
+  case CodeCompletionContext::CCC_Name:

consider Recovery, NaturalLanguage there too? and maybe Other?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53192



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