[PATCH] D60728: [clang] [test] Add a (xfailing) test for PR41027

[Hans Wennborg via Phabricator via cfe-commits]

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

In D60728#1468713 , @krytarowski wrote:

> In D60728#1468486 , @hans wrote:
>
> > What's the value in checking in this xfail'ed test without an actual fix 
> > for the problem?
>
>
> Raise awareness about the problem.


I don't think that works. No one is reading through the test files of the 
repository.

In D60728#1468868 , @mgorny wrote:

> 1. It may help whoever tries to address it in the future, to have a 
> known-good reproducer.


The usual way to do this is to post it on the bug tracker, which was already 
done.

> 2. If someone addresses this independently and doesn't notice the bug, it 
> will help us get informed that the issue was fixed.

Fair enough, that seems somewhat useful :-)




Comment at: clang/test/Sema/pr41027.c:1
+// XFAIL: *
+// RUN: %clang_cc1 -triple x86_64 -fsyntax-only %s

nit: the XFAIL usually comes after the RUN line, and there's usually an empty 
line between these lines and the other contents of the file


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

https://reviews.llvm.org/D60728



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

[PATCH] D60728: [clang] [test] Add a (xfailing) test for PR41027

[Michał Górny via Phabricator via cfe-commits]

mgorny updated this revision to Diff 195518.
mgorny added a comment.

Updated per request.


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

https://reviews.llvm.org/D60728

Files:
  clang/test/Sema/pr41027.c


Index: clang/test/Sema/pr41027.c
===
--- /dev/null
+++ clang/test/Sema/pr41027.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple x86_64 -fsyntax-only %s
+// XFAIL: *
+
+inline void pr41027(unsigned a, unsigned b) {
+  if (__builtin_constant_p(a)) {
+__asm__ volatile("outl %0,%w1" : : "a"(b), "n"(a));
+  } else {
+__asm__ volatile("outl %0,%w1" : : "a"(b), "d"(a));
+  }
+}


Index: clang/test/Sema/pr41027.c
===
--- /dev/null
+++ clang/test/Sema/pr41027.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple x86_64 -fsyntax-only %s
+// XFAIL: *
+
+inline void pr41027(unsigned a, unsigned b) {
+  if (__builtin_constant_p(a)) {
+__asm__ volatile("outl %0,%w1" : : "a"(b), "n"(a));
+  } else {
+__asm__ volatile("outl %0,%w1" : : "a"(b), "d"(a));
+  }
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60316: [clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 195519.
sammccall marked 3 inline comments as done.
sammccall added a comment.

address review comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60316

Files:
  clangd/index/CanonicalIncludes.cpp
  clangd/index/CanonicalIncludes.h
  clangd/index/SymbolCollector.cpp
  unittests/clangd/CodeCompleteTests.cpp
  unittests/clangd/FileIndexTests.cpp
  unittests/clangd/SymbolCollectorTests.cpp
  unittests/clangd/TestTU.cpp
  unittests/clangd/TestTU.h

Index: unittests/clangd/TestTU.h
===
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -52,6 +52,9 @@
   // Index to use when building AST.
   const SymbolIndex *ExternalIndex = nullptr;
 
+  // Simulate a header guard of the header (using an #import directive).
+  bool ImplicitHeaderGuard = true;
+
   ParsedAST build() const;
   SymbolSlab headerSymbols() const;
   std::unique_ptr index() const;
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -19,13 +19,19 @@
 
 ParsedAST TestTU::build() const {
   std::string FullFilename = testPath(Filename),
-  FullHeaderName = testPath(HeaderFilename);
+  FullHeaderName = testPath(HeaderFilename),
+  ImportThunk = testPath("import_thunk.h");
   std::vector Cmd = {"clang", FullFilename.c_str()};
+  // We want to implicitly include HeaderFilename without messing up offsets.
+  // -include achieves this, but sometimes we want #import (to simulate a header
+  // guard without messing up offsets). In this case, use an intermediate file.
+  std::string ThunkContents = "#import \"" + FullHeaderName + "\"\n";
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
   if (!HeaderCode.empty()) {
 Cmd.push_back("-include");
-Cmd.push_back(FullHeaderName.c_str());
+Cmd.push_back(ImplicitHeaderGuard ? ImportThunk.c_str()
+  : FullHeaderName.c_str());
   }
   Cmd.insert(Cmd.end(), ExtraArgs.begin(), ExtraArgs.end());
   ParseInputs Inputs;
@@ -33,7 +39,9 @@
   Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
   Inputs.CompileCommand.Directory = testRoot();
   Inputs.Contents = Code;
-  Inputs.FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, HeaderCode}});
+  Inputs.FS = buildTestFS({{FullFilename, Code},
+   {FullHeaderName, HeaderCode},
+   {ImportThunk, ThunkContents}});
   Inputs.Opts = ParseOptions();
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
   Inputs.Index = ExternalIndex;
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -20,6 +20,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
+#include "gmock/gmock-more-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -33,7 +34,7 @@
 using testing::_;
 using testing::AllOf;
 using testing::Contains;
-using testing::Eq;
+using testing::ElementsAre;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Not;
@@ -58,6 +59,7 @@
   return StringRef(arg.CanonicalDeclaration.FileURI) == P;
 }
 MATCHER_P(DefURI, P, "") { return StringRef(arg.Definition.FileURI) == P; }
+MATCHER(IncludeHeader, "") { return !arg.IncludeHeaders.empty(); }
 MATCHER_P(IncludeHeader, P, "") {
   return (arg.IncludeHeaders.size() == 1) &&
  (arg.IncludeHeaders.begin()->IncludeHeader == P);
@@ -249,6 +251,8 @@
 TestFileURI = URI::create(TestFileName).toString();
   }
 
+  // Note that unlike TestTU, no automatic header guard is added.
+  // HeaderCode should start with #pragma once to be treated as modular.
   bool runSymbolCollector(llvm::StringRef HeaderCode, llvm::StringRef MainCode,
   const std::vector &ExtraArgs = {}) {
 llvm::IntrusiveRefCntPtr Files(
@@ -268,8 +272,8 @@
 Args, Factory->create(), Files.get(),
 std::make_shared());
 
-InMemoryFileSystem->addFile(TestHeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+InMemoryFileSystem->addFile(
+TestHeaderName, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 InMemoryFileSystem->addFile(TestFileName, 0,
 llvm::MemoryBuffer::getMemBuffer(MainCode));
 Invocation.run();
@@ -920,7 +924,7 @@
 
 TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
   CollectorOpts.CollectIncludePath = true;
-  runSymbolCollector("class Foo {};", /*Main=*/"");
+  runSymbolCollector("#pragma once\nclass 

[PATCH] D60719: Demonstrate how to fix freestanding for memcpy

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

As discussed offline, I think this should go through an RFC process.

I guess the main reservation that people will have is that this might generate 
a very large number of load stores (because we don't have a good way to 
generate loops here). This is not an issue for X86 because of REPMOVS, but it 
would be cool to have the opinion of people familiar with other architectures. 
One way to tackle this issue could be to expand existing `mem*` functions 
before the DAG. This is already happening for `memcmp` in `ExpandMemcmp`, which 
could be made aware of this flag. Similar approaches could be used for other 
`mem*` functions. But eventually this has to be handled here as the dag itself 
can call `getMemcpy()`.

Also, the current approach does not protect against current users and future 
unsavvy users calling `SelectionDAG::getMemcpy` with `AlwaysAlign == false`, so 
what about actually retrieving the flag //within// the function instead of 
outside ? This happens e.g. in `SelectionDAG::visitMemPCpyCall()`m 
`AArch64TargetLowering::LowerCall` and others.




Comment at: clang/test/CodeGen/freestanding-disables-libc.c:6
+
+// NOTE: Test that assembly doesn't call memcpy function in freestanding mode.
+// RUN: %clang_cc1 -triple i386-unknown-unknown -O2 -S %s -o - | grep 'memcpy'

That's the responsibility of LLVM, so this test should be in 
`llvm/test/Codegen`.



Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6093
 
+  assert(MF->getFunction().getParent()->getModuleFlag("force-inline-libc") ==
+ nullptr);

Please add an error message (maybe "modules with 'force-inline-libc' should 
never emit library calls")



Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:5483
 
+static bool IsForceInlineLibc(const SelectionDAG &DAG) {
+  const Module *M = DAG.getMachineFunction().getFunction().getParent();

isForceInlineLibc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60719



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

[PATCH] D58236: Make address space conversions a bit stricter.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 195520.
ebevhan edited the summary of this revision.

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

https://reviews.llvm.org/D58236

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaExpr.cpp
  test/CodeGenOpenCL/numbered-address-space.cl
  test/SemaOpenCL/address-spaces.cl
  test/SemaOpenCL/event_t_overload.cl
  test/SemaOpenCL/numbered-address-space.cl
  test/SemaOpenCL/queue_t_overload.cl

Index: test/SemaOpenCL/queue_t_overload.cl
===
--- test/SemaOpenCL/queue_t_overload.cl
+++ test/SemaOpenCL/queue_t_overload.cl
@@ -7,6 +7,6 @@
   queue_t q;
   foo(q, src1);
   foo(0, src2);
-  foo(q, src3); // expected-error {{call to 'foo' is ambiguous}}
+  foo(q, src3); // expected-error {{no matching function for call to 'foo'}}
   foo(1, src3); // expected-error {{no matching function for call to 'foo'}}
 }
Index: test/SemaOpenCL/numbered-address-space.cl
===
--- test/SemaOpenCL/numbered-address-space.cl
+++ test/SemaOpenCL/numbered-address-space.cl
@@ -26,6 +26,6 @@
 
 void test_generic_as_to_builtin_parameterimplicit_cast_numeric(__attribute__((address_space(3))) int *as3_ptr, float src) {
   generic int* generic_ptr = as3_ptr;
-  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-warning {{incompatible pointer types passing '__generic int *' to parameter of type '__local float *'}}
+  volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-error {{passing '__generic int *' to parameter of type '__local float *' changes address space of pointer}}
 }
 
Index: test/SemaOpenCL/event_t_overload.cl
===
--- test/SemaOpenCL/event_t_overload.cl
+++ test/SemaOpenCL/event_t_overload.cl
@@ -7,5 +7,5 @@
   event_t evt;
   foo(evt, src1);
   foo(0, src2);
-  foo(evt, src3); // expected-error {{call to 'foo' is ambiguous}}
+  foo(evt, src3); // expected-error {{no matching function for call to 'foo'}}
 }
Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -124,6 +124,106 @@
   p = (__private int *)p2;
 }
 
+#if !__OPENCL_CPP_VERSION__
+void nested(__global int *g, __global int * __private *gg, __local int *l, __local int * __private *ll, __global float * __private *gg_f) {
+  g = gg;// expected-error {{assigning '__global int **' to '__global int *' changes address space of pointer}}
+  g = l; // expected-error {{assigning '__local int *' to '__global int *' changes address space of pointer}}
+  g = ll;// expected-error {{assigning '__local int **' to '__global int *' changes address space of pointer}}
+  g = gg_f;  // expected-error {{assigning '__global float **' to '__global int *' changes address space of pointer}}
+  g = (__global int *)gg_f; // expected-error {{casting '__global float **' to type '__global int *' changes address space of pointer}}
+
+  gg = g;// expected-error {{assigning '__global int *' to '__global int **' changes address space of pointer}}
+  gg = l;// expected-error {{assigning '__local int *' to '__global int **' changes address space of pointer}}
+  gg = ll;   // expected-error {{assigning '__local int **' to '__global int **' changes address space of nested pointer}}
+  gg = gg_f; // expected-warning {{incompatible pointer types assigning to '__global int **' from '__global float **'}}
+  gg = (__global int * __private *)gg_f;
+
+  l = g; // expected-error {{assigning '__global int *' to '__local int *' changes address space of pointer}}
+  l = gg;// expected-error {{assigning '__global int **' to '__local int *' changes address space of pointer}}
+  l = ll;// expected-error {{assigning '__local int **' to '__local int *' changes address space of pointer}}
+  l = gg_f;  // expected-error {{assigning '__global float **' to '__local int *' changes address space of pointer}}
+  l = (__local int *)gg_f; // expected-error {{casting '__global float **' to type '__local int *' changes address space of pointer}}
+
+  ll = g;// expected-error {{assigning '__global int *' to '__local int **' changes address space of pointer}}
+  ll = gg;   // expected-error {{assigning '__global int **' to '__local int **' changes address space of nested pointer}}
+  ll = l;// expected-error {{assigning '__local int *' to '__local int **' changes address space of pointer}}
+  ll = gg_f; // expected-error {{assigning '__global float **' to '__local int **' changes address space of nested pointer}}
+  ll = (__local int * __private *)gg_f; // expected-warning {{casting '__global float **' to type '__local int **' discards qualifiers in nested pointer types}}

[PATCH] D60775: [libclang] Expose ext_vector_type

[Sven van Haastregt via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL358566: [libclang] Expose ext_vector_type (authored by 
svenvh, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60775?vs=195373&id=195521#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60775

Files:
  cfe/trunk/bindings/python/clang/cindex.py
  cfe/trunk/include/clang-c/Index.h
  cfe/trunk/test/Index/opencl-types.cl
  cfe/trunk/tools/libclang/CXType.cpp


Index: cfe/trunk/include/clang-c/Index.h
===
--- cfe/trunk/include/clang-c/Index.h
+++ cfe/trunk/include/clang-c/Index.h
@@ -32,7 +32,7 @@
  * compatible, thus CINDEX_VERSION_MAJOR is expected to remain stable.
  */
 #define CINDEX_VERSION_MAJOR 0
-#define CINDEX_VERSION_MINOR 54
+#define CINDEX_VERSION_MINOR 55
 
 #define CINDEX_VERSION_ENCODE(major, minor) ( \
   ((major) * 1)   \
@@ -3315,7 +3315,9 @@
   CXType_OCLIntelSubgroupAVCImeResultDualRefStreamout = 173,
   CXType_OCLIntelSubgroupAVCImeSingleRefStreamin = 174,
 
-  CXType_OCLIntelSubgroupAVCImeDualRefStreamin = 175
+  CXType_OCLIntelSubgroupAVCImeDualRefStreamin = 175,
+
+  CXType_ExtVector = 176
 };
 
 /**
Index: cfe/trunk/test/Index/opencl-types.cl
===
--- cfe/trunk/test/Index/opencl-types.cl
+++ cfe/trunk/test/Index/opencl-types.cl
@@ -17,11 +17,11 @@
 }
 
 // CHECK: VarDecl=scalarHalf:11:8 (Definition){{( \(invalid\))?}} [type=half] 
[typekind=Half] [isPOD=1]
-// CHECK: VarDecl=vectorHalf:12:9 (Definition) [type=half4] [typekind=Typedef] 
[canonicaltype=half __attribute__((ext_vector_type(4)))] 
[canonicaltypekind=Unexposed] [isPOD=1]
+// CHECK: VarDecl=vectorHalf:12:9 (Definition) [type=half4] [typekind=Typedef] 
[canonicaltype=half __attribute__((ext_vector_type(4)))] 
[canonicaltypekind=ExtVector] [isPOD=1]
 // CHECK: VarDecl=scalarFloat:13:9 (Definition) [type=float] [typekind=Float] 
[isPOD=1]
-// CHECK: VarDecl=vectorFloat:14:10 (Definition) [type=float4] 
[typekind=Typedef] [canonicaltype=float __attribute__((ext_vector_type(4)))] 
[canonicaltypekind=Unexposed] [isPOD=1]
+// CHECK: VarDecl=vectorFloat:14:10 (Definition) [type=float4] 
[typekind=Typedef] [canonicaltype=float __attribute__((ext_vector_type(4)))] 
[canonicaltypekind=ExtVector] [isPOD=1]
 // CHECK: VarDecl=scalarDouble:15:10 (Definition){{( \(invalid\))?}} 
[type=double] [typekind=Double] [isPOD=1]
-// CHECK: VarDecl=vectorDouble:16:11 (Definition){{( \(invalid\))?}} 
[type=double4] [typekind=Typedef] [canonicaltype=double 
__attribute__((ext_vector_type(4)))] [canonicaltypekind=Unexposed] [isPOD=1]
+// CHECK: VarDecl=vectorDouble:16:11 (Definition){{( \(invalid\))?}} 
[type=double4] [typekind=Typedef] [canonicaltype=double 
__attribute__((ext_vector_type(4)))] [canonicaltypekind=ExtVector] [isPOD=1]
 
 #pragma OPENCL EXTENSION cl_khr_gl_msaa_sharing : enable
 
Index: cfe/trunk/tools/libclang/CXType.cpp
===
--- cfe/trunk/tools/libclang/CXType.cpp
+++ cfe/trunk/tools/libclang/CXType.cpp
@@ -109,6 +109,7 @@
 TKCASE(VariableArray);
 TKCASE(DependentSizedArray);
 TKCASE(Vector);
+TKCASE(ExtVector);
 TKCASE(MemberPointer);
 TKCASE(Auto);
 TKCASE(Elaborated);
@@ -600,6 +601,7 @@
 TKIND(VariableArray);
 TKIND(DependentSizedArray);
 TKIND(Vector);
+TKIND(ExtVector);
 TKIND(MemberPointer);
 TKIND(Auto);
 TKIND(Elaborated);
@@ -804,6 +806,9 @@
 case Type::Vector:
   ET = cast (TP)->getElementType();
   break;
+case Type::ExtVector:
+  ET = cast(TP)->getElementType();
+  break;
 case Type::Complex:
   ET = cast (TP)->getElementType();
   break;
@@ -827,6 +832,9 @@
 case Type::Vector:
   result = cast (TP)->getNumElements();
   break;
+case Type::ExtVector:
+  result = cast(TP)->getNumElements();
+  break;
 default:
   break;
 }
Index: cfe/trunk/bindings/python/clang/cindex.py
===
--- cfe/trunk/bindings/python/clang/cindex.py
+++ cfe/trunk/bindings/python/clang/cindex.py
@@ -2121,6 +2121,8 @@
 TypeKind.OCLQUEUE = TypeKind(159)
 TypeKind.OCLRESERVEID = TypeKind(160)
 
+TypeKind.EXTVECTOR = TypeKind(176)
+
 class RefQualifierKind(BaseEnumeration):
 """Describes a specific ref-qualifier of a type."""
 


Index: cfe/trunk/include/clang-c/Index.h
===
--- cfe/trunk/include/clang-c/Index.h
+++ cfe/trunk/include/clang-c/Index.h
@@ -32,7 +32,7 @@
  * compatible, thus CINDEX_VERSION_MAJOR is expected to remain stable.
  */
 #define CINDEX_VERSION_MAJOR 0
-#define CINDEX_VERSION_MINOR 54
+#define CINDEX_VERSION_MINOR 5

r358566 - [libclang] Expose ext_vector_type

[Sven van Haastregt via cfe-commits]

Author: svenvh
Date: Wed Apr 17 02:08:50 2019
New Revision: 358566

URL: http://llvm.org/viewvc/llvm-project?rev=358566&view=rev
Log:
[libclang] Expose ext_vector_type

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

Modified:
cfe/trunk/bindings/python/clang/cindex.py
cfe/trunk/include/clang-c/Index.h
cfe/trunk/test/Index/opencl-types.cl
cfe/trunk/tools/libclang/CXType.cpp

Modified: cfe/trunk/bindings/python/clang/cindex.py
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/bindings/python/clang/cindex.py?rev=358566&r1=358565&r2=358566&view=diff
==
--- cfe/trunk/bindings/python/clang/cindex.py (original)
+++ cfe/trunk/bindings/python/clang/cindex.py Wed Apr 17 02:08:50 2019
@@ -2121,6 +2121,8 @@ TypeKind.OCLEVENT = TypeKind(158)
 TypeKind.OCLQUEUE = TypeKind(159)
 TypeKind.OCLRESERVEID = TypeKind(160)
 
+TypeKind.EXTVECTOR = TypeKind(176)
+
 class RefQualifierKind(BaseEnumeration):
 """Describes a specific ref-qualifier of a type."""
 

Modified: cfe/trunk/include/clang-c/Index.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang-c/Index.h?rev=358566&r1=358565&r2=358566&view=diff
==
--- cfe/trunk/include/clang-c/Index.h (original)
+++ cfe/trunk/include/clang-c/Index.h Wed Apr 17 02:08:50 2019
@@ -32,7 +32,7 @@
  * compatible, thus CINDEX_VERSION_MAJOR is expected to remain stable.
  */
 #define CINDEX_VERSION_MAJOR 0
-#define CINDEX_VERSION_MINOR 54
+#define CINDEX_VERSION_MINOR 55
 
 #define CINDEX_VERSION_ENCODE(major, minor) ( \
   ((major) * 1)   \
@@ -3315,7 +3315,9 @@ enum CXTypeKind {
   CXType_OCLIntelSubgroupAVCImeResultDualRefStreamout = 173,
   CXType_OCLIntelSubgroupAVCImeSingleRefStreamin = 174,
 
-  CXType_OCLIntelSubgroupAVCImeDualRefStreamin = 175
+  CXType_OCLIntelSubgroupAVCImeDualRefStreamin = 175,
+
+  CXType_ExtVector = 176
 };
 
 /**

Modified: cfe/trunk/test/Index/opencl-types.cl
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Index/opencl-types.cl?rev=358566&r1=358565&r2=358566&view=diff
==
--- cfe/trunk/test/Index/opencl-types.cl (original)
+++ cfe/trunk/test/Index/opencl-types.cl Wed Apr 17 02:08:50 2019
@@ -17,11 +17,11 @@ void kernel testFloatTypes() {
 }
 
 // CHECK: VarDecl=scalarHalf:11:8 (Definition){{( \(invalid\))?}} [type=half] 
[typekind=Half] [isPOD=1]
-// CHECK: VarDecl=vectorHalf:12:9 (Definition) [type=half4] [typekind=Typedef] 
[canonicaltype=half __attribute__((ext_vector_type(4)))] 
[canonicaltypekind=Unexposed] [isPOD=1]
+// CHECK: VarDecl=vectorHalf:12:9 (Definition) [type=half4] [typekind=Typedef] 
[canonicaltype=half __attribute__((ext_vector_type(4)))] 
[canonicaltypekind=ExtVector] [isPOD=1]
 // CHECK: VarDecl=scalarFloat:13:9 (Definition) [type=float] [typekind=Float] 
[isPOD=1]
-// CHECK: VarDecl=vectorFloat:14:10 (Definition) [type=float4] 
[typekind=Typedef] [canonicaltype=float __attribute__((ext_vector_type(4)))] 
[canonicaltypekind=Unexposed] [isPOD=1]
+// CHECK: VarDecl=vectorFloat:14:10 (Definition) [type=float4] 
[typekind=Typedef] [canonicaltype=float __attribute__((ext_vector_type(4)))] 
[canonicaltypekind=ExtVector] [isPOD=1]
 // CHECK: VarDecl=scalarDouble:15:10 (Definition){{( \(invalid\))?}} 
[type=double] [typekind=Double] [isPOD=1]
-// CHECK: VarDecl=vectorDouble:16:11 (Definition){{( \(invalid\))?}} 
[type=double4] [typekind=Typedef] [canonicaltype=double 
__attribute__((ext_vector_type(4)))] [canonicaltypekind=Unexposed] [isPOD=1]
+// CHECK: VarDecl=vectorDouble:16:11 (Definition){{( \(invalid\))?}} 
[type=double4] [typekind=Typedef] [canonicaltype=double 
__attribute__((ext_vector_type(4)))] [canonicaltypekind=ExtVector] [isPOD=1]
 
 #pragma OPENCL EXTENSION cl_khr_gl_msaa_sharing : enable
 

Modified: cfe/trunk/tools/libclang/CXType.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/libclang/CXType.cpp?rev=358566&r1=358565&r2=358566&view=diff
==
--- cfe/trunk/tools/libclang/CXType.cpp (original)
+++ cfe/trunk/tools/libclang/CXType.cpp Wed Apr 17 02:08:50 2019
@@ -109,6 +109,7 @@ static CXTypeKind GetTypeKind(QualType T
 TKCASE(VariableArray);
 TKCASE(DependentSizedArray);
 TKCASE(Vector);
+TKCASE(ExtVector);
 TKCASE(MemberPointer);
 TKCASE(Auto);
 TKCASE(Elaborated);
@@ -600,6 +601,7 @@ CXString clang_getTypeKindSpelling(enum
 TKIND(VariableArray);
 TKIND(DependentSizedArray);
 TKIND(Vector);
+TKIND(ExtVector);
 TKIND(MemberPointer);
 TKIND(Auto);
 TKIND(Elaborated);
@@ -804,6 +806,9 @@ CXType clang_getElementType(CXType CT) {
 case Type::Vector:
   ET = cast (TP)->getElementType();
   break;
+case Type::ExtVector:
+  ET = cast(TP)->getElementType();
+  break;
 cas

[PATCH] D58291: [clangd] Include textual diagnostic ID as Diagnostic.code.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 195522.
sammccall marked an inline comment as done.
sammccall added a comment.
Herald added a subscriber: dexonsmith.

Rebase to head and expand scope a bit:

- now also setting code for clang-tidy checks
- to enable this to be used from the C++ API, the string code is now on Diag
- and also expose Source over LSP (just an oversight?)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58291

Files:
  clangd/ClangdUnit.cpp
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/diagnostic-category.test
  test/clangd/diagnostics.test
  test/clangd/did-change-configuration-params.test
  test/clangd/execute-command.test
  test/clangd/fixits-codeaction.test
  test/clangd/fixits-command.test
  test/clangd/fixits-embed-in-diagnostic.test
  unittests/clangd/DiagnosticsTests.cpp

Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -12,6 +12,7 @@
 #include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
+#include "clang/Basic/DiagnosticSema.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
@@ -58,7 +59,8 @@
  std::tie(LSPDiag.range, LSPDiag.severity, LSPDiag.message);
 }
 
-MATCHER_P(DiagSource, Source, "") { return arg.S == Source; }
+MATCHER_P(DiagSource, S, "") { return arg.Source == S; }
+MATCHER_P(DiagName, N, "") { return arg.Name == N; }
 
 MATCHER_P(EqualToFix, Fix, "LSP fix " + llvm::to_string(Fix)) {
   if (arg.Message != Fix.Message)
@@ -105,6 +107,7 @@
   AllOf(Diag(Test.range("typo"),
  "use of undeclared identifier 'goo'; did you mean 'foo'?"),
 DiagSource(Diag::Clang),
+DiagName("err_undeclared_var_use_suggest"),
 WithFix(
 Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")),
 // This is a pretty normal range.
@@ -149,7 +152,8 @@
   EXPECT_THAT(TU.build().getDiagnostics(),
   ElementsAre(testing::AllOf(
   Diag(Test.range(), "'not-found.h' file not found"),
-  DiagSource(Diag::Clang;
+  DiagSource(Diag::Clang),
+  DiagName("err_pp_file_not_found";
 }
 
 TEST(DiagnosticsTest, ClangTidy) {
@@ -175,6 +179,7 @@
  "inclusion of deprecated C++ header 'assert.h'; consider "
  "using 'cassert' instead [modernize-deprecated-headers]"),
 DiagSource(Diag::ClangTidy),
+DiagName("modernize-deprecated-headers"),
 WithFix(Fix(Test.range("deprecated"), "",
 "change '\"assert.h\"' to ''"))),
   Diag(Test.range("doubled"),
@@ -185,6 +190,7 @@
"side effects in the 1st macro argument 'X' are repeated in "
"macro expansion [bugprone-macro-repeated-side-effects]"),
   DiagSource(Diag::ClangTidy),
+  DiagName("bugprone-macro-repeated-side-effects"),
   WithNote(Diag(Test.range("macrodef"),
 "macro 'SQUARE' defined here "
 "[bugprone-macro-repeated-side-effects]"))),
@@ -246,6 +252,9 @@
 
 TEST(DiagnosticsTest, ToLSP) {
   clangd::Diag D;
+  D.ID = clang::diag::err_enum_class_reference;
+  D.Name = "err_enum_class_reference";
+  D.Source = clangd::Diag::Clang;
   D.Message = "something terrible happened";
   D.Range = {pos(1, 2), pos(3, 4)};
   D.InsideMainFile = true;
@@ -314,6 +323,10 @@
   LSPDiags,
   ElementsAre(Pair(EqualToLSPDiag(MainLSP), ElementsAre(EqualToFix(F))),
   Pair(EqualToLSPDiag(NoteInMainLSP), IsEmpty(;
+  EXPECT_EQ(LSPDiags[0].first.code, "err_enum_class_reference");
+  EXPECT_EQ(LSPDiags[0].first.source, "clang");
+  EXPECT_EQ(LSPDiags[1].first.code, "");
+  EXPECT_EQ(LSPDiags[1].first.source, "");
 }
 
 struct SymbolWithHeader {
Index: test/clangd/fixits-embed-in-diagnostic.test
===
--- test/clangd/fixits-embed-in-diagnostic.test
+++ test/clangd/fixits-embed-in-diagnostic.test
@@ -6,6 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"code": "err_use_with_wrong_tag",
 # CHECK-NEXT:"codeActions": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"edit": {
@@ -42,7 +43,8 @@
 # CHECK-NEXT:"line": 0
 # CHECK-NEXT:  }
 # CHECK-NEXT:},
-# CHECK-NEXT:"severity": 1
+# CHECK-NEXT:"severity": 1,
+# CHECK-NEXT:"source": "clang"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  {
 # CHECK-NEXT:"message": "Previo

[PATCH] D58291: [clangd] Include textual diagnostic ID as Diagnostic.code.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added a comment.

In D58291#1400569 , @kadircet wrote:

> LG but is this information really useful to users? According to LSP `The 
> diagnostic's code, which might appear in the user interface.`, I think seeing 
> this will be mostly noise for users.


It's a good question, it depends how this is surfaced, and we may want to tweak 
the behavior or suppress entirely in some cases.
I think at least some are useful:

- clang-tidy check names are things users need to know about (used for 
configuration)
- for warnings, we quite likely should replace with the most specific warning 
category (e.g. "unreachable-code-loop-increment"), again these are used for 
configuration (-W)
- for others, maybe we should at least trim the err_ prefix, or maybe drop them 
entirely.




Comment at: clangd/Diagnostics.cpp:39
+#include "clang/Basic/DiagnosticCommentKinds.inc"
+#include "clang/Basic/DiagnosticSemaKinds.inc"
+#include "clang/Basic/DiagnosticAnalysisKinds.inc"

kadircet wrote:
> I suppose `CrossTUKinds` is left out intentionally ?
Yeah, this isn't really part of clang, and seems to be part of static analyzer.
Some places include it and others don't...



Comment at: clangd/Diagnostics.cpp:281
+if (auto* Name = getDiagnosticCode(D.ID))
+  Main.code = Name;
 if (Opts.EmbedFixesInDiagnostics) {

jkorous wrote:
> It seems to me that in case `ID` is undefined (hits the default case in 
> `getDiagnosticCode`) we are calling `std::string` non-explicit constructor 
> with `nullptr` which is UB.
> How about changing `char* getDiagnosticCode` to `Optional 
> getDiagnosticCode` or similar to make it less error-prone?
This is an if statement - if the pointer is null we never assign to std::string.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58291



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

[PATCH] D58291: [clangd] Include textual diagnostic ID as Diagnostic.code.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added a comment.

In D58291#1469880 , @sammccall wrote:

> It's a good question, it depends how this is surfaced, and we may want to 
> tweak the behavior or suppress entirely in some cases.
>  I think at least some are useful:
>
> - clang-tidy check names are things users need to know about (used for 
> configuration)
> - for warnings, we quite likely should replace with the most specific warning 
> category (e.g. "unreachable-code-loop-increment"), again these are used for 
> configuration (-W)
> - for others, maybe we should at least trim the err_ prefix, or maybe drop 
> them entirely.


I see, I believe we can decide on what tweaks to perform after landing this 
patch and seeing behaviors in different editors, but up to you.

As for the lit-tests, it would be great to have a diag with source clang-tidy 
if it is not too much of a hustle.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58291



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

r358570 - clang-cl: Parse /openmp:experimental

[Hans Wennborg via cfe-commits]

Author: hans
Date: Wed Apr 17 03:05:58 2019
New Revision: 358570

URL: http://llvm.org/viewvc/llvm-project?rev=358570&view=rev
Log:
clang-cl: Parse /openmp:experimental

It was added to the MS docs recently here:
https://github.com/MicrosoftDocs/cpp-docs/commit/3951085ab722fbb488ca40864f4a0553f7b71855

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=358570&r1=358569&r2=358570&view=diff
==
--- cfe/trunk/include/clang/Driver/CLCompatOptions.td (original)
+++ cfe/trunk/include/clang/Driver/CLCompatOptions.td Wed Apr 17 03:05:58 2019
@@ -428,6 +428,7 @@ def _SLASH_kernel : CLFlag<"kernel">;
 def _SLASH_LN : CLFlag<"LN">;
 def _SLASH_MP : CLJoined<"MP">;
 def _SLASH_openmp : CLFlag<"openmp">;
+def _SLASH_openmp_experimental : CLFlag<"openmp:experimental">;
 def _SLASH_Qfast_transcendentals : CLFlag<"Qfast_transcendentals">;
 def _SLASH_QIfist : CLFlag<"QIfist">;
 def _SLASH_Qimprecise_fwaits : CLFlag<"Qimprecise_fwaits">;

Modified: cfe/trunk/test/Driver/cl-options.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/cl-options.c?rev=358570&r1=358569&r2=358570&view=diff
==
--- cfe/trunk/test/Driver/cl-options.c (original)
+++ cfe/trunk/test/Driver/cl-options.c Wed Apr 17 03:05:58 2019
@@ -442,6 +442,7 @@
 // RUN: /o foo.obj \
 // RUN: /ofoo.obj \
 // RUN: /openmp \
+// RUN: /openmp:experimental \
 // RUN: /Qfast_transcendentals \
 // RUN: /QIfist \
 // RUN: /Qimprecise_fwaits \


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

[PATCH] D60719: Demonstrate how to fix freestanding for memcpy

[Tim Northover via Phabricator via cfe-commits]

t.p.northover added a comment.

I think it'd be pretty unpopular with the people I know who use freestanding. 
They're mostly working on microcontrollers and compiling -Oz so the extra code 
size would be untenable; they also have memcpy implementations anyway because 
they use it in their own code.

If I was trying to solve this (and I'm also not 100% sure it needs solving), I 
think I'd instead generate a `linkonce` definition of `memcpy` whenever it's 
needed and leave CodeGen unmodified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60719



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

[PATCH] D60316: [clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL358571: [clangd] Include insertion: require header guards, 
drop other heuristics, treat… (authored by sammccall, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60316?vs=195519&id=195531#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60316

Files:
  clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h
  clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
  clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
  clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
  clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
  clang-tools-extra/trunk/unittests/clangd/TestTU.h

Index: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
@@ -20,6 +20,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
+#include "gmock/gmock-more-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
@@ -33,7 +34,7 @@
 using testing::_;
 using testing::AllOf;
 using testing::Contains;
-using testing::Eq;
+using testing::ElementsAre;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Not;
@@ -58,6 +59,7 @@
   return StringRef(arg.CanonicalDeclaration.FileURI) == P;
 }
 MATCHER_P(DefURI, P, "") { return StringRef(arg.Definition.FileURI) == P; }
+MATCHER(IncludeHeader, "") { return !arg.IncludeHeaders.empty(); }
 MATCHER_P(IncludeHeader, P, "") {
   return (arg.IncludeHeaders.size() == 1) &&
  (arg.IncludeHeaders.begin()->IncludeHeader == P);
@@ -249,6 +251,8 @@
 TestFileURI = URI::create(TestFileName).toString();
   }
 
+  // Note that unlike TestTU, no automatic header guard is added.
+  // HeaderCode should start with #pragma once to be treated as modular.
   bool runSymbolCollector(llvm::StringRef HeaderCode, llvm::StringRef MainCode,
   const std::vector &ExtraArgs = {}) {
 llvm::IntrusiveRefCntPtr Files(
@@ -920,7 +924,7 @@
 
 TEST_F(SymbolCollectorTest, IncludeHeaderSameAsFileURI) {
   CollectorOpts.CollectIncludePath = true;
-  runSymbolCollector("class Foo {};", /*Main=*/"");
+  runSymbolCollector("#pragma once\nclass Foo {};", /*Main=*/"");
   EXPECT_THAT(Symbols, UnorderedElementsAre(
AllOf(QName("Foo"), DeclURI(TestHeaderURI;
   EXPECT_THAT(Symbols.begin()->IncludeHeaders,
@@ -1020,53 +1024,54 @@
 
 TEST_F(SymbolCollectorTest, MainFileIsHeaderWhenSkipIncFile) {
   CollectorOpts.CollectIncludePath = true;
-  CanonicalIncludes Includes;
-  CollectorOpts.Includes = &Includes;
-  TestFileName = testPath("main.h");
+  // To make this case as hard as possible, we won't tell clang main is a
+  // header. No extension, no -x c++-header.
+  TestFileName = testPath("no_ext_main");
   TestFileURI = URI::create(TestFileName).toString();
   auto IncFile = testPath("test.inc");
   auto IncURI = URI::create(IncFile).toString();
   InMemoryFileSystem->addFile(IncFile, 0,
   llvm::MemoryBuffer::getMemBuffer("class X {};"));
-  runSymbolCollector("", /*Main=*/"#include \"test.inc\"",
+  runSymbolCollector("", R"cpp(
+// Can't use #pragma once in a main file clang doesn't think is a header.
+#ifndef MAIN_H_
+#define MAIN_H_
+#include "test.inc"
+#endif
+  )cpp",
  /*ExtraArgs=*/{"-I", testRoot()});
   EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("X"), DeclURI(IncURI),
   IncludeHeader(TestFileURI;
 }
 
-TEST_F(SymbolCollectorTest, MainFileIsHeaderWithoutExtensionWhenSkipIncFile) {
+TEST_F(SymbolCollectorTest, IncFileInNonHeader) {
   CollectorOpts.CollectIncludePath = true;
-  CanonicalIncludes Includes;
-  CollectorOpts.Includes = &Includes;
-  TestFileName = testPath("no_ext_main");
+  TestFileName = testPath("main.cc");
   TestFileURI = URI::create(TestFileName).toString();
   auto IncFile = testPath("test.inc");
   auto IncURI = URI::create(IncFile).toString();
   InMemoryFileSystem->addFile(IncFile, 0,
   llvm::MemoryBuffer::getMemBuffer("class X {};"));
-  runSymbolCollector("", /*Main=*/"#include \"test.inc\"",
+  runSymbolCollector("", R"cpp(
+#include "test.inc"
+  )cpp",
  /*ExtraArgs=*/{"-I", testRoot()});
   EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("X"), DeclURI(IncURI),
-  IncludeHeader(TestFileURI;
+   

[clang-tools-extra] r358571 - [clangd] Include insertion: require header guards, drop other heuristics, treat .def like .inc.

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Apr 17 03:36:02 2019
New Revision: 358571

URL: http://llvm.org/viewvc/llvm-project?rev=358571&view=rev
Log:
[clangd] Include insertion: require header guards, drop other heuristics, treat 
.def like .inc.

Summary:
We do have some reports of include insertion behaving badly in some
codebases. Requiring header guards both makes sense in principle, and is
likely to disable this "nice-to-have" feature in codebases where headers don't
follow the expected pattern.

With this we can drop some other heuristics, such as looking at file
extensions to detect known non-headers - implementation files have no guards.

One wrinkle here is #import - objc headers may not have guards because
they're intended to be used via #import. If the header is the main file
or is #included, we won't collect locations - merge should take care of
this if we see the file #imported somewhere. Seems likely to be OK.

Headers which have a canonicalization (stdlib, IWYU) are exempt from this check.
*.inc files continue to be handled by looking up to the including file.
This patch also adds *.def here - tablegen wants this pattern too.

In terms of code structure, the division between SymbolCollector and
CanonicalIncludes has shifted: SymbolCollector is responsible for more.
This is because SymbolCollector has all the SourceManager/HeaderSearch access
needed for checking for guards, and we interleave these checks with the *.def
checks in a loop (potentially).
We could hand all the info into CanonicalIncludes and put the logic there
if that's preferable.

Reviewers: ioeric

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

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

Modified:
clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp
clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
clang-tools-extra/trunk/unittests/clangd/TestTU.h

Modified: clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp?rev=358571&r1=358570&r2=358571&view=diff
==
--- clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/CanonicalIncludes.cpp Wed Apr 17 
03:36:02 2019
@@ -37,31 +37,12 @@ void CanonicalIncludes::addSymbolMapping
 }
 
 llvm::StringRef
-CanonicalIncludes::mapHeader(llvm::ArrayRef Headers,
+CanonicalIncludes::mapHeader(llvm::StringRef Header,
  llvm::StringRef QualifiedName) const {
-  assert(!Headers.empty());
+  assert(!Header.empty());
   auto SE = SymbolMapping.find(QualifiedName);
   if (SE != SymbolMapping.end())
 return SE->second;
-  // Find the first header such that the extension is not '.inc', and isn't a
-  // recognized non-header file
-  auto I = llvm::find_if(Headers, [](llvm::StringRef Include) {
-// Skip .inc file whose including header file should
-// be #included instead.
-return !Include.endswith(".inc");
-  });
-  if (I == Headers.end())
-return Headers[0]; // Fallback to the declaring header.
-  llvm::StringRef Header = *I;
-  // If Header is not expected be included (e.g. .cc file), we fall back to
-  // the declaring header.
-  llvm::StringRef Ext = llvm::sys::path::extension(Header).trim('.');
-  // Include-able headers must have precompile type. Treat files with
-  // non-recognized extenstions (TY_INVALID) as headers.
-  auto ExtType = driver::types::lookupTypeForExtension(Ext);
-  if ((ExtType != driver::types::TY_INVALID) &&
-  !driver::types::onlyPrecompileType(ExtType))
-return Headers[0];
 
   auto MapIt = FullPathMapping.find(Header);
   if (MapIt != FullPathMapping.end())

Modified: clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h?rev=358571&r1=358570&r2=358571&view=diff
==
--- clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h (original)
+++ clang-tools-extra/trunk/clangd/index/CanonicalIncludes.h Wed Apr 17 
03:36:02 2019
@@ -50,9 +50,9 @@ public:
 llvm::StringRef CanonicalPath);
 
   /// Returns the canonical include for symbol with \p QualifiedName.
-  /// \p Headers is the include stack: Headers.front() is the file declaring 
the
-  /// symbol, and Headers.back() is the main file.
-  llvm::StringRef mapHeader(llvm::ArrayRef Headers,
+  /// \p Header is the file the declaration was reachable from.
+  /// Hea

[PATCH] D60815: [clangd] Recognize "don't include me directly" pattern, and suppress include insertion.

[Sam McCall via Phabricator via cfe-commits]

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

Typically used with umbrella headers, e.g. GTK:

#if !defined (__GTK_H_INSIDE__) && !defined (GTK_COMPILATION)
 #error "Only  can be included directly."
 #endif

Heuristic is fairly conservative, a quick code search over github showed
a fair number of hits and few/no false positives. (Not all were umbrella
headers, but I'd be happy avoiding include insertion for all of them).

We may want to relax the heuristic later to catch more cases.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D60815

Files:
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -1064,8 +1064,19 @@
   auto TU = TestTU::withHeaderCode("int x();");
   EXPECT_THAT(TU.headerSymbols(), ElementsAre(IncludeHeader()));
 
+  // Files missing include guards aren't eligible for insertion.
   TU.ImplicitHeaderGuard = false;
   EXPECT_THAT(TU.headerSymbols(), ElementsAre(Not(IncludeHeader(;
+
+  // We recognize some patterns of trying to prevent insertion.
+  TU = TestTU::withHeaderCode(R"cpp(
+#ifndef SECRET
+#error "This file isn't safe to include directly"
+#endif
+int x();
+)cpp");
+  TU.ExtraArgs.push_back("-DSECRET"); // *we're* able to include it.
+  EXPECT_THAT(TU.headerSymbols(), ElementsAre(Not(IncludeHeader(;
 }
 
 TEST_F(SymbolCollectorTest, AvoidUsingFwdDeclsAsCanonicalDecls) {
Index: clangd/index/SymbolCollector.h
===
--- clangd/index/SymbolCollector.h
+++ clangd/index/SymbolCollector.h
@@ -19,6 +19,7 @@
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/Regex.h"
 #include 
 
 namespace clang {
@@ -117,6 +118,15 @@
bool IsMainFileSymbol);
   void addDefinition(const NamedDecl &, const Symbol &DeclSymbol);
 
+  llvm::Optional getIncludeHeader(llvm::StringRef QName, FileID);
+  bool isSelfContainedHeader(FileID);
+  // Heuristic to detect headers that aren't self-contained, usually because
+  // they need to be included via an umbrella header. e.g. GTK matches this.
+  llvm::Regex DontIncludeMe = {
+  "^[ \t]*#[ \t]*if.*\n" // An #if, #ifndef etc directive, then
+  "[ \t]*#[ \t]*error.*include", // an #error directive mentioning "include"
+  llvm::Regex::Newline};
+
   // All Symbols collected from the AST.
   SymbolSlab::Builder Symbols;
   // All refs collected from the AST.
@@ -141,6 +151,7 @@
   llvm::DenseMap CanonicalDecls;
   // Cache whether to index a file or not.
   llvm::DenseMap FilesToIndexCache;
+  llvm::DenseMap HeaderIsSelfContainedCache;
 };
 
 } // namespace clangd
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -128,48 +128,6 @@
   }
 }
 
-bool isSelfContainedHeader(FileID FID, const SourceManager &SM,
-   const Preprocessor &PP) {
-  const FileEntry *FE = SM.getFileEntryForID(FID);
-  if (!FE)
-return false;
-  return PP.getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE);
-}
-
-/// Gets a canonical include (URI of the header or  or "header") for
-/// header of \p FID (which should usually be the *expansion* file).
-/// Returns None if includes should not be inserted for this file.
-llvm::Optional
-getIncludeHeader(llvm::StringRef QName, const SourceManager &SM,
- const Preprocessor &PP, FileID FID,
- const SymbolCollector::Options &Opts) {
-  const FileEntry *FE = SM.getFileEntryForID(FID);
-  if (!FE || FE->getName().empty())
-return llvm::None;
-  llvm::StringRef Filename = FE->getName();
-  // If a file is mapped by canonical headers, use that mapping, regardless
-  // of whether it's an otherwise-good header (header guards etc).
-  if (Opts.Includes) {
-llvm::StringRef Canonical = Opts.Includes->mapHeader(Filename, QName);
-// If we had a mapping, always use it.
-if (Canonical.startswith("<") || Canonical.startswith("\""))
-  return Canonical.str();
-if (Canonical != Filename)
-  return toURI(SM, Canonical, Opts);
-  }
-  if (!isSelfContainedHeader(FID, SM, PP)) {
-// A .inc or .def file is often included into a real header to define
-// symbols (e.g. LLVM tablegen files).
-if (Filename.endswith(".inc") || Filename.endswith(".def"))
-  return getIncludeHeader(QName, SM, PP,
-  SM.getFileID(SM.getIncludeLoc(FID)), Opts);
-// Conservatively refuse to 

[PATCH] D60815: [clangd] Recognize "don't include me directly" pattern, and suppress include insertion.

[Sam McCall via Phabricator via cfe-commits]

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

unconfusing my git repo


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60815

Files:
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -272,8 +272,8 @@
 Args, Factory->create(), Files.get(),
 std::make_shared());
 
-InMemoryFileSystem->addFile(TestHeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+InMemoryFileSystem->addFile(
+TestHeaderName, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 InMemoryFileSystem->addFile(TestFileName, 0,
 llvm::MemoryBuffer::getMemBuffer(MainCode));
 Invocation.run();
@@ -1064,8 +1064,19 @@
   auto TU = TestTU::withHeaderCode("int x();");
   EXPECT_THAT(TU.headerSymbols(), ElementsAre(IncludeHeader()));
 
+  // Files missing include guards aren't eligible for insertion.
   TU.ImplicitHeaderGuard = false;
   EXPECT_THAT(TU.headerSymbols(), ElementsAre(Not(IncludeHeader(;
+
+  // We recognize some patterns of trying to prevent insertion.
+  TU = TestTU::withHeaderCode(R"cpp(
+#ifndef SECRET
+#error "This file isn't safe to include directly"
+#endif
+int x();
+)cpp");
+  TU.ExtraArgs.push_back("-DSECRET"); // *we're* able to include it.
+  EXPECT_THAT(TU.headerSymbols(), ElementsAre(Not(IncludeHeader(;
 }
 
 TEST_F(SymbolCollectorTest, AvoidUsingFwdDeclsAsCanonicalDecls) {
Index: clangd/index/SymbolCollector.h
===
--- clangd/index/SymbolCollector.h
+++ clangd/index/SymbolCollector.h
@@ -19,6 +19,7 @@
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/Regex.h"
 #include 
 
 namespace clang {
@@ -117,6 +118,15 @@
bool IsMainFileSymbol);
   void addDefinition(const NamedDecl &, const Symbol &DeclSymbol);
 
+  llvm::Optional getIncludeHeader(llvm::StringRef QName, FileID);
+  bool isSelfContainedHeader(FileID);
+  // Heuristic to detect headers that aren't self-contained, usually because
+  // they need to be included via an umbrella header. e.g. GTK matches this.
+  llvm::Regex DontIncludeMe = {
+  "^[ \t]*#[ \t]*if.*\n" // An #if, #ifndef etc directive, then
+  "[ \t]*#[ \t]*error.*include", // an #error directive mentioning "include"
+  llvm::Regex::Newline};
+
   // All Symbols collected from the AST.
   SymbolSlab::Builder Symbols;
   // All refs collected from the AST.
@@ -141,6 +151,7 @@
   llvm::DenseMap CanonicalDecls;
   // Cache whether to index a file or not.
   llvm::DenseMap FilesToIndexCache;
+  llvm::DenseMap HeaderIsSelfContainedCache;
 };
 
 } // namespace clangd
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -128,48 +128,6 @@
   }
 }
 
-bool isSelfContainedHeader(FileID FID, const SourceManager &SM,
-   const Preprocessor &PP) {
-  const FileEntry *FE = SM.getFileEntryForID(FID);
-  if (!FE)
-return false;
-  return PP.getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE);
-}
-
-/// Gets a canonical include (URI of the header or  or "header") for
-/// header of \p FID (which should usually be the *expansion* file).
-/// Returns None if includes should not be inserted for this file.
-llvm::Optional
-getIncludeHeader(llvm::StringRef QName, const SourceManager &SM,
- const Preprocessor &PP, FileID FID,
- const SymbolCollector::Options &Opts) {
-  const FileEntry *FE = SM.getFileEntryForID(FID);
-  if (!FE || FE->getName().empty())
-return llvm::None;
-  llvm::StringRef Filename = FE->getName();
-  // If a file is mapped by canonical headers, use that mapping, regardless
-  // of whether it's an otherwise-good header (header guards etc).
-  if (Opts.Includes) {
-llvm::StringRef Canonical = Opts.Includes->mapHeader(Filename, QName);
-// If we had a mapping, always use it.
-if (Canonical.startswith("<") || Canonical.startswith("\""))
-  return Canonical.str();
-if (Canonical != Filename)
-  return toURI(SM, Canonical, Opts);
-  }
-  if (!isSelfContainedHeader(FID, SM, PP)) {
-// A .inc or .def file is often included into a real header to define
-// symbols (e.g. LLVM tablegen files).
-if (Filename.endswith(".inc") || Filename.endswith(".def"))
-  return getIncludeHeader(QName, SM, PP,
-  SM.getFileID(SM.getIncludeLoc(FID)), Opts);
-// Cons

[PATCH] D60815: [clangd] Recognize "don't include me directly" pattern, and suppress include insertion.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 195534.
sammccall marked an inline comment as done.
sammccall added a comment.

remove leftover debugging


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60815

Files:
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -272,8 +272,8 @@
 Args, Factory->create(), Files.get(),
 std::make_shared());
 
-InMemoryFileSystem->addFile(TestHeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+InMemoryFileSystem->addFile(
+TestHeaderName, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 InMemoryFileSystem->addFile(TestFileName, 0,
 llvm::MemoryBuffer::getMemBuffer(MainCode));
 Invocation.run();
@@ -1064,8 +1064,19 @@
   auto TU = TestTU::withHeaderCode("int x();");
   EXPECT_THAT(TU.headerSymbols(), ElementsAre(IncludeHeader()));
 
+  // Files missing include guards aren't eligible for insertion.
   TU.ImplicitHeaderGuard = false;
   EXPECT_THAT(TU.headerSymbols(), ElementsAre(Not(IncludeHeader(;
+
+  // We recognize some patterns of trying to prevent insertion.
+  TU = TestTU::withHeaderCode(R"cpp(
+#ifndef SECRET
+#error "This file isn't safe to include directly"
+#endif
+int x();
+)cpp");
+  TU.ExtraArgs.push_back("-DSECRET"); // *we're* able to include it.
+  EXPECT_THAT(TU.headerSymbols(), ElementsAre(Not(IncludeHeader(;
 }
 
 TEST_F(SymbolCollectorTest, AvoidUsingFwdDeclsAsCanonicalDecls) {
Index: clangd/index/SymbolCollector.h
===
--- clangd/index/SymbolCollector.h
+++ clangd/index/SymbolCollector.h
@@ -19,6 +19,7 @@
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/Regex.h"
 #include 
 
 namespace clang {
@@ -117,6 +118,15 @@
bool IsMainFileSymbol);
   void addDefinition(const NamedDecl &, const Symbol &DeclSymbol);
 
+  llvm::Optional getIncludeHeader(llvm::StringRef QName, FileID);
+  bool isSelfContainedHeader(FileID);
+  // Heuristic to detect headers that aren't self-contained, usually because
+  // they need to be included via an umbrella header. e.g. GTK matches this.
+  llvm::Regex DontIncludeMe = {
+  "^[ \t]*#[ \t]*if.*\n" // An #if, #ifndef etc directive, then
+  "[ \t]*#[ \t]*error.*include", // an #error directive mentioning "include"
+  llvm::Regex::Newline};
+
   // All Symbols collected from the AST.
   SymbolSlab::Builder Symbols;
   // All refs collected from the AST.
@@ -141,6 +151,7 @@
   llvm::DenseMap CanonicalDecls;
   // Cache whether to index a file or not.
   llvm::DenseMap FilesToIndexCache;
+  llvm::DenseMap HeaderIsSelfContainedCache;
 };
 
 } // namespace clangd
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -128,48 +128,6 @@
   }
 }
 
-bool isSelfContainedHeader(FileID FID, const SourceManager &SM,
-   const Preprocessor &PP) {
-  const FileEntry *FE = SM.getFileEntryForID(FID);
-  if (!FE)
-return false;
-  return PP.getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE);
-}
-
-/// Gets a canonical include (URI of the header or  or "header") for
-/// header of \p FID (which should usually be the *expansion* file).
-/// Returns None if includes should not be inserted for this file.
-llvm::Optional
-getIncludeHeader(llvm::StringRef QName, const SourceManager &SM,
- const Preprocessor &PP, FileID FID,
- const SymbolCollector::Options &Opts) {
-  const FileEntry *FE = SM.getFileEntryForID(FID);
-  if (!FE || FE->getName().empty())
-return llvm::None;
-  llvm::StringRef Filename = FE->getName();
-  // If a file is mapped by canonical headers, use that mapping, regardless
-  // of whether it's an otherwise-good header (header guards etc).
-  if (Opts.Includes) {
-llvm::StringRef Canonical = Opts.Includes->mapHeader(Filename, QName);
-// If we had a mapping, always use it.
-if (Canonical.startswith("<") || Canonical.startswith("\""))
-  return Canonical.str();
-if (Canonical != Filename)
-  return toURI(SM, Canonical, Opts);
-  }
-  if (!isSelfContainedHeader(FID, SM, PP)) {
-// A .inc or .def file is often included into a real header to define
-// symbols (e.g. LLVM tablegen files).
-if (Filename.endswith(".inc") || Filename.endswith(".def"))
-  return getIncludeHeader(QName, SM, PP,
-  SM.getFile

[PATCH] D60815: [clangd] Recognize "don't include me directly" pattern, and suppress include insertion.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.



Comment at: clangd/index/SymbolCollector.cpp:602
+/// Returns None if includes should not be inserted for this file.
+llvm::Optional
+SymbolCollector::getIncludeHeader(llvm::StringRef QName, FileID FID) {

this function has been moved to a member so it can call isSelfContainedHeader 
which is now a member.

Apart from no longer having to change so many params, it is unmodified.



Comment at: clangd/index/SymbolCollector.cpp:631
+
+bool SymbolCollector::isSelfContainedHeader(FileID FID) {
+  // The real computation (which will be memoized).

this has been moved to a member so it can use the cache and the 
(non-threadsafe) regex, and its logic has obviously been changed.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60815



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

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D60455#1469150 , @aaron.ballman 
wrote:

> In D60455#1468714 , @bader wrote:
>
> > In D60455#1468386 , @Fznamznon 
> > wrote:
> >
> > > > Ok, my question is whether you are planning to duplicate the same logic 
> > > > as for OpenCL kernel which doesn't really seem like an ideal design 
> > > > choice. Is this the only difference then we can simply add an extra 
> > > > check for SYCL compilation mode in this template handling case. The 
> > > > overall interaction between OpenCL and SYCL implementation is still a 
> > > > very big unknown to me so it's not very easy to judge about the 
> > > > implementations details...
> > >
> > > Of course, if nothing prevents us to re-use OpenCL kernel attribute for 
> > > SYCL I assume it would be good idea. 
> > >  But I'm thinking about the situation with 
> > > https://reviews.llvm.org/D60454 . If we re-use OpenCL kernel attributes - 
> > > we affected by OpenCL-related changes and OpenCL-related changes 
> > > shouldn't violate SYCL semantics. Will it be usable for SYCL/OpenCL clang 
> > > developers? @bader , what do you think about it?
> >
> >
> > I also think it's worth trying. We should be able to cover "SYCL semantics" 
> > with LIT test to avoid regressions by OpenCL related changes. E.g. add a 
> > test case checking that -fsycl-is-device option disables restriction on 
> > applying `__kernel` to template functions.
> >  I want to confirm that everyone is okay to enable `__kernel` keyword for 
> > SYCL extension and cover SYCL use cases with additional regression tests. 
> > IIRC, on yesterday call, @keryell, said that having SYCL specific 
> > attributes useful for separation of concerns.
>
>
> I'm not comfortable with that decision unless the attribute semantics are 
> sufficiently related to justify it. If we're just going to have a lot of 
> `KernelAttr->isSYCL()` vs `KernelAttr->isOpenCL()` accessor calls, it may 
> make more sense to use separate semantic attributes (even if they share 
> spellings), though then I'd be curious how a user combines OpenCL and SYCL 
> attributes.


I am not sure we need to add a keyword actually, the attribute can just be 
added in AST since it's not supposed to be used in the source code? My 
understanding of SYCL kernel is that it mainly matches OpenCL kernel 
functionality because the original intent of SYCL was to provide single source 
functionality on top of OpenCL. But I am not an expert in SYCL to confirm that. 
I think what we are missing currently is a thorough analysis/comparison between 
SYCL device mode and OpenCL kernel language mode to understand what's the best 
implementation strategy. That would apply to many other features: kernel 
function restrictions, address spaces, vectors, special types, etc. I still see 
no point in polluting our code base with extra code that just does the same 
thing. It will save us a lot of time to just work cooperatively on the same 
problem and even improve readability of the code. But of course this can only 
be done if there is no need to diverge the implementation significantly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455



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

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D60455#1468386 , @Fznamznon wrote:

> In D60455#1467018 , @Anastasia wrote:
>
> > Just to understand how this will work. I would imagine you can have a 
> > device function definition preceding its use. When the function is being 
> > parsed it's not known yet whether it will be called from the device or not, 
> > so it won't be possible to set the language mode correctly and hence 
> > provide the right diagnostics. So is the plan to launch a separate parsing 
> > phase then just to extract the call graph and annotate the device functions?
>
>
> I'm not an expert in clang terminology but I will try briefly explain our 
> current implementation approach. 
>  In SYCL all kernel functions should be template functions so these functions 
> have a deferred instantiation. If we found that we instantiated a sycl kernel 
> function - we add it to a special array with sycl device functions (you can 
> see the corresponding code here 
> 
>  and here 
> ,
>  actually `AddSyclKernel` adds declaration to the special array inside the 
> Sema).  After performing
>  pending instantiations we run a recursive AST visitor for each SYCL kernel 
> to mark all device functions and add them to a special array with SYCL device 
> functions (here 
> 
>  we start traverse AST from `MarkDevice` function, code of `MarkDevice` is 
> here 
> ).
>  To get a correct set of SYCL device functions in produced module we added a 
> check for all declarations inside the CodeGen on  `sycl_device` attribute 
> existence - so it will ignore declarations without `sycl_device` attribute if 
> we are compiling for SYCL device (code is here 
> ).
>  But with this check it's possible situation when function was parsed and 
> ignored by the CodeGen before we added `sycl_device` attribute to it so we 
> added yet another parsing action inside the `clang::ParseAST` to generate 
> code for all SYCL device functions from the special array (code is here 
> ).


Thanks for explanation! I would need to spend a bit more time to go through the 
pointers you have provided. Btw just to understand whether the use case with 
externally defined device side functions is covered too? I.e. can you have 
something like this:

  extern void foo();
  [clang::sycl_kernel]] void bar() {
foo();
  }

When `foo` is defined in a separate module that doesn't call it on a device side

  void foo() {
dosomething();
  }

would compiler still be able to detect that this is a device function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455



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

[clang-tools-extra] r358575 - [clangd] Include textual diagnostic ID as Diagnostic.code.

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Apr 17 05:35:16 2019
New Revision: 358575

URL: http://llvm.org/viewvc/llvm-project?rev=358575&view=rev
Log:
[clangd] Include textual diagnostic ID as Diagnostic.code.

Reviewers: kadircet

Subscribers: ilya-biryukov, ioeric, MaskRay, jkorous, arphaman, jdoerfert, 
cfe-commits

Tags: #clang

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

Modified:
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/Diagnostics.cpp
clang-tools-extra/trunk/clangd/Diagnostics.h
clang-tools-extra/trunk/clangd/Protocol.cpp
clang-tools-extra/trunk/clangd/Protocol.h
clang-tools-extra/trunk/test/clangd/compile-commands-path-in-initialize.test
clang-tools-extra/trunk/test/clangd/diagnostic-category.test
clang-tools-extra/trunk/test/clangd/diagnostics.test
clang-tools-extra/trunk/test/clangd/did-change-configuration-params.test
clang-tools-extra/trunk/test/clangd/execute-command.test
clang-tools-extra/trunk/test/clangd/fixits-codeaction.test
clang-tools-extra/trunk/test/clangd/fixits-command.test
clang-tools-extra/trunk/test/clangd/fixits-embed-in-diagnostic.test
clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp

Modified: clang-tools-extra/trunk/clangd/ClangdUnit.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdUnit.cpp?rev=358575&r1=358574&r2=358575&view=diff
==
--- clang-tools-extra/trunk/clangd/ClangdUnit.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp Wed Apr 17 05:35:16 2019
@@ -371,11 +371,7 @@ ParsedAST::build(std::unique_ptrgetPreprocessor().EndSourceFile();
 
-  std::vector Diags = ASTDiags.take();
-  // Populate diagnostic source.
-  for (auto &D : Diags)
-D.S =
-!CTContext->getCheckName(D.ID).empty() ? Diag::ClangTidy : Diag::Clang;
+  std::vector Diags = ASTDiags.take(CTContext.getPointer());
   // Add diagnostics from the preamble, if any.
   if (Preamble)
 Diags.insert(Diags.begin(), Preamble->Diags.begin(), 
Preamble->Diags.end());
@@ -544,8 +540,6 @@ buildPreamble(PathRef FileName, Compiler
 vlog("Built preamble of size {0} for file {1}", BuiltPreamble->getSize(),
  FileName);
 std::vector Diags = PreambleDiagnostics.take();
-for (auto &Diag : Diags)
-  Diag.S = Diag::Clang;
 return std::make_shared(
 std::move(*BuiltPreamble), std::move(Diags),
 SerializedDeclsCollector.takeIncludes(), std::move(StatCache),

Modified: clang-tools-extra/trunk/clangd/Diagnostics.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Diagnostics.cpp?rev=358575&r1=358574&r2=358575&view=diff
==
--- clang-tools-extra/trunk/clangd/Diagnostics.cpp (original)
+++ clang-tools-extra/trunk/clangd/Diagnostics.cpp Wed Apr 17 05:35:16 2019
@@ -7,9 +7,12 @@
 
//===--===//
 
 #include "Diagnostics.h"
+#include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
 #include "Compiler.h"
 #include "Logger.h"
 #include "SourceCode.h"
+#include "clang/Basic/AllDiagnostics.h"
+#include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
 #include "llvm/Support/Capacity.h"
@@ -18,9 +21,31 @@
 
 namespace clang {
 namespace clangd {
-
 namespace {
 
+const char *getDiagnosticCode(unsigned ID) {
+  switch (ID) {
+#define DIAG(ENUM, CLASS, DEFAULT_MAPPING, DESC, GROPU, SFINAE, NOWERROR,  
\
+ SHOWINSYSHEADER, CATEGORY)
\
+  case clang::diag::ENUM:  
\
+return #ENUM;
+#include "clang/Basic/DiagnosticASTKinds.inc"
+#include "clang/Basic/DiagnosticAnalysisKinds.inc"
+#include "clang/Basic/DiagnosticCommentKinds.inc"
+#include "clang/Basic/DiagnosticCommonKinds.inc"
+#include "clang/Basic/DiagnosticDriverKinds.inc"
+#include "clang/Basic/DiagnosticFrontendKinds.inc"
+#include "clang/Basic/DiagnosticLexKinds.inc"
+#include "clang/Basic/DiagnosticParseKinds.inc"
+#include "clang/Basic/DiagnosticRefactoringKinds.inc"
+#include "clang/Basic/DiagnosticSemaKinds.inc"
+#include "clang/Basic/DiagnosticSerializationKinds.inc"
+#undef DIAG
+  default:
+return nullptr;
+  }
+}
+
 bool mentionsMainFile(const Diag &D) {
   if (D.InsideMainFile)
 return true;
@@ -253,6 +278,18 @@ void toLSPDiags(
   {
 clangd::Diagnostic Main = FillBasicFields(D);
 Main.message = mainMessage(D, Opts.DisplayFixesCount);
+if (!D.Name.empty())
+  Main.code = D.Name;
+switch (D.Source) {
+case Diag::Clang:
+  Main.source = "clang";
+  break;
+case Diag::ClangTidy:
+  Main.source = "clang-tidy";
+  break;
+case Diag::Unknown:
+  break;
+}
 if (Opts.EmbedFixesInDiagnostics) {
   Main.codeActions.emplace(

[PATCH] D58291: [clangd] Include textual diagnostic ID as Diagnostic.code.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D58291#1469917 , @kadircet wrote:

> In D58291#1469880 , @sammccall wrote:
>
> > It's a good question, it depends how this is surfaced, and we may want to 
> > tweak the behavior or suppress entirely in some cases.
> >  I think at least some are useful:
> >
> > - clang-tidy check names are things users need to know about (used for 
> > configuration)
> > - for warnings, we quite likely should replace with the most specific 
> > warning category (e.g. "unreachable-code-loop-increment"), again these are 
> > used for configuration (-W)
> > - for others, maybe we should at least trim the err_ prefix, or maybe drop 
> > them entirely.
>
>
> I see, I believe we can decide on what tweaks to perform after landing this 
> patch and seeing behaviors in different editors, but up to you.
>
> As for the lit-tests, it would be great to have a diag with source clang-tidy 
> if it is not too much of a hustle.


Done.
There are some things I know I want to tweak (clang-tidy keeps the name of the 
check in the message...), will follow up.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58291



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

[PATCH] D58291: [clangd] Include textual diagnostic ID as Diagnostic.code.

[Sam McCall via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE358575: [clangd] Include textual diagnostic ID as 
Diagnostic.code. (authored by sammccall, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58291?vs=195522&id=195544#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58291

Files:
  clangd/ClangdUnit.cpp
  clangd/Diagnostics.cpp
  clangd/Diagnostics.h
  clangd/Protocol.cpp
  clangd/Protocol.h
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/diagnostic-category.test
  test/clangd/diagnostics.test
  test/clangd/did-change-configuration-params.test
  test/clangd/execute-command.test
  test/clangd/fixits-codeaction.test
  test/clangd/fixits-command.test
  test/clangd/fixits-embed-in-diagnostic.test
  unittests/clangd/DiagnosticsTests.cpp

Index: test/clangd/fixits-command.test
===
--- test/clangd/fixits-command.test
+++ test/clangd/fixits-command.test
@@ -6,6 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"code": "warn_condition_is_assignment",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
@@ -17,7 +18,8 @@
 # CHECK-NEXT:"line": 0
 # CHECK-NEXT:  }
 # CHECK-NEXT:},
-# CHECK-NEXT:"severity": 2
+# CHECK-NEXT:"severity": 2,
+# CHECK-NEXT:"source": "clang"
 # CHECK-NEXT:  }
 # CHECK-NEXT:],
 # CHECK-NEXT:"uri": "file://{{.*}}/foo.c"
Index: test/clangd/fixits-codeaction.test
===
--- test/clangd/fixits-codeaction.test
+++ test/clangd/fixits-codeaction.test
@@ -6,6 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
+# CHECK-NEXT:"code": "warn_condition_is_assignment",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
@@ -17,19 +18,21 @@
 # CHECK-NEXT:"line": 0
 # CHECK-NEXT:  }
 # CHECK-NEXT:},
-# CHECK-NEXT:"severity": 2
+# CHECK-NEXT:"severity": 2,
+# CHECK-NEXT:"source": "clang"
 # 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":0,"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 (fixes available)"}]}}}
+{"jsonrpc":"2.0","id":2,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///foo.c"},"range":{"start":{"line":0,"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 (fixes available)", "code": "warn_condition_is_assignment", "source": "clang"}]}}}
 #  CHECK:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0",
 # CHECK-NEXT:  "result": [
 # CHECK-NEXT:{
 # CHECK-NEXT:  "diagnostics": [
 # CHECK-NEXT:{
+# CHECK-NEXT:  "code": "warn_condition_is_assignment",
 # CHECK-NEXT:  "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:  "range": {
 # CHECK-NEXT:"end": {
@@ -41,7 +44,8 @@
 # CHECK-NEXT:  "line": 0
 # CHECK-NEXT:}
 # CHECK-NEXT:  },
-# CHECK-NEXT:  "severity": 2
+# CHECK-NEXT:  "severity": 2,
+# CHECK-NEXT:  "source": "clang"
 # CHECK-NEXT:}
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  "edit": {
@@ -82,6 +86,7 @@
 # CHECK-NEXT:{
 # CHECK-NEXT:  "diagnostics": [
 # CHECK-NEXT:{
+# CHECK-NEXT:  "code": "warn_condition_is_assignment",
 # CHECK-NEXT:  "message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:  "range": {
 # CHECK-NEXT:"end": {
@@ -93,7 +98,8 @@
 # CHECK-NEXT:  "line": 0
 # CHECK-NEXT:}
 # CHECK-NEXT:  },
-# CHECK-NEXT:  "severity": 2
+# CHECK-NEXT:  "severity": 2,
+# CHECK-NEXT:  "source": "clang"
 # CHECK-NEXT:}
 # CHECK-NEXT:  ],
 # CHECK-NEXT:  "edit": {
Ind

[PATCH] D60818: [CUDA][Windows] restrict long double functions declarations to Windows

[Evgeny Mankov via Phabricator via cfe-commits]

emankov created this revision.
emankov added a reviewer: tra.
emankov added a project: clang.
Herald added a subscriber: cfe-commits.

As agreed in D60220 , make `long double` 
declarations unobservable on non-windows platforms.


Repository:
  rC Clang

https://reviews.llvm.org/D60818

Files:
  clang/lib/Headers/__clang_cuda_cmath.h
  clang/lib/Headers/__clang_cuda_device_functions.h
  clang/lib/Headers/__clang_cuda_math_forward_declares.h


Index: clang/lib/Headers/__clang_cuda_math_forward_declares.h
===
--- clang/lib/Headers/__clang_cuda_math_forward_declares.h
+++ clang/lib/Headers/__clang_cuda_math_forward_declares.h
@@ -84,14 +84,18 @@
 __DEVICE__ float hypot(float, float);
 __DEVICE__ int ilogb(double);
 __DEVICE__ int ilogb(float);
+#ifdef _MSC_VER
 __DEVICE__ bool isfinite(long double);
+#endif
 __DEVICE__ bool isfinite(double);
 __DEVICE__ bool isfinite(float);
 __DEVICE__ bool isgreater(double, double);
 __DEVICE__ bool isgreaterequal(double, double);
 __DEVICE__ bool isgreaterequal(float, float);
 __DEVICE__ bool isgreater(float, float);
+#ifdef _MSC_VER
 __DEVICE__ bool isinf(long double);
+#endif
 __DEVICE__ bool isinf(double);
 __DEVICE__ bool isinf(float);
 __DEVICE__ bool isless(double, double);
@@ -100,7 +104,9 @@
 __DEVICE__ bool isless(float, float);
 __DEVICE__ bool islessgreater(double, double);
 __DEVICE__ bool islessgreater(float, float);
+#ifdef _MSC_VER
 __DEVICE__ bool isnan(long double);
+#endif
 __DEVICE__ bool isnan(double);
 __DEVICE__ bool isnan(float);
 __DEVICE__ bool isnormal(double);
Index: clang/lib/Headers/__clang_cuda_device_functions.h
===
--- clang/lib/Headers/__clang_cuda_device_functions.h
+++ clang/lib/Headers/__clang_cuda_device_functions.h
@@ -223,7 +223,9 @@
 __DEVICE__ int __ffsll(long long __a) { return __nv_ffsll(__a); }
 __DEVICE__ int __finite(double __a) { return __nv_isfinited(__a); }
 __DEVICE__ int __finitef(float __a) { return __nv_finitef(__a); }
+#ifdef _MSC_VER
 __DEVICE__ int __finitel(long double __a);
+#endif
 __DEVICE__ int __float2int_rd(float __a) { return __nv_float2int_rd(__a); }
 __DEVICE__ int __float2int_rn(float __a) { return __nv_float2int_rn(__a); }
 __DEVICE__ int __float2int_ru(float __a) { return __nv_float2int_ru(__a); }
@@ -432,10 +434,14 @@
 __DEVICE__ int __isfinited(double __a) { return __nv_isfinited(__a); }
 __DEVICE__ int __isinf(double __a) { return __nv_isinfd(__a); }
 __DEVICE__ int __isinff(float __a) { return __nv_isinff(__a); }
+#ifdef _MSC_VER
 __DEVICE__ int __isinfl(long double __a);
+#endif
 __DEVICE__ int __isnan(double __a) { return __nv_isnand(__a); }
 __DEVICE__ int __isnanf(float __a) { return __nv_isnanf(__a); }
+#ifdef _MSC_VER
 __DEVICE__ int __isnanl(long double __a);
+#endif
 __DEVICE__ double __ll2double_rd(long long __a) {
   return __nv_ll2double_rd(__a);
 }
Index: clang/lib/Headers/__clang_cuda_cmath.h
===
--- clang/lib/Headers/__clang_cuda_cmath.h
+++ clang/lib/Headers/__clang_cuda_cmath.h
@@ -64,16 +64,13 @@
 #ifndef _MSC_VER
 __DEVICE__ bool isinf(float __x) { return ::__isinff(__x); }
 __DEVICE__ bool isinf(double __x) { return ::__isinf(__x); }
-__DEVICE__ bool isinf(long double __x) { return ::__isinfl(__x); }
 __DEVICE__ bool isfinite(float __x) { return ::__finitef(__x); }
 // For inscrutable reasons, __finite(), the double-precision version of
 // __finitef, does not exist when compiling for MacOS.  __isfinited is 
available
 // everywhere and is just as good.
 __DEVICE__ bool isfinite(double __x) { return ::__isfinited(__x); }
-__DEVICE__ bool isfinite(long double __x) { return ::__finitel(__x); }
 __DEVICE__ bool isnan(float __x) { return ::__isnanf(__x); }
 __DEVICE__ bool isnan(double __x) { return ::__isnan(__x); }
-__DEVICE__ bool isnan(long double __x) { return ::__isnanl(__x); }
 #endif
 
 __DEVICE__ bool isgreater(float __x, float __y) {


Index: clang/lib/Headers/__clang_cuda_math_forward_declares.h
===
--- clang/lib/Headers/__clang_cuda_math_forward_declares.h
+++ clang/lib/Headers/__clang_cuda_math_forward_declares.h
@@ -84,14 +84,18 @@
 __DEVICE__ float hypot(float, float);
 __DEVICE__ int ilogb(double);
 __DEVICE__ int ilogb(float);
+#ifdef _MSC_VER
 __DEVICE__ bool isfinite(long double);
+#endif
 __DEVICE__ bool isfinite(double);
 __DEVICE__ bool isfinite(float);
 __DEVICE__ bool isgreater(double, double);
 __DEVICE__ bool isgreaterequal(double, double);
 __DEVICE__ bool isgreaterequal(float, float);
 __DEVICE__ bool isgreater(float, float);
+#ifdef _MSC_VER
 __DEVICE__ bool isinf(long double);
+#endif
 __DEVICE__ bool isinf(double);
 __DEVICE__ bool isinf(float);
 __DEVICE__ bool isless(double, double);
@@ -100,7 +104,9 @@
 __DEVICE__ bool isless(float, float);
 __DEVICE__ bool islessgreater(doub

[PATCH] D60819: [clangd] Strip the ' [some-check-name]' suffix from clang-tidy diagnostics. The check name is reported in Diagnostic.code.

[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.
Herald added a project: clang.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D60819

Files:
  clangd/Diagnostics.cpp
  test/clangd/diagnostics.test
  unittests/clangd/DiagnosticsTests.cpp


Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -176,23 +176,21 @@
   UnorderedElementsAre(
   AllOf(Diag(Test.range("deprecated"),
  "inclusion of deprecated C++ header 'assert.h'; consider "
- "using 'cassert' instead [modernize-deprecated-headers]"),
+ "using 'cassert' instead"),
 DiagSource(Diag::ClangTidy),
 DiagName("modernize-deprecated-headers"),
 WithFix(Fix(Test.range("deprecated"), "",
 "change '\"assert.h\"' to ''"))),
   Diag(Test.range("doubled"),
-   "suspicious usage of 'sizeof(sizeof(...))' "
-   "[bugprone-sizeof-expression]"),
+   "suspicious usage of 'sizeof(sizeof(...))'"),
   AllOf(
   Diag(Test.range("macroarg"),
"side effects in the 1st macro argument 'X' are repeated in 
"
-   "macro expansion [bugprone-macro-repeated-side-effects]"),
+   "macro expansion"),
   DiagSource(Diag::ClangTidy),
   DiagName("bugprone-macro-repeated-side-effects"),
-  WithNote(Diag(Test.range("macrodef"),
-"macro 'SQUARE' defined here "
-"[bugprone-macro-repeated-side-effects]"))),
+  WithNote(
+  Diag(Test.range("macrodef"), "macro 'SQUARE' defined 
here"))),
   Diag(Test.range("macroarg"),
"multiple unsequenced modifications to 'y'")));
 }
Index: test/clangd/diagnostics.test
===
--- test/clangd/diagnostics.test
+++ test/clangd/diagnostics.test
@@ -23,7 +23,7 @@
 # CHECK-NEXT:  },
 # CHECK-NEXT:  {
 # CHECK-NEXT:"code": "bugprone-sizeof-expression",
-# CHECK-NEXT:"message": "Suspicious usage of 'sizeof(K)'; did you mean 
'K'? [bugprone-sizeof-expression]",
+# CHECK-NEXT:"message": "Suspicious usage of 'sizeof(K)'; did you mean 
'K'?",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
 # CHECK-NEXT:"character": 12,
Index: clangd/Diagnostics.cpp
===
--- clangd/Diagnostics.cpp
+++ clangd/Diagnostics.cpp
@@ -340,6 +340,17 @@
   if (!TidyDiag.empty()) {
 Diag.Name = std::move(TidyDiag);
 Diag.Source = Diag::ClangTidy;
+// clang-tidy bakes the name into diagnostic messages. Strip it out.
+// It would be much nicer to make clang-tidy not do this.
+auto CleanMessage = [&](std::string &Msg) {
+  StringRef Rest(Msg);
+  if (Rest.consume_back("]") && Rest.consume_back(Diag.Name) &&
+  Rest.consume_back(" ["))
+Msg.resize(Rest.size());
+};
+CleanMessage(Diag.Message);
+for (auto& Note : Diag.Notes)
+  CleanMessage(Note.Message);
 continue;
   }
 }


Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -176,23 +176,21 @@
   UnorderedElementsAre(
   AllOf(Diag(Test.range("deprecated"),
  "inclusion of deprecated C++ header 'assert.h'; consider "
- "using 'cassert' instead [modernize-deprecated-headers]"),
+ "using 'cassert' instead"),
 DiagSource(Diag::ClangTidy),
 DiagName("modernize-deprecated-headers"),
 WithFix(Fix(Test.range("deprecated"), "",
 "change '\"assert.h\"' to ''"))),
   Diag(Test.range("doubled"),
-   "suspicious usage of 'sizeof(sizeof(...))' "
-   "[bugprone-sizeof-expression]"),
+   "suspicious usage of 'sizeof(sizeof(...))'"),
   AllOf(
   Diag(Test.range("macroarg"),
"side effects in the 1st macro argument 'X' are repeated in "
-   "macro expansion [bugprone-macro-repeated-side-effects]"),
+   "macro expansion"),
   DiagSource(Diag::ClangTidy),
   DiagName("bugprone-macro-repeated-side-effects"),
-  WithNote(Diag(Test.range("macrodef"),
-"macro 'SQUARE' defined here "
-"[bugpron

[clang-tools-extra] r358576 - [clang-tidy] Add fix descriptions to clang-tidy checks.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Wed Apr 17 05:53:59 2019
New Revision: 358576

URL: http://llvm.org/viewvc/llvm-project?rev=358576&view=rev
Log:
[clang-tidy] Add fix descriptions to clang-tidy checks.

Summary:
Motivation/Context: in the code review system integrating with clang-tidy,
clang-tidy doesn't provide a human-readable description of the fix. Usually
developers have to preview a code diff (before vs after apply the fix) to
understand what the fix does before applying a fix.

This patch proposes that each clang-tidy check provides a short and
actional fix description that can be shown in the UI, so that users can know
what the fix does without previewing diff.

This patch extends clang-tidy framework to support fix descriptions (will add 
implementations for
existing checks in the future). Fix descriptions and fixes are emitted via 
diagnostic::Note (rather than
attaching the main warning diagnostic).

Before this patch:

```
void MyCheck::check(...) {
   ...
   diag(loc, "my check warning") <<  FixtItHint::CreateReplacement(...);
}
```

After:

```
void MyCheck::check(...) {
   ...
   diag(loc, "my check warning"); // Emit a check warning
   diag(loc, "fix description", DiagnosticIDs::Note) << 
FixtItHint::CreateReplacement(...); // Emit a diagnostic note and a fix
}
```

Reviewers: sammccall, alexfh

Reviewed By: alexfh

Subscribers: MyDeveloperDay, Eugene.Zelenko, aaron.ballman, JonasToth, 
xazax.hun, jdoerfert, cfe-commits

Tags: #clang-tools-extra, #clang

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

Modified:

clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/trunk/clang-tidy/add_new_check.py
clang-tools-extra/trunk/clang-tidy/misc/UnusedUsingDeclsCheck.cpp

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/basic/file1.yaml

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/basic/file2.yaml

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/conflict/file1.yaml

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/conflict/file2.yaml

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/conflict/file3.yaml
clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/crlf/file1.yaml
clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/format/no.yaml
clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/format/yes.yaml

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/file1.yaml

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/identical/file2.yaml

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/order-dependent/file1.yaml

clang-tools-extra/trunk/test/clang-apply-replacements/Inputs/order-dependent/file2.yaml
clang-tools-extra/trunk/test/clang-tidy/export-diagnostics.cpp

clang-tools-extra/trunk/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
clang-tools-extra/trunk/unittests/clang-tidy/ClangTidyTest.h

Modified: 
clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp?rev=358576&r1=358575&r2=358576&view=diff
==
--- 
clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
 (original)
+++ 
clang-tools-extra/trunk/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
 Wed Apr 17 05:53:59 2019
@@ -19,6 +19,7 @@
 #include "clang/Format/Format.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Rewrite/Core/Rewriter.h"
+#include "clang/Tooling/Core/Diagnostic.h"
 #include "clang/Tooling/DiagnosticsYaml.h"
 #include "clang/Tooling/ReplacementsYaml.h"
 #include "llvm/ADT/ArrayRef.h"
@@ -169,9 +170,11 @@ groupReplacements(const TUReplacements &
 
   for (const auto &TU : TUDs)
 for (const auto &D : TU.Diagnostics)
-  for (const auto &Fix : D.Fix)
-for (const tooling::Replacement &R : Fix.second)
-  AddToGroup(R, true);
+  if (const auto *ChoosenFix = tooling::selectFirstFix(D)) {
+for (const auto &Fix : *ChoosenFix)
+  for (const tooling::Replacement &R : Fix.second)
+AddToGroup(R, true);
+  }
 
   // Sort replacements per file to keep consistent behavior when
   // clang-apply-replacements run on differents machine.

Modified: clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp?rev=358576&r1=358575&r2=358576&view=diff
==
--- clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp Wed Apr 17 05:53:59 2019
@@ -

r358576 - [clang-tidy] Add fix descriptions to clang-tidy checks.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Wed Apr 17 05:53:59 2019
New Revision: 358576

URL: http://llvm.org/viewvc/llvm-project?rev=358576&view=rev
Log:
[clang-tidy] Add fix descriptions to clang-tidy checks.

Summary:
Motivation/Context: in the code review system integrating with clang-tidy,
clang-tidy doesn't provide a human-readable description of the fix. Usually
developers have to preview a code diff (before vs after apply the fix) to
understand what the fix does before applying a fix.

This patch proposes that each clang-tidy check provides a short and
actional fix description that can be shown in the UI, so that users can know
what the fix does without previewing diff.

This patch extends clang-tidy framework to support fix descriptions (will add 
implementations for
existing checks in the future). Fix descriptions and fixes are emitted via 
diagnostic::Note (rather than
attaching the main warning diagnostic).

Before this patch:

```
void MyCheck::check(...) {
   ...
   diag(loc, "my check warning") <<  FixtItHint::CreateReplacement(...);
}
```

After:

```
void MyCheck::check(...) {
   ...
   diag(loc, "my check warning"); // Emit a check warning
   diag(loc, "fix description", DiagnosticIDs::Note) << 
FixtItHint::CreateReplacement(...); // Emit a diagnostic note and a fix
}
```

Reviewers: sammccall, alexfh

Reviewed By: alexfh

Subscribers: MyDeveloperDay, Eugene.Zelenko, aaron.ballman, JonasToth, 
xazax.hun, jdoerfert, cfe-commits

Tags: #clang-tools-extra, #clang

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

Modified:
cfe/trunk/include/clang/Tooling/Core/Diagnostic.h
cfe/trunk/include/clang/Tooling/DiagnosticsYaml.h
cfe/trunk/lib/Tooling/Core/Diagnostic.cpp
cfe/trunk/unittests/Tooling/DiagnosticsYamlTest.cpp

Modified: cfe/trunk/include/clang/Tooling/Core/Diagnostic.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Core/Diagnostic.h?rev=358576&r1=358575&r2=358576&view=diff
==
--- cfe/trunk/include/clang/Tooling/Core/Diagnostic.h (original)
+++ cfe/trunk/include/clang/Tooling/Core/Diagnostic.h Wed Apr 17 05:53:59 2019
@@ -42,6 +42,9 @@ struct DiagnosticMessage {
   std::string Message;
   std::string FilePath;
   unsigned FileOffset;
+
+  /// Fixes for this diagnostic, grouped by file path.
+  llvm::StringMap Fix;
 };
 
 /// Represents the diagnostic with the level of severity and possible
@@ -58,7 +61,6 @@ struct Diagnostic {
  StringRef BuildDirectory);
 
   Diagnostic(llvm::StringRef DiagnosticName, const DiagnosticMessage &Message,
- const llvm::StringMap &Fix,
  const SmallVector &Notes, Level DiagLevel,
  llvm::StringRef BuildDirectory);
 
@@ -68,9 +70,6 @@ struct Diagnostic {
   /// Message associated to the diagnostic.
   DiagnosticMessage Message;
 
-  /// Fixes to apply, grouped by file path.
-  llvm::StringMap Fix;
-
   /// Potential notes about the diagnostic.
   SmallVector Notes;
 
@@ -94,6 +93,10 @@ struct TranslationUnitDiagnostics {
   std::vector Diagnostics;
 };
 
+// Get the first fix to apply for this diagnostic.
+// Return nullptr if no fixes attached to the diagnostic.
+const llvm::StringMap *selectFirstFix(const Diagnostic& D);
+
 } // end namespace tooling
 } // end namespace clang
 #endif // LLVM_CLANG_TOOLING_CORE_DIAGNOSTIC_H

Modified: cfe/trunk/include/clang/Tooling/DiagnosticsYaml.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/DiagnosticsYaml.h?rev=358576&r1=358575&r2=358576&view=diff
==
--- cfe/trunk/include/clang/Tooling/DiagnosticsYaml.h (original)
+++ cfe/trunk/include/clang/Tooling/DiagnosticsYaml.h Wed Apr 17 05:53:59 2019
@@ -31,6 +31,20 @@ template <> struct MappingTraits Fixes;
+for (auto &Replacements : M.Fix) {
+  for (auto &Replacement : Replacements.second)
+Fixes.push_back(Replacement);
+}
+Io.mapRequired("Replacements", Fixes);
+for (auto &Fix : Fixes) {
+  llvm::Error Err = M.Fix[Fix.getFilePath()].add(Fix);
+  if (Err) {
+// FIXME: Implement better conflict handling.
+llvm::errs() << "Fix conflicts with existing fix: "
+ << llvm::toString(std::move(Err)) << "\n";
+  }
+}
   }
 };
 
@@ -43,12 +57,11 @@ template <> struct MappingTraits struct MappingTraits 
Keys(
 Io, D);
 Io.mapRequired("DiagnosticName", Keys->DiagnosticName);
-Io.mapRequired("Message", Keys->Message.Message);
-Io.mapRequired("FileOffset", Keys->Message.FileOffset);
-Io.mapRequired("FilePath", Keys->Message.FilePath);
+Io.mapRequired("DiagnosticMessage", Keys->Message);
 Io.mapOptional("Notes", Keys->Notes);
 
 // FIXME: Export properly all the different fields.
-
-std::vector Fixes;
-for (auto &Replacements : Keys->Fix) {
-  for (auto &Replacement : Replacements.second) {
-Fixes.push_back(

[PATCH] D60719: Demonstrate how to fix freestanding for memcpy

[Guillaume Chatelet via Phabricator via cfe-commits]

gchatelet added a comment.

In D60719#1469958 , @t.p.northover 
wrote:

> I think it'd be pretty unpopular with the people I know who use freestanding. 
> They're mostly working on microcontrollers and compiling -Oz so the extra 
> code size would be untenable; they also have memcpy implementations anyway 
> because they use it in their own code.
>
> If I was trying to solve this (and I'm also not 100% sure it needs solving), 
> I think I'd instead generate a `linkonce` definition of `memcpy` whenever 
> it's needed and leave CodeGen unmodified.


Thx for chiming in @t.p.northover . Since the patch was mostly a proof of 
concept I haven't explained well what the intent was.
Ultimately I'm interested in implementing libc's mem function in C++. Let's 
take `memcpy` for instance, it would be composed out of fixed sized copies 
created from `__builtin_memcpy`. Unfortunately, the compiler is allowed to 
replace the intrinsic with a call to `memcpy` - which I'm currently defining - 
the generated code would recurse indefinitely.

IIUC //freestanding// environment should not rely on `memcpy` being present so 
my take on it was that by "fixing" //freestanding// I could have my cake and 
eat it too.

There are other options to explore but your comment shows that it's better to 
start a discussion around an RFC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60719



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

[PATCH] D59932: [clang-tidy] Add fix descriptions to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC358576: [clang-tidy] Add fix descriptions to clang-tidy 
checks. (authored by hokein, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59932?vs=194817&id=195550#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59932

Files:
  include/clang/Tooling/Core/Diagnostic.h
  include/clang/Tooling/DiagnosticsYaml.h
  lib/Tooling/Core/Diagnostic.cpp
  unittests/Tooling/DiagnosticsYamlTest.cpp

Index: unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- unittests/Tooling/DiagnosticsYamlTest.cpp
+++ unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -20,11 +20,13 @@
 using clang::tooling::Diagnostic;
 
 static DiagnosticMessage makeMessage(const std::string &Message, int FileOffset,
- const std::string &FilePath) {
+ const std::string &FilePath,
+ const StringMap &Fix) {
   DiagnosticMessage DiagMessage;
   DiagMessage.Message = Message;
   DiagMessage.FileOffset = FileOffset;
   DiagMessage.FilePath = FilePath;
+  DiagMessage.Fix = Fix;
   return DiagMessage;
 }
 
@@ -32,10 +34,52 @@
  const std::string &Message, int FileOffset,
  const std::string &FilePath,
  const StringMap &Fix) {
-  return Diagnostic(DiagnosticName, makeMessage(Message, FileOffset, FilePath),
-Fix, {}, Diagnostic::Warning, "path/to/build/directory");
+  return Diagnostic(DiagnosticName,
+makeMessage(Message, FileOffset, FilePath, Fix), {},
+Diagnostic::Warning, "path/to/build/directory");
 }
 
+static const char *YAMLContent =
+"---\n"
+"MainSourceFile:  'path/to/source.cpp'\n"
+"Diagnostics: \n"
+"  - DiagnosticName:  'diagnostic#1\'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #1'\n"
+"  FilePath:'path/to/source.cpp'\n"
+"  FileOffset:  55\n"
+"  Replacements:\n"
+"- FilePath:'path/to/source.cpp'\n"
+"  Offset:  100\n"
+"  Length:  12\n"
+"  ReplacementText: 'replacement #1'\n"
+"  - DiagnosticName:  'diagnostic#2'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #2'\n"
+"  FilePath:'path/to/header.h'\n"
+"  FileOffset:  60\n"
+"  Replacements:\n"
+"- FilePath:'path/to/header.h'\n"
+"  Offset:  62\n"
+"  Length:  2\n"
+"  ReplacementText: 'replacement #2'\n"
+"  - DiagnosticName:  'diagnostic#3'\n"
+"DiagnosticMessage: \n"
+"  Message: 'message #3'\n"
+"  FilePath:'path/to/source2.cpp'\n"
+"  FileOffset:  72\n"
+"  Replacements:[]\n"
+"Notes:   \n"
+"  - Message: Note1\n"
+"FilePath:'path/to/note1.cpp'\n"
+"FileOffset:  88\n"
+"Replacements:[]\n"
+"  - Message: Note2\n"
+"FilePath:'path/to/note2.cpp'\n"
+"FileOffset:  99\n"
+"Replacements:[]\n"
+"...\n";
+
 TEST(DiagnosticsYamlTest, serializesDiagnostics) {
   TranslationUnitDiagnostics TUD;
   TUD.MainSourceFile = "path/to/source.cpp";
@@ -55,9 +99,9 @@
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72,
"path/to/source2.cpp", {}));
   TUD.Diagnostics.back().Notes.push_back(
-  makeMessage("Note1", 88, "path/to/note1.cpp"));
+  makeMessage("Note1", 88, "path/to/note1.cpp", {}));
   TUD.Diagnostics.back().Notes.push_back(
-  makeMessage("Note2", 99, "path/to/note2.cpp"));
+  makeMessage("Note2", 99, "path/to/note2.cpp", {}));
 
   std::string YamlContent;
   raw_string_ostream YamlContentStream(YamlContent);
@@ -65,80 +109,12 @@
   yaml::Output YAML(YamlContentStream);
   YAML << TUD;
 
-  EXPECT_EQ("---\n"
-"MainSourceFile:  'path/to/source.cpp'\n"
-"Diagnostics: \n"
-"  - DiagnosticName:  'diagnostic#1\'\n"
-"Message: 'message #1'\n"
-"FileOffset:  55\n"
-"FilePath:'path/to/source.cpp'\n"
-"Replacements:\n"
-"  - FilePath:'path/to/source.cpp'\n"
-"Offset:  100\n"
-"Length:  12\n"
-"ReplacementText: 'replacement #1'\n"
-"  - DiagnosticName:  'diagnostic#2'\n"
-"Message: 'message #2'\n"
-"FileOffset:  60\n"
-   

[PATCH] D60362: [clang-format] [PR39719] clang-format converting object-like macro to function-like macro

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@klimek one possible solution to this might be to replace the "keyword" back to 
an identifier in a  '#define ' scenario

Maybe something like this?

  bool FormatTokenLexer::tryConvertKeyWordDefines() {
// ensure #define keyword x = tok::hash,tok::identifier,tok::identifier
if (Tokens.size() < 3)
  return false;
  
auto &Hash = *(Tokens.end() - 3);
auto &Define = *(Tokens.end() - 2);
auto &Keyword = *(Tokens.end() - 1);
  
if (!Hash->is(tok::hash))
  return false;
  
if (!Define->is(tok::identifier))
  return false;
  
// Already an identifier
if (Keyword->is(tok::identifier))
  return false;
  
if (!Define->Tok.getIdentifierInfo() ||
Define->Tok.getIdentifierInfo()->getPPKeywordID() != tok::pp_define)
  return false;
  
// switch the type to be an identifier
Keyword->Tok.setKind(tok::identifier);
return true;
  }


Repository:
  rC Clang

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

https://reviews.llvm.org/D60362



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

[PATCH] D60516: [LTO] Add plumbing to save stats during LTO on Darwin.

[Florian Hahn via Phabricator via cfe-commits]

fhahn added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60516



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D60778#1468825 , @lebedev.ri wrote:

> > Not sure how to test this though? I guess we can trust that std::map is 
> > always sorted just as it says in its description.
>
> You could add a test that contains several entries in `OpenCLTypeExtMap` and 
> several entries in `OpenCLDeclExtMap`.
>  In that test, write te PCH, and then apply `llvm-bcanalyzer` to that PCH. It 
> should dump it the bitcode in textual dump.
>  And then simply apply FileCheck to that textual dump, with CHECK lines 
> requiring the specific order of these entries.
>  If the order is not deterministic in PCH, then that test would (should!) 
> fail sporadically.
>  At least that is my guess.


Ok I could do that, but I guess it can then fail on the commits that didn't 
actually trigger the issue. Would it not be a problem?


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

https://reviews.llvm.org/D60778



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

[PATCH] D60821: [clangd] Emit better error messages when rename fails.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ioeric, 
ilya-biryukov.
Herald added a project: clang.

Currently we emit an unfriendly "clang diagnostic" message when rename fails. 
This
patch makes clangd to emit a detailed diagnostic message.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60821

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/test/clangd/rename.test


Index: clang-tools-extra/test/clangd/rename.test
===
--- clang-tools-extra/test/clangd/rename.test
+++ clang-tools-extra/test/clangd/rename.test
@@ -29,7 +29,7 @@
 
{"jsonrpc":"2.0","id":2,"method":"textDocument/rename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2},"newName":"bar"}}
 #  CHECK:  "error": {
 # CHECK-NEXT:"code": -32001,
-# CHECK-NEXT:"message": "clang diagnostic"
+# CHECK-NEXT:"message": "there is no symbol at the given location"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0"
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -44,15 +44,23 @@
 namespace clangd {
 namespace {
 
+llvm::Error toStringError(DiagnosticError DiagErr, DiagnosticsEngine &DE) {
+  SmallVector DiagMessage;
+  DiagErr.getDiagnostic().second.EmitToString(DE, DiagMessage);
+  return llvm::make_error(DiagMessage,
+ llvm::inconvertibleErrorCode());
+}
+
 class RefactoringResultCollector final
 : public tooling::RefactoringResultConsumer {
 public:
+  RefactoringResultCollector(DiagnosticsEngine &DE) : Diags(DE) {}
   void handleError(llvm::Error Err) override {
 assert(!Result.hasValue());
-// FIXME: figure out a way to return better message for DiagnosticError.
-// clangd uses llvm::toString to convert the Err to string, however, for
-// DiagnosticError, only "clang diagnostic" will be generated.
-Result = std::move(Err);
+if (auto Diag = DiagnosticError::take(Err))
+  Result = toStringError(*Diag, Diags);
+else
+  Result = std::move(Err);
   }
 
   // Using the handle(SymbolOccurrences) from parent class.
@@ -63,6 +71,7 @@
 Result = std::move(SourceReplacements);
   }
 
+  DiagnosticsEngine &Diags;
   llvm::Optional> Result;
 };
 
@@ -291,7 +300,8 @@
   return CB(InpAST.takeError());
 auto &AST = InpAST->AST;
 
-RefactoringResultCollector ResultCollector;
+RefactoringResultCollector ResultCollector(
+AST.getASTContext().getDiagnostics());
 const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
 SourceLocation SourceLocationBeg =
 clangd::getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
@@ -300,8 +310,15 @@
 Context.setASTContext(AST.getASTContext());
 auto Rename = clang::tooling::RenameOccurrences::initiate(
 Context, SourceRange(SourceLocationBeg), NewName);
-if (!Rename)
-  return CB(Rename.takeError());
+if (!Rename) {
+  auto Error = Rename.takeError();
+  if (auto Diag = DiagnosticError::take(Error)) {
+llvm::cantFail(std::move(Error));
+return CB(toStringError(*Diag, AST.getASTContext().getDiagnostics()));
+  }
+
+  return CB(std::move(Error));
+}
 
 Rename->invoke(ResultCollector, Context);
 


Index: clang-tools-extra/test/clangd/rename.test
===
--- clang-tools-extra/test/clangd/rename.test
+++ clang-tools-extra/test/clangd/rename.test
@@ -29,7 +29,7 @@
 {"jsonrpc":"2.0","id":2,"method":"textDocument/rename","params":{"textDocument":{"uri":"test:///foo.cpp"},"position":{"line":0,"character":2},"newName":"bar"}}
 #  CHECK:  "error": {
 # CHECK-NEXT:"code": -32001,
-# CHECK-NEXT:"message": "clang diagnostic"
+# CHECK-NEXT:"message": "there is no symbol at the given location"
 # CHECK-NEXT:  },
 # CHECK-NEXT:  "id": 2,
 # CHECK-NEXT:  "jsonrpc": "2.0"
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -44,15 +44,23 @@
 namespace clangd {
 namespace {
 
+llvm::Error toStringError(DiagnosticError DiagErr, DiagnosticsEngine &DE) {
+  SmallVector DiagMessage;
+  DiagErr.getDiagnostic().second.EmitToString(DE, DiagMessage);
+  return llvm::make_error(DiagMessage,
+ llvm::inconvertibleErrorCode());
+}
+
 class RefactoringResultCollector final
 : public tooling::RefactoringResultConsumer {
 public:
+  RefactoringResultCollector(DiagnosticsEngine &DE) : Diags(DE) {}
   void handleError(llvm::Error Err) override {
 as

[PATCH] D60570: [Sema] Add more tests for the behavior of argument-dependent name lookup

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added subscribers: aaron.ballman, rsmith.
riccibruno added a comment.

@rsmith @aaron.ballman Do you have any opinion on whether the ADL rules from 
CWG 997 should be implemented ? The issue here as I understand it is that these 
rules expand ADL, but no one apart from GCC implement them. By implementing 
these rules, the argument to remove them in the future becomes weaker since 
real code might actually start to depend on them.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60570



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D60778#1470152 , @Anastasia wrote:

> In D60778#1468825 , @lebedev.ri 
> wrote:
>
> > > Not sure how to test this though? I guess we can trust that std::map is 
> > > always sorted just as it says in its description.
> >
> > You could add a test that contains several entries in `OpenCLTypeExtMap` 
> > and several entries in `OpenCLDeclExtMap`.
> >  In that test, write te PCH, and then apply `llvm-bcanalyzer` to that PCH. 
> > It should dump it the bitcode in textual dump.
> >  And then simply apply FileCheck to that textual dump, with CHECK lines 
> > requiring the specific order of these entries.
> >  If the order is not deterministic in PCH, then that test would (should!) 
> > fail sporadically.
> >  At least that is my guess.
>
>
> Ok I could do that, but I guess it can then fail on the commits that didn't 
> actually trigger the issue. Would it not be a problem?


Why would it fail if this fixes the issue?
Re randomness - see https://reviews.llvm.org/D59401#change-QMkBH7328Dyv


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

https://reviews.llvm.org/D60778



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

[PATCH] D60822: [clangd] Use shorter, more recognizable codes for diagnostics.

[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.
Herald added a project: clang.

- for warnings, use "foo" if the warning is controlled by -Wfoo
- for errors, keep using the internal name (there's nothing better) but drop 
the err_ prefix

This comes at the cost of uniformity, it's no longer totally obvious
exactly what the code field contains. But the -Wname flags are so much
more useful to end-users than the internal warn_foo that this seems worth it.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D60822

Files:
  clangd/Diagnostics.cpp
  test/clangd/compile-commands-path-in-initialize.test
  test/clangd/diagnostic-category.test
  test/clangd/diagnostics.test
  test/clangd/did-change-configuration-params.test
  test/clangd/execute-command.test
  test/clangd/fixits-codeaction.test
  test/clangd/fixits-command.test
  test/clangd/fixits-embed-in-diagnostic.test
  unittests/clangd/DiagnosticsTests.cpp

Index: unittests/clangd/DiagnosticsTests.cpp
===
--- unittests/clangd/DiagnosticsTests.cpp
+++ unittests/clangd/DiagnosticsTests.cpp
@@ -107,7 +107,7 @@
   AllOf(Diag(Test.range("typo"),
  "use of undeclared identifier 'goo'; did you mean 'foo'?"),
 DiagSource(Diag::Clang),
-DiagName("err_undeclared_var_use_suggest"),
+DiagName("undeclared_var_use_suggest"),
 WithFix(
 Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")),
 // This is a pretty normal range.
@@ -152,7 +152,7 @@
   EXPECT_THAT(TU.build().getDiagnostics(),
   ElementsAre(testing::AllOf(
   Diag(Test.range(), "'not-found.h' file not found"),
-  DiagSource(Diag::Clang), DiagName("err_pp_file_not_found";
+  DiagSource(Diag::Clang), DiagName("pp_file_not_found";
 }
 
 TEST(DiagnosticsTest, ClangTidy) {
@@ -252,7 +252,7 @@
 TEST(DiagnosticsTest, ToLSP) {
   clangd::Diag D;
   D.ID = clang::diag::err_enum_class_reference;
-  D.Name = "err_enum_class_reference";
+  D.Name = "enum_class_reference";
   D.Source = clangd::Diag::Clang;
   D.Message = "something terrible happened";
   D.Range = {pos(1, 2), pos(3, 4)};
@@ -322,7 +322,7 @@
   LSPDiags,
   ElementsAre(Pair(EqualToLSPDiag(MainLSP), ElementsAre(EqualToFix(F))),
   Pair(EqualToLSPDiag(NoteInMainLSP), IsEmpty(;
-  EXPECT_EQ(LSPDiags[0].first.code, "err_enum_class_reference");
+  EXPECT_EQ(LSPDiags[0].first.code, "enum_class_reference");
   EXPECT_EQ(LSPDiags[0].first.source, "clang");
   EXPECT_EQ(LSPDiags[1].first.code, "");
   EXPECT_EQ(LSPDiags[1].first.source, "");
Index: test/clangd/fixits-embed-in-diagnostic.test
===
--- test/clangd/fixits-embed-in-diagnostic.test
+++ test/clangd/fixits-embed-in-diagnostic.test
@@ -6,7 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"code": "err_use_with_wrong_tag",
+# CHECK-NEXT:"code": "use_with_wrong_tag",
 # CHECK-NEXT:"codeActions": [
 # CHECK-NEXT:  {
 # CHECK-NEXT:"edit": {
Index: test/clangd/fixits-command.test
===
--- test/clangd/fixits-command.test
+++ test/clangd/fixits-command.test
@@ -6,7 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"code": "warn_condition_is_assignment",
+# CHECK-NEXT:"code": "parentheses",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
Index: test/clangd/fixits-codeaction.test
===
--- test/clangd/fixits-codeaction.test
+++ test/clangd/fixits-codeaction.test
@@ -6,7 +6,7 @@
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:"diagnostics": [
 # CHECK-NEXT:  {
-# CHECK-NEXT:"code": "warn_condition_is_assignment",
+# CHECK-NEXT:"code": "parentheses",
 # CHECK-NEXT:"message": "Using the result of an assignment as a condition without parentheses (fixes available)",
 # CHECK-NEXT:"range": {
 # CHECK-NEXT:  "end": {
@@ -25,14 +25,14 @@
 # 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":0,"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 

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Mmm. I hope I am not missing something obvious, but how is this actually fixing 
the issue ? I don't see why the order between the `Type *` and between the 
`Decl *` should be deterministic (I think they will be when they are part of 
the same slab in the allocator, but I don't see why the slab ordering should be 
stable).


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

https://reviews.llvm.org/D60778



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D60778#1470181 , @riccibruno wrote:

> Mmm. I hope I am not missing something obvious, but how is this actually 
> fixing the issue ? I don't see why the order between the `Type *` and between 
> the `Decl *` should be deterministic (I think they will be when they are part 
> of the same slab in the allocator, but I don't see why the slab ordering 
> should be stable).


Oh. Duh.
Can a custom comparator be provided for these `std::map`, equivalent to the one 
in https://reviews.llvm.org/D60379 ?
Something like `getTypeID(LHS.first->getCanonicalTypeInternal()) < 
getTypeID(RHS.first->getCanonicalTypeInternal())` ?


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

https://reviews.llvm.org/D60778



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

[PATCH] D60815: [clangd] Recognize "don't include me directly" pattern, and suppress include insertion.

[Eric Liu via Phabricator via cfe-commits]

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

lgtm




Comment at: clangd/index/SymbolCollector.cpp:631
+
+bool SymbolCollector::isSelfContainedHeader(FileID FID) {
+  // The real computation (which will be memoized).

sammccall wrote:
> this has been moved to a member so it can use the cache and the 
> (non-threadsafe) regex, and its logic has obviously been changed.
"self-contained header" doesn't sound accurate anymore. Maybe something like 
`isIncludable`?



Comment at: clangd/index/SymbolCollector.h:125
+  // they need to be included via an umbrella header. e.g. GTK matches this.
+  llvm::Regex DontIncludeMe = {
+  "^[ \t]*#[ \t]*if.*\n" // An #if, #ifndef etc directive, then

`DontIncludeMePattern` or `DontIncludeMeRegex`?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60815



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

Maybe, but is there an equivalent to `getTypeID` for declarations ?


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

https://reviews.llvm.org/D60778



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

[PATCH] D60815: [clangd] Recognize "don't include me directly" pattern, and suppress include insertion.

[Sam McCall via Phabricator via cfe-commits]

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

DontIncludeMePattern


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60815

Files:
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  unittests/clangd/SymbolCollectorTests.cpp

Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -272,8 +272,8 @@
 Args, Factory->create(), Files.get(),
 std::make_shared());
 
-InMemoryFileSystem->addFile(TestHeaderName, 0,
-llvm::MemoryBuffer::getMemBuffer(HeaderCode));
+InMemoryFileSystem->addFile(
+TestHeaderName, 0, llvm::MemoryBuffer::getMemBuffer(HeaderCode));
 InMemoryFileSystem->addFile(TestFileName, 0,
 llvm::MemoryBuffer::getMemBuffer(MainCode));
 Invocation.run();
@@ -1064,8 +1064,19 @@
   auto TU = TestTU::withHeaderCode("int x();");
   EXPECT_THAT(TU.headerSymbols(), ElementsAre(IncludeHeader()));
 
+  // Files missing include guards aren't eligible for insertion.
   TU.ImplicitHeaderGuard = false;
   EXPECT_THAT(TU.headerSymbols(), ElementsAre(Not(IncludeHeader(;
+
+  // We recognize some patterns of trying to prevent insertion.
+  TU = TestTU::withHeaderCode(R"cpp(
+#ifndef SECRET
+#error "This file isn't safe to include directly"
+#endif
+int x();
+)cpp");
+  TU.ExtraArgs.push_back("-DSECRET"); // *we're* able to include it.
+  EXPECT_THAT(TU.headerSymbols(), ElementsAre(Not(IncludeHeader(;
 }
 
 TEST_F(SymbolCollectorTest, AvoidUsingFwdDeclsAsCanonicalDecls) {
Index: clangd/index/SymbolCollector.h
===
--- clangd/index/SymbolCollector.h
+++ clangd/index/SymbolCollector.h
@@ -19,6 +19,7 @@
 #include "clang/Index/IndexSymbol.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/Support/Regex.h"
 #include 
 
 namespace clang {
@@ -117,6 +118,15 @@
bool IsMainFileSymbol);
   void addDefinition(const NamedDecl &, const Symbol &DeclSymbol);
 
+  llvm::Optional getIncludeHeader(llvm::StringRef QName, FileID);
+  bool isSelfContainedHeader(FileID);
+  // Heuristic to detect headers that aren't self-contained, usually because
+  // they need to be included via an umbrella header. e.g. GTK matches this.
+  llvm::Regex DontIncludeMePattern = {
+  "^[ \t]*#[ \t]*if.*\n" // An #if, #ifndef etc directive, then
+  "[ \t]*#[ \t]*error.*include", // an #error directive mentioning "include"
+  llvm::Regex::Newline};
+
   // All Symbols collected from the AST.
   SymbolSlab::Builder Symbols;
   // All refs collected from the AST.
@@ -141,6 +151,7 @@
   llvm::DenseMap CanonicalDecls;
   // Cache whether to index a file or not.
   llvm::DenseMap FilesToIndexCache;
+  llvm::DenseMap HeaderIsSelfContainedCache;
 };
 
 } // namespace clangd
Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -128,48 +128,6 @@
   }
 }
 
-bool isSelfContainedHeader(FileID FID, const SourceManager &SM,
-   const Preprocessor &PP) {
-  const FileEntry *FE = SM.getFileEntryForID(FID);
-  if (!FE)
-return false;
-  return PP.getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE);
-}
-
-/// Gets a canonical include (URI of the header or  or "header") for
-/// header of \p FID (which should usually be the *expansion* file).
-/// Returns None if includes should not be inserted for this file.
-llvm::Optional
-getIncludeHeader(llvm::StringRef QName, const SourceManager &SM,
- const Preprocessor &PP, FileID FID,
- const SymbolCollector::Options &Opts) {
-  const FileEntry *FE = SM.getFileEntryForID(FID);
-  if (!FE || FE->getName().empty())
-return llvm::None;
-  llvm::StringRef Filename = FE->getName();
-  // If a file is mapped by canonical headers, use that mapping, regardless
-  // of whether it's an otherwise-good header (header guards etc).
-  if (Opts.Includes) {
-llvm::StringRef Canonical = Opts.Includes->mapHeader(Filename, QName);
-// If we had a mapping, always use it.
-if (Canonical.startswith("<") || Canonical.startswith("\""))
-  return Canonical.str();
-if (Canonical != Filename)
-  return toURI(SM, Canonical, Opts);
-  }
-  if (!isSelfContainedHeader(FID, SM, PP)) {
-// A .inc or .def file is often included into a real header to define
-// symbols (e.g. LLVM tablegen files).
-if (Filename.endswith(".inc") || Filename.endswith(".def"))
-  return getIncludeHeader(QName, SM, PP,
-  SM.getFileID(SM.getIncludeLoc(FID)), Opts);
-// 

[PATCH] D60822: [clangd] Use shorter, more recognizable codes for diagnostics.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

I agree that these are more useful. What about also adding "-W" in front of 
warning flags to make them more explicit and stand out from "non-flag" error 
names?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60822



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

[PATCH] D60815: [clangd] Recognize "don't include me directly" pattern, and suppress include insertion.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 2 inline comments as done.
sammccall added inline comments.



Comment at: clangd/index/SymbolCollector.cpp:631
+
+bool SymbolCollector::isSelfContainedHeader(FileID FID) {
+  // The real computation (which will be memoized).

ioeric wrote:
> sammccall wrote:
> > this has been moved to a member so it can use the cache and the 
> > (non-threadsafe) regex, and its logic has obviously been changed.
> "self-contained header" doesn't sound accurate anymore. Maybe something like 
> `isIncludable`?
Let me explain how I think about it, and then I'll change the name if you still 
don't think it fits.

What we're detecting here (usually) is that this header can't be included 
unless a certain symbol/state is defined first from the outside.
Whether that's for some direct technical reason (e.g. configuration is needed) 
or to enforce a coding style, the dependency on external preprocessor state 
means the header isn't self contained. (And the non-self-containedness is the 
reason we can't include it in arbitrary contexts) 


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60815



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

[PATCH] D60815: [clangd] Recognize "don't include me directly" pattern, and suppress include insertion.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/index/SymbolCollector.cpp:631
+
+bool SymbolCollector::isSelfContainedHeader(FileID FID) {
+  // The real computation (which will be memoized).

sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > this has been moved to a member so it can use the cache and the 
> > > (non-threadsafe) regex, and its logic has obviously been changed.
> > "self-contained header" doesn't sound accurate anymore. Maybe something 
> > like `isIncludable`?
> Let me explain how I think about it, and then I'll change the name if you 
> still don't think it fits.
> 
> What we're detecting here (usually) is that this header can't be included 
> unless a certain symbol/state is defined first from the outside.
> Whether that's for some direct technical reason (e.g. configuration is 
> needed) or to enforce a coding style, the dependency on external preprocessor 
> state means the header isn't self contained. (And the non-self-containedness 
> is the reason we can't include it in arbitrary contexts) 
That sounds good. Thanks for explaining!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60815



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

[PATCH] D60819: [clangd] Strip the ' [some-check-name]' suffix from clang-tidy diagnostics. The check name is reported in Diagnostic.code.

[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, we might need to make sure clients that do not show "code" field become 
aware of this before LLVM9 releases


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60819



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

[PATCH] D58033: Add option for emitting dbg info for call sites

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro marked 2 inline comments as done.
djtodoro added a comment.

@probinson @aprantl Thanks a lot for your comments!

Let's clarify some things. I'm sorry about the confusion.

Initial patch for the functionality can be restricted by this option (like we 
pushed here), since **LLDB** doesn't read this type of debug info (yet).
The plan is, when  **LLDB** has all necessary info and we are sure that 
everything works fine during using this info in debugging sessions, we can turn 
it to `ON` by default.

Does that make sense ? :)




Comment at: include/clang/Driver/Options.td:921
+   Group,
+   Flags<[CC1Option]>,
+   HelpText<"Enables debug info about call site and call 
site parameters">;

probinson wrote:
> I believe that by marking this with `[CC1Option]` cc1 will automatically 
> understand it and you won't need to define a separate option in CC1Options.td.
I can't remember what problem we occurred with this on 4.0, but we definitely 
don't need both definitions.
Thanks for this!  


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

https://reviews.llvm.org/D58033



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

[PATCH] D60826: Clarify -Winfinite-recursion message

[Christian Kühnel via Phabricator via cfe-commits]

kuhnel created this revision.
kuhnel added a reviewer: meikeb.
Herald added a project: clang.

Test review from the tutorial


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60826

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/test/SemaCXX/warn-infinite-recursion.cpp

Index: clang/test/SemaCXX/warn-infinite-recursion.cpp
===
--- clang/test/SemaCXX/warn-infinite-recursion.cpp
+++ clang/test/SemaCXX/warn-infinite-recursion.cpp
@@ -1,10 +1,10 @@
 // RUN: %clang_cc1 %s -fsyntax-only -verify -Winfinite-recursion
 
-void a() {  // expected-warning{{call itself}}
+void a() {  // expected-warning{{to understand recursion}}
   a();
 }
 
-void b(int x) {  // expected-warning{{call itself}}
+void b(int x) {  // expected-warning{{to understand recursion}}
   if (x)
 b(x);
   else
@@ -16,7 +16,7 @@
 c(5);
 }
 
-void d(int x) {  // expected-warning{{call itself}}
+void d(int x) {  // expected-warning{{to understand recursion}}
   if (x)
 ++x;
   return d(x);
@@ -29,7 +29,7 @@
 void e() { f(); }
 void f() { e(); }
 
-void g() {  // expected-warning{{call itself}}
+void g() {  // expected-warning{{to understand recursion}}
   while (true)
 g();
 
@@ -42,14 +42,14 @@
   }
 }
 
-void i(int x) {  // expected-warning{{call itself}}
+void i(int x) {  // expected-warning{{to understand recursion}}
   while (x < 5) {
 --x;
   }
   i(0);
 }
 
-int j() {  // expected-warning{{call itself}}
+int j() {  // expected-warning{{to understand recursion}}
   return 5 + j();
 }
 
@@ -80,11 +80,11 @@
   void b();
 };
 
-void S::a() {  // expected-warning{{call itself}}
+void S::a() {  // expected-warning{{to understand recursion}}
   return a();
 }
 
-void S::b() {  // expected-warning{{call itself}}
+void S::b() {  // expected-warning{{to understand recursion}}
   int i = 0;
   do {
 ++i;
@@ -95,8 +95,8 @@
 template
 struct T {
   member m;
-  void a() { return a(); }  // expected-warning{{call itself}}
-  static void b() { return b(); }  // expected-warning{{call itself}}
+  void a() { return a(); }  // expected-warning{{to understand recursion}}
+  static void b() { return b(); }  // expected-warning{{to understand recursion}}
 };
 
 void test_T() {
@@ -107,13 +107,13 @@
 
 class U {
   U* u;
-  void Fun() {  // expected-warning{{call itself}}
+  void Fun() {  // expected-warning{{to understand recursion}}
 u->Fun();
   }
 };
 
 // No warnings on templated functions
-// sum<0>() is instantiated, does recursively call itself, but never runs.
+// sum<0>() is instantiated, does recursively to understand recursion, but never runs.
 template 
 int sum() {
   return value + sum();
@@ -157,7 +157,7 @@
   return 0;
 return Wrapper::run();
   }
-  static int run2() {  // expected-warning{{call itself}}
+  static int run2() {  // expected-warning{{to understand recursion}}
 return run2();
   }
 };
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -61,7 +61,7 @@
   "remove call to max function and unsigned zero argument">;
 
 def warn_infinite_recursive_function : Warning<
-  "all paths through this function will call itself">,
+  "in order to understand recursion, you must first understand recursion">,
   InGroup, DefaultIgnore;
 
 def warn_comma_operator : Warning<"possible misuse of comma operator here">,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58033: Add option for emitting dbg info for call site parameters

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 195568.
djtodoro retitled this revision from "Add option for emitting dbg info for call 
sites" to "Add option for emitting dbg info for call site parameters".
djtodoro added a comment.

-Refactor
-Remove `CC1` def


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

https://reviews.llvm.org/D58033

Files:
  include/clang/Basic/CodeGenOptions.def
  include/clang/Driver/Options.td
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp


Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -746,6 +746,7 @@
 
   Opts.DisableLLVMPasses = Args.hasArg(OPT_disable_llvm_passes);
   Opts.DisableLifetimeMarkers = Args.hasArg(OPT_disable_lifetimemarkers);
+  Opts.EnableParamEntryValues = Args.hasArg(OPT_femit_param_entry_values);
   Opts.DisableO0ImplyOptNone = Args.hasArg(OPT_disable_O0_optnone);
   Opts.DisableRedZone = Args.hasArg(OPT_disable_red_zone);
   Opts.IndirectTlsSegRefs = Args.hasArg(OPT_mno_tls_direct_seg_refs);
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3396,6 +3396,10 @@
   if (DebuggerTuning == llvm::DebuggerKind::SCE)
 CmdArgs.push_back("-dwarf-explicit-import");
 
+  // Enable param entry values functionlaity.
+  if (Args.hasArg(options::OPT_femit_param_entry_values))
+CmdArgs.push_back("-femit-param-entry-values");
+
   RenderDebugInfoCompressionArgs(Args, CmdArgs, D, TC);
 }
 
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -4558,7 +4558,10 @@
   // were part of DWARF v4.
   bool SupportsDWARFv4Ext =
   CGM.getCodeGenOpts().DwarfVersion == 4 &&
-  CGM.getCodeGenOpts().getDebuggerTuning() == llvm::DebuggerKind::LLDB;
+  (CGM.getCodeGenOpts().getDebuggerTuning() == llvm::DebuggerKind::LLDB ||
+   (CGM.getCodeGenOpts().EnableParamEntryValues &&
+   CGM.getCodeGenOpts().getDebuggerTuning() == llvm::DebuggerKind::GDB));
+
   if (!SupportsDWARFv4Ext && CGM.getCodeGenOpts().DwarfVersion < 5)
 return llvm::DINode::FlagZero;
 
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -916,6 +916,9 @@
 def fjump_tables : Flag<["-"], "fjump-tables">, Group;
 def fno_jump_tables : Flag<["-"], "fno-jump-tables">, Group, 
Flags<[CC1Option]>,
   HelpText<"Do not use jump tables for lowering switches">;
+def femit_param_entry_values : Flag<["-"], "femit-param-entry-values">, 
Group,
+  Flags<[CC1Option]>,
+  HelpText<"Enables debug info about call site parameter's entry values">;
 def fforce_enable_int128 : Flag<["-"], "fforce-enable-int128">,
   Group, Flags<[CC1Option]>,
   HelpText<"Enable support for int128_t type">;
Index: include/clang/Basic/CodeGenOptions.def
===
--- include/clang/Basic/CodeGenOptions.def
+++ include/clang/Basic/CodeGenOptions.def
@@ -61,6 +61,7 @@
 CODEGENOPT(DebugPassManager, 1, 0) ///< Prints debug information for the new
///< pass manager.
 CODEGENOPT(DisableRedZone, 1, 0) ///< Set when -mno-red-zone is enabled.
+CODEGENOPT(EnableParamEntryValues, 1, 0) ///< Emit any call site dbg info
 CODEGENOPT(IndirectTlsSegRefs, 1, 0) ///< Set when -mno-tls-direct-seg-refs
  ///< is specified.
 CODEGENOPT(DisableTailCalls  , 1, 0) ///< Do not emit tail calls.


Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -746,6 +746,7 @@
 
   Opts.DisableLLVMPasses = Args.hasArg(OPT_disable_llvm_passes);
   Opts.DisableLifetimeMarkers = Args.hasArg(OPT_disable_lifetimemarkers);
+  Opts.EnableParamEntryValues = Args.hasArg(OPT_femit_param_entry_values);
   Opts.DisableO0ImplyOptNone = Args.hasArg(OPT_disable_O0_optnone);
   Opts.DisableRedZone = Args.hasArg(OPT_disable_red_zone);
   Opts.IndirectTlsSegRefs = Args.hasArg(OPT_mno_tls_direct_seg_refs);
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3396,6 +3396,10 @@
   if (DebuggerTuning == llvm::DebuggerKind::SCE)
 CmdArgs.push_back("-dwarf-explicit-import");
 
+  // Enable param entry values functionlaity.
+  if (Args.hasArg(options::OPT_femit_param_entry_values))
+CmdArgs.push_back("-femit-param-entry-values");
+
   RenderDebugInfoCompressionArgs(Args, CmdArgs, D, TC);
 }
 
Index: lib/CodeG

[PATCH] D58043: Add experimental options for call site related dbg info

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 195569.
djtodoro added a comment.

-Rebase


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

https://reviews.llvm.org/D58043

Files:
  lib/Driver/ToolChains/Clang.cpp


Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3397,8 +3397,15 @@
 CmdArgs.push_back("-dwarf-explicit-import");
 
   // Enable param entry values functionlaity.
-  if (Args.hasArg(options::OPT_femit_param_entry_values))
+  if (Args.hasArg(options::OPT_femit_param_entry_values) &&
+  areOptimizationsEnabled(Args)) {
 CmdArgs.push_back("-femit-param-entry-values");
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-emit-entry-values=1");
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-emit-call-site-info=1");
+
+  }
 
   RenderDebugInfoCompressionArgs(Args, CmdArgs, D, TC);
 }


Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3397,8 +3397,15 @@
 CmdArgs.push_back("-dwarf-explicit-import");
 
   // Enable param entry values functionlaity.
-  if (Args.hasArg(options::OPT_femit_param_entry_values))
+  if (Args.hasArg(options::OPT_femit_param_entry_values) &&
+  areOptimizationsEnabled(Args)) {
 CmdArgs.push_back("-femit-param-entry-values");
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-emit-entry-values=1");
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-emit-call-site-info=1");
+
+  }
 
   RenderDebugInfoCompressionArgs(Args, CmdArgs, D, TC);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58033: Add option for emitting dbg info for call site parameters

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 195571.
djtodoro added a comment.

-Fix comments


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

https://reviews.llvm.org/D58033

Files:
  include/clang/Basic/CodeGenOptions.def
  include/clang/Driver/Options.td
  lib/CodeGen/CGDebugInfo.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp


Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -746,6 +746,7 @@
 
   Opts.DisableLLVMPasses = Args.hasArg(OPT_disable_llvm_passes);
   Opts.DisableLifetimeMarkers = Args.hasArg(OPT_disable_lifetimemarkers);
+  Opts.EnableParamEntryValues = Args.hasArg(OPT_femit_param_entry_values);
   Opts.DisableO0ImplyOptNone = Args.hasArg(OPT_disable_O0_optnone);
   Opts.DisableRedZone = Args.hasArg(OPT_disable_red_zone);
   Opts.IndirectTlsSegRefs = Args.hasArg(OPT_mno_tls_direct_seg_refs);
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3396,6 +3396,10 @@
   if (DebuggerTuning == llvm::DebuggerKind::SCE)
 CmdArgs.push_back("-dwarf-explicit-import");
 
+  // Enable param entry values functionality.
+  if (Args.hasArg(options::OPT_femit_param_entry_values))
+CmdArgs.push_back("-femit-param-entry-values");
+
   RenderDebugInfoCompressionArgs(Args, CmdArgs, D, TC);
 }
 
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -4558,7 +4558,10 @@
   // were part of DWARF v4.
   bool SupportsDWARFv4Ext =
   CGM.getCodeGenOpts().DwarfVersion == 4 &&
-  CGM.getCodeGenOpts().getDebuggerTuning() == llvm::DebuggerKind::LLDB;
+  (CGM.getCodeGenOpts().getDebuggerTuning() == llvm::DebuggerKind::LLDB ||
+   (CGM.getCodeGenOpts().EnableParamEntryValues &&
+   CGM.getCodeGenOpts().getDebuggerTuning() == llvm::DebuggerKind::GDB));
+
   if (!SupportsDWARFv4Ext && CGM.getCodeGenOpts().DwarfVersion < 5)
 return llvm::DINode::FlagZero;
 
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -916,6 +916,9 @@
 def fjump_tables : Flag<["-"], "fjump-tables">, Group;
 def fno_jump_tables : Flag<["-"], "fno-jump-tables">, Group, 
Flags<[CC1Option]>,
   HelpText<"Do not use jump tables for lowering switches">;
+def femit_param_entry_values : Flag<["-"], "femit-param-entry-values">, 
Group,
+  Flags<[CC1Option]>,
+  HelpText<"Enables debug info about call site parameter's entry values">;
 def fforce_enable_int128 : Flag<["-"], "fforce-enable-int128">,
   Group, Flags<[CC1Option]>,
   HelpText<"Enable support for int128_t type">;
Index: include/clang/Basic/CodeGenOptions.def
===
--- include/clang/Basic/CodeGenOptions.def
+++ include/clang/Basic/CodeGenOptions.def
@@ -61,6 +61,7 @@
 CODEGENOPT(DebugPassManager, 1, 0) ///< Prints debug information for the new
///< pass manager.
 CODEGENOPT(DisableRedZone, 1, 0) ///< Set when -mno-red-zone is enabled.
+CODEGENOPT(EnableParamEntryValues, 1, 0) ///< Emit call site parameter dbg info
 CODEGENOPT(IndirectTlsSegRefs, 1, 0) ///< Set when -mno-tls-direct-seg-refs
  ///< is specified.
 CODEGENOPT(DisableTailCalls  , 1, 0) ///< Do not emit tail calls.


Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -746,6 +746,7 @@
 
   Opts.DisableLLVMPasses = Args.hasArg(OPT_disable_llvm_passes);
   Opts.DisableLifetimeMarkers = Args.hasArg(OPT_disable_lifetimemarkers);
+  Opts.EnableParamEntryValues = Args.hasArg(OPT_femit_param_entry_values);
   Opts.DisableO0ImplyOptNone = Args.hasArg(OPT_disable_O0_optnone);
   Opts.DisableRedZone = Args.hasArg(OPT_disable_red_zone);
   Opts.IndirectTlsSegRefs = Args.hasArg(OPT_mno_tls_direct_seg_refs);
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3396,6 +3396,10 @@
   if (DebuggerTuning == llvm::DebuggerKind::SCE)
 CmdArgs.push_back("-dwarf-explicit-import");
 
+  // Enable param entry values functionality.
+  if (Args.hasArg(options::OPT_femit_param_entry_values))
+CmdArgs.push_back("-femit-param-entry-values");
+
   RenderDebugInfoCompressionArgs(Args, CmdArgs, D, TC);
 }
 
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -4558,

[PATCH] D60827: [rename] Deduplicate symbol occurrences

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added reviewers: arphaman, kadircet.
Herald added subscribers: dexonsmith, jkorous.
Herald added a project: clang.

SymbolOccurrences is not guaranteed to be unique -- as some parts of the
AST may get traversed twice, we may add duplicated occurrences, we
should deduplicate them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60827

Files:
  clang-tools-extra/unittests/clangd/ClangdTests.cpp
  clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp


Index: clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -79,8 +79,8 @@
 
   // Non-visitors:
 
-  /// Returns a set of unique symbol occurrences. Duplicate or
-  /// overlapping occurrences are erroneous and should be reported!
+  /// Returns a set of unique symbol occurrences. Overlapping occurrences are
+  /// erroneous and should be reported!
   SymbolOccurrences takeOccurrences() { return std::move(Occurrences); }
 
 private:
@@ -95,9 +95,20 @@
 
 // The token of the source location we find actually has the old
 // name.
-if (Offset != StringRef::npos)
-  Occurrences.emplace_back(PrevName, SymbolOccurrence::MatchingSymbol,
-   BeginLoc.getLocWithOffset(Offset));
+if (Offset != StringRef::npos) {
+  SymbolOccurrence NewOccurrence = {PrevName,
+SymbolOccurrence::MatchingSymbol,
+BeginLoc.getLocWithOffset(Offset)};
+  // Don't insert duplicated occurrences.
+  auto It = llvm::find_if(
+  Occurrences, [&NewOccurrence](const SymbolOccurrence &L) {
+return std::make_pair(L.getKind(), L.getNameRanges().vec()) ==
+   std::make_pair(NewOccurrence.getKind(),
+  NewOccurrence.getNameRanges().vec());
+  });
+  if (It == Occurrences.end())
+Occurrences.push_back(std::move(NewOccurrence));
+}
   }
 
   const std::set USRSet;
Index: clang-tools-extra/unittests/clangd/ClangdTests.cpp
===
--- clang-tools-extra/unittests/clangd/ClangdTests.cpp
+++ clang-tools-extra/unittests/clangd/ClangdTests.cpp
@@ -1157,6 +1157,33 @@
 Field(&CodeCompletion::Scope, "ns::";
 }
 
+TEST_F(ClangdVFSTest, NoDuplicatedTextEditsOnRename) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+
+  ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  Annotations Code(R"cpp(
+struct [[Foo]] {};
+[[Foo]] test() {
+ [[F^oo]] x;
+ return x;
+}
+  )cpp");
+
+  runAddDocument(Server, FooCpp, Code.code());
+
+  auto TextEdits =
+  llvm::cantFail(runRename(Server, FooCpp, Code.point(), "new_name"));
+  std::vector ReplacementRanges;
+  for (const auto& TestEdit : TextEdits)
+ReplacementRanges.push_back(TestEdit.range);
+  EXPECT_THAT(ReplacementRanges,
+  testing::UnorderedElementsAreArray(Code.ranges()));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang


Index: clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ clang/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -79,8 +79,8 @@
 
   // Non-visitors:
 
-  /// Returns a set of unique symbol occurrences. Duplicate or
-  /// overlapping occurrences are erroneous and should be reported!
+  /// Returns a set of unique symbol occurrences. Overlapping occurrences are
+  /// erroneous and should be reported!
   SymbolOccurrences takeOccurrences() { return std::move(Occurrences); }
 
 private:
@@ -95,9 +95,20 @@
 
 // The token of the source location we find actually has the old
 // name.
-if (Offset != StringRef::npos)
-  Occurrences.emplace_back(PrevName, SymbolOccurrence::MatchingSymbol,
-   BeginLoc.getLocWithOffset(Offset));
+if (Offset != StringRef::npos) {
+  SymbolOccurrence NewOccurrence = {PrevName,
+SymbolOccurrence::MatchingSymbol,
+BeginLoc.getLocWithOffset(Offset)};
+  // Don't insert duplicated occurrences.
+  auto It = llvm::find_if(
+  Occurrences, [&NewOccurrence](const SymbolOccurrence &L) {
+return std::make_pair(L.getKind(), L.getNameRanges().vec()) ==
+   std::make_pair(NewOccurrence.getKind(),
+  NewOccurrence.getNameRanges().vec());
+  });
+  if (It == Occurrences.end())
+Occurrences.push_back(std::move(NewOccurrence));
+}
   }
 
   const std::set U

r358582 - Explicitly say we don't define new/delete in libc++ during Apple stage1 bootstrap

[Louis Dionne via cfe-commits]

Author: ldionne
Date: Wed Apr 17 07:58:59 2019
New Revision: 358582

URL: http://llvm.org/viewvc/llvm-project?rev=358582&view=rev
Log:
Explicitly say we don't define new/delete in libc++ during Apple stage1 
bootstrap

This is not necessary in stage2 because we don't even build libc++.dylib
there.

Modified:
cfe/trunk/cmake/caches/Apple-stage1.cmake

Modified: cfe/trunk/cmake/caches/Apple-stage1.cmake
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/cmake/caches/Apple-stage1.cmake?rev=358582&r1=358581&r2=358582&view=diff
==
--- cfe/trunk/cmake/caches/Apple-stage1.cmake (original)
+++ cfe/trunk/cmake/caches/Apple-stage1.cmake Wed Apr 17 07:58:59 2019
@@ -33,6 +33,9 @@ set(COMPILER_RT_ENABLE_TVOS OFF CACHE BO
 set(BOOTSTRAP_LLVM_ENABLE_LTO ON CACHE BOOL "")
 set(CMAKE_BUILD_TYPE RelWithDebInfo CACHE STRING "")
 
+set(LIBCXX_ENABLE_NEW_DELETE_DEFINITIONS OFF CACHE BOOL "")
+set(LIBCXXABI_ENABLE_NEW_DELETE_DEFINITIONS ON CACHE BOOL "")
+
 set(CLANG_BOOTSTRAP_TARGETS
   generate-order-file
   check-all


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

[PATCH] D58043: Add experimental options for call site related dbg info

[Djordje Todorovic via Phabricator via cfe-commits]

djtodoro updated this revision to Diff 195572.
djtodoro added a comment.

-Rebase


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

https://reviews.llvm.org/D58043

Files:
  lib/Driver/ToolChains/Clang.cpp


Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3397,8 +3397,14 @@
 CmdArgs.push_back("-dwarf-explicit-import");
 
   // Enable param entry values functionality.
-  if (Args.hasArg(options::OPT_femit_param_entry_values))
+  if (Args.hasArg(options::OPT_femit_param_entry_values) &&
+  areOptimizationsEnabled(Args)) {
 CmdArgs.push_back("-femit-param-entry-values");
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-emit-entry-values=1");
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-emit-call-site-info=1");
+  }
 
   RenderDebugInfoCompressionArgs(Args, CmdArgs, D, TC);
 }


Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3397,8 +3397,14 @@
 CmdArgs.push_back("-dwarf-explicit-import");
 
   // Enable param entry values functionality.
-  if (Args.hasArg(options::OPT_femit_param_entry_values))
+  if (Args.hasArg(options::OPT_femit_param_entry_values) &&
+  areOptimizationsEnabled(Args)) {
 CmdArgs.push_back("-femit-param-entry-values");
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-emit-entry-values=1");
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-emit-call-site-info=1");
+  }
 
   RenderDebugInfoCompressionArgs(Args, CmdArgs, D, TC);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

It would be better to rename `clang/test/SemaSYCL/device-attrubutes.cpp` to 
`clang/test/SemaSYCL/device-attributes.cpp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455



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

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added a comment.

In D60455#1470318 , @keryell wrote:

> It would be better to rename `clang/test/SemaSYCL/device-attrubutes.cpp` to 
> `clang/test/SemaSYCL/device-attributes.cpp`


It's already renamed in the latest patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455



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

[PATCH] D59744: Fix i386 ABI "__m64" type bug

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added a comment.

One last style comment from me but we need somebody better with the different 
ABIs to finally approve this.




Comment at: lib/CodeGen/TargetInfo.cpp:1416
+  return ABIArgInfo::getDirect(llvm::Type::getX86_MMXTy(getVMContext()));
+}
+

superfluous braces?


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

https://reviews.llvm.org/D59744



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-prefer-isa-or-dyn-cast-in-conditionals

[Don Hinton via Phabricator via cfe-commits]

hintonda marked an inline comment as done.
hintonda added inline comments.



Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:27-28
 CheckFactories.registerCheck("llvm-include-order");
+CheckFactories.registerCheck(
+"llvm-prefer-isa-or-dyn-cast-in-conditionals");
 CheckFactories.registerCheck(

hintonda wrote:
> aaron.ballman wrote:
> > hintonda wrote:
> > > aaron.ballman wrote:
> > > > Please keep this list sorted alphabetically.
> > > I think this is where add_new_check.py put it, but I'll alphabetize.
> > Huh, I thought that script kept everything alphabetized? That may be worth 
> > investigating (don't feel obligated though).
> I'm sure it's something I did inadvertently, but can't really remember the 
> details. 
Looks like rename_check.py definitely does not alphabetize.  I'll try to 
address than in another patch.  Thanks for pointing this out. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802



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

[PATCH] D60828: [ARM] Fix armv8 features tree and add fp16fml

[Diogo N. Sampaio via Phabricator via cfe-commits]

dnsampaio created this revision.
dnsampaio added reviewers: ostannard, DavidSpickett.
Herald added subscribers: cfe-commits, kristof.beyls, javed.absar.
Herald added a project: clang.

This patch adds the fp16fml feature parser as well fixes
the FPU and the HW_FP flags when +fullfp16 and +dotprod 
features are passed, to account for pre-requisite features.

The ARM backend (ARM.td) defines this tree of feature dependencies:

   fp16  vfp3
 |  /   |
vfp4neon
 |  \
  fp-armv8   FeatureDotProd
 |
  fullfp16
 |
  fp16fml

However, clang does not capture that when using +fullfp16 we
also have vfp4, so compiling
tools/clang/test/CodeGen/arm_neon_intrinsics.c

with

  clang -target armv8a-linux-eabi -march=armv8.4-a+fp16 -S -emit-llvm

will give an error because vfp4 is not added to the FPU flag.

As test now we can compile that test file with the command

  clang -target armv8a-linux-eabi -march=armv8-a+fp16fml -S -emit-llvm


Repository:
  rC Clang

https://reviews.llvm.org/D60828

Files:
  lib/Basic/Targets/ARM.cpp
  test/CodeGen/arm_neon_intrinsics.c
  test/CodeGen/arm_neon_intrinsics.c.v8



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

[PATCH] D60629: [clang-tidy] Change the namespace for llvm checkers from 'llvm' to 'llvm_check'

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 195581.
hintonda added a comment.

- Remove clang module check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60629

Files:
  clang-tools-extra/clang-tidy/add_new_check.py
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.h
  clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.cpp
  clang-tools-extra/clang-tidy/llvm/IncludeOrderCheck.h
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/TwineLocalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/TwineLocalCheck.h
  clang-tools-extra/clang-tidy/rename_check.py
  clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/LLVMModuleTest.cpp
@@ -3,7 +3,7 @@
 #include "llvm/IncludeOrderCheck.h"
 #include "gtest/gtest.h"
 
-using namespace clang::tidy::llvm;
+using namespace clang::tidy::llvm_check;
 
 namespace clang {
 namespace tidy {
Index: clang-tools-extra/clang-tidy/rename_check.py
===
--- clang-tools-extra/clang-tidy/rename_check.py
+++ clang-tools-extra/clang-tidy/rename_check.py
@@ -14,6 +14,18 @@
 import re
 
 
+def replaceInFileRegex(fileName, sFrom, sTo):
+  if sFrom == sTo:
+return
+  txt = None
+  with open(fileName, "r") as f:
+txt = f.read()
+
+  txt = re.sub(sFrom, sTo, txt)
+  print("Replacing '%s' -> '%s' in '%s'..." % (sFrom, sTo, fileName))
+  with open(fileName, "w") as f:
+f.write(txt)
+
 def replaceInFile(fileName, sFrom, sTo):
   if sFrom == sTo:
 return
@@ -203,6 +215,8 @@
   clang_tidy_path = os.path.dirname(__file__)
 
   header_guard_variants = [
+  (args.old_check_name.replace('-', '_') + '_Check').upper(),
+  (old_module + '_' + check_name_camel).upper(),
   (old_module + '_' + new_check_name_camel).upper(),
   args.old_check_name.replace('-', '_').upper()]
   header_guard_new = (new_module + '_' + new_check_name_camel).upper()
@@ -210,18 +224,19 @@
   old_module_path = os.path.join(clang_tidy_path, old_module)
   new_module_path = os.path.join(clang_tidy_path, new_module)
 
-  # Remove the check from the old module.
-  cmake_lists = os.path.join(old_module_path, 'CMakeLists.txt')
-  check_found = deleteMatchingLines(cmake_lists, '\\b' + check_name_camel)
-  if not check_found:
-print("Check name '%s' not found in %s. Exiting." %
+  if (args.old_check_name != args.new_check_name):
+# Remove the check from the old module.
+cmake_lists = os.path.join(old_module_path, 'CMakeLists.txt')
+check_found = deleteMatchingLines(cmake_lists, '\\b' + check_name_camel)
+if not check_found:
+  print("Check name '%s' not found in %s. Exiting." %
 (check_name_camel, cmake_lists))
-return 1
+  return 1
 
-  modulecpp = filter(
-  lambda p: p.lower() == old_module.lower() + 'tidymodule.cpp',
-  os.listdir(old_module_path))[0]
-  deleteMatchingLines(os.path.join(old_module_path, modulecpp),
+modulecpp = filter(
+lambda p: p.lower() == old_module.lower() + 'tidymodule.cpp',
+os.listdir(old_module_path))[0]
+deleteMatchingLines(os.path.join(old_module_path, modulecpp),
   '\\b' + check_name_camel + '|\\b' + args.old_check_name)
 
   for filename in getListOfFiles(clang_tidy_path):
@@ -249,14 +264,21 @@
   new_module + '/' + new_check_name_camel)
 replaceInFile(filename, check_name_camel, new_check_name_camel)
 
-  if old_module != new_module:
+  if old_module != new_module or new_module == 'llvm':
+if new_module == 'llvm':
+  new_namespace = new_module + '_check'
+else:
+  new_namespace = new_module
 check_implementation_files = glob.glob(
 os.path.join(old_module_path, new_check_name_camel + '*'))
 for filename in check_implementation_files:
   # Move check implementation to the directory of the new module.
   filename = fileRename(filename, old_module_path, new_module_path)
-  replaceInFile(filename, 'namespace ' + old_module,
-'namespace ' + new_module)
+  replaceInFileRegex(filename, 'namespace ' + old_module + '[^ \n]*',
+ 'namespace ' + new_namespace)
+
+  if (args.old_check_name == args.new_check_name):
+return
 
   # Add check to the new module.
   adapt_cmake(new_module_path, new_check_name_camel)
Index: clang-tools-extra/clang-tidy/llvm/TwineLocalCheck.h
===
--- clang-tools-extra/clang-tidy/llvm/TwineLocalCheck.h
+++ clang-tools-extra/clang-tidy/llvm/TwineLocalCheck.h
@@ -6,14 +6,14 @@
 //
 //===

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D60778#1470188 , @riccibruno wrote:

> Maybe, but is there an equivalent to `getTypeID` for declarations ?


`getDeclID` is used in the first patch...

although now I am thinking may be original patch is a better approach?


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

https://reviews.llvm.org/D60778



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

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.



> 
> 
>> I think what we are missing currently is a thorough analysis/comparison 
>> between SYCL device mode and OpenCL kernel language mode to understand 
>> what's the best implementation strategy. That would apply to many other 
>> features: kernel function restrictions, address spaces, vectors, special 
>> types, etc.
> 
> That would make definitely sense when we target OpenCL.
> 
>> I still see no point in polluting our code base with extra code that just 
>> does the same thing. It will save us a lot of time to just work 
>> cooperatively on the same problem and even improve readability of the code. 
>> But of course this can only be done if there is no need to diverge the 
>> implementation significantly.
> 
> Yes. Probably that even when the target is not OpenCL, the general principles 
> remain similar. Probably the same for CUDA & OpenMP 5 too...

In the interest of speeding up the upstreaming work, would you be able to 
highlight the differences and similarity at least for SYCL and OpenCL kernel 
modes? Not sure if you are familiar enough with both. Because apart from the 
public announcements I can't see what has been changed in SYCL that would 
disallow to use OpenCL mode. It would be a valuable input to determine the 
implementation choices.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455



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

[PATCH] D60629: [clang-tidy] Change the namespace for llvm checkers from 'llvm' to 'llvm_check'

[Don Hinton via Phabricator via cfe-commits]

hintonda marked 2 inline comments as done.
hintonda added inline comments.



Comment at: clang-tools-extra/clang-tidy/add_new_check.py:381
+  # Don't allow 'clang' or 'llvm' namespaces
+  if module == 'llvm' or module == 'clang':
+namespace = module + '_check'

alexfh wrote:
> We don't have the `clang` module. No need to check for it here. If we ever 
> add one (which I doubt), we can modify this script again. But for now let's 
> remove this to avoid confusion.
No problem.  Or we could outlaw it...



Comment at: clang-tools-extra/clang-tidy/rename_check.py:268
+  if old_module != new_module or new_module == 'llvm' or new_module == 'clang':
+if new_module == 'llvm' or new_module -- 'clang':
+  new_namespace = new_module + '_check'

alexfh wrote:
> `--`?
> 
> Anyways, we don't have (or plan to have) a module named `clang`.
Thanks for catching that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60629



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

[clang-tools-extra] r358589 - [clang-tidy] Fix invalid location in readability-misleading-indentation diagnostic

[Alexander Kornienko via cfe-commits]

Author: alexfh
Date: Wed Apr 17 09:19:47 2019
New Revision: 358589

URL: http://llvm.org/viewvc/llvm-project?rev=358589&view=rev
Log:
[clang-tidy] Fix invalid location in readability-misleading-indentation 
diagnostic

Before this patch readability-misleading-indentation could issue diagnostics
with an invalid location, which would lead to an assertion failure in
ClangTidyContext::diag()

Modified:

clang-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.cpp

clang-tools-extra/trunk/test/clang-tidy/readability-misleading-indentation.cpp

Modified: 
clang-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.cpp?rev=358589&r1=358588&r2=358589&view=diff
==
--- 
clang-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.cpp 
(original)
+++ 
clang-tools-extra/trunk/clang-tidy/readability/MisleadingIndentationCheck.cpp 
Wed Apr 17 09:19:47 2019
@@ -81,6 +81,10 @@ void MisleadingIndentationCheck::missing
 SourceLocation InnerLoc = Inner->getBeginLoc();
 SourceLocation OuterLoc = CurrentStmt->getBeginLoc();
 
+if (InnerLoc.isInvalid() || InnerLoc.isMacroID() || OuterLoc.isInvalid() ||
+OuterLoc.isMacroID())
+  continue;
+
 if (SM.getExpansionLineNumber(InnerLoc) ==
 SM.getExpansionLineNumber(OuterLoc))
   continue;
@@ -88,7 +92,7 @@ void MisleadingIndentationCheck::missing
 const Stmt *NextStmt = CStmt->body_begin()[i + 1];
 SourceLocation NextLoc = NextStmt->getBeginLoc();
 
-if (InnerLoc.isMacroID() || OuterLoc.isMacroID() || NextLoc.isMacroID())
+if (NextLoc.isInvalid() || NextLoc.isMacroID())
   continue;
 
 if (SM.getExpansionColumnNumber(InnerLoc) ==

Modified: 
clang-tools-extra/trunk/test/clang-tidy/readability-misleading-indentation.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/readability-misleading-indentation.cpp?rev=358589&r1=358588&r2=358589&view=diff
==
--- 
clang-tools-extra/trunk/test/clang-tidy/readability-misleading-indentation.cpp 
(original)
+++ 
clang-tools-extra/trunk/test/clang-tidy/readability-misleading-indentation.cpp 
Wed Apr 17 09:19:47 2019
@@ -8,7 +8,7 @@ void foo2();
 foo1();   \
 foo2();
 
-int main()
+void f()
 {
   bool cond1 = true;
   bool cond2 = true;
@@ -90,7 +90,7 @@ int main()
else {
   }
   // CHECK-MESSAGES: :[[@LINE-2]]:8: warning: different indentation for 'if' 
and corresponding 'else' [readability-misleading-indentation]
-  
+
   if (cond1) {
 if (cond1) {
 }
@@ -109,3 +109,12 @@ int main()
 
   BLOCK
 }
+
+void g(bool x) {
+  if (x)
+#pragma unroll
+for (int k = 0; k < 1; ++k) {}
+
+  #pragma unroll
+  for (int k = 0; k < 1; ++k) {}
+}


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

r358590 - Remove --show-includes flag in crash reduce script

[Amy Huang via cfe-commits]

Author: akhuang
Date: Wed Apr 17 09:20:56 2019
New Revision: 358590

URL: http://llvm.org/viewvc/llvm-project?rev=358590&view=rev
Log:
Remove --show-includes flag in crash reduce script

Modified:
cfe/trunk/utils/creduce-clang-crash.py

Modified: cfe/trunk/utils/creduce-clang-crash.py
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/utils/creduce-clang-crash.py?rev=358590&r1=358589&r2=358590&view=diff
==
--- cfe/trunk/utils/creduce-clang-crash.py (original)
+++ cfe/trunk/utils/creduce-clang-crash.py Wed Apr 17 09:20:56 2019
@@ -286,6 +286,10 @@ class Reduce(object):
 opts_startswith=["-gcodeview",
  "-debug-info-kind=",
  "-debugger-tuning="])
+
+new_args = self.try_remove_args(new_args,
+msg="Removed --show-includes",
+opts_startswith=["--show-includes"])
 # Not suppressing warnings (-w) sometimes prevents the crash from occurring
 # after preprocessing
 new_args = self.try_remove_args(new_args,


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

[PATCH] D60828: [ARM] Fix armv8 features tree and add fp16fml

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added a reviewer: simon_tatham.
ostannard added inline comments.



Comment at: lib/Basic/Targets/ARM.cpp:443
   HasLegalHalfType = true;
+  HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
+  FPU |= VFP4FPU;

Is it always correct to set HW_FP_DP here, now that MVE can have full fp16 
without double-precision? I'll add Simon since he's working on that.



Comment at: lib/Basic/Targets/ARM.cpp:444
+  HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
+  FPU |= VFP4FPU;
 } else if (Feature == "+dotprod") {

Should this be FPARMV8, since fullfp16 doesn't apply to earlier architectures? 
Maybe MVE  complicates this even further?



Comment at: lib/Basic/Targets/ARM.cpp:446
 } else if (Feature == "+dotprod") {
+  FPU |= NeonFPU;
+  HW_FP |= HW_FP_SP | HW_FP_DP;

Should this also add FPARMV8?



Comment at: lib/Basic/Targets/ARM.cpp:452
+  HasLegalHalfType = true;
+  FPU |= VFP4FPU;
+  HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;

Again, should this be FPARMV8?



Comment at: lib/Basic/Targets/ARM.cpp:453
+  FPU |= VFP4FPU;
+  HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
 }

You are adding HW_FP_HP twice.



Comment at: test/CodeGen/arm_neon_intrinsics.c:8
+// RUN: %clang -O1 -target armv8a-linux-eabi -march=armv8a+fp16fml\
+// RUN:  -S -emit-llvm -o - %s | FileCheck %s.v8
+

Does the generate code differ enough to have a second copy of it? Actually, I 
assume the problem here is that we are not setting the correct preprocessor 
macros? in which case, it would be better to test them directly, than by 
re-running this 21kloc test.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60828



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

[PATCH] D60818: [CUDA][Windows] restrict long double device functions declarations to Windows

[Artem Belevich via Phabricator via cfe-commits]

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

LGTM. Thank you for cleaning this up.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60818



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

r358595 - [OPENMP][NVPTX]Run combined constructs with if clause in SPMD mode.

[Alexey Bataev via cfe-commits]

Author: abataev
Date: Wed Apr 17 09:53:08 2019
New Revision: 358595

URL: http://llvm.org/viewvc/llvm-project?rev=358595&view=rev
Log:
[OPENMP][NVPTX]Run combined constructs with if clause in SPMD mode.

All target-parallel-based constructs can be run in SPMD mode from now
on. Even if num_threads clauses or if clauses are used, such constructs
can be executed in SPMD mode.

Modified:
cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
cfe/trunk/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
cfe/trunk/test/OpenMP/nvptx_distribute_parallel_generic_mode_codegen.cpp
cfe/trunk/test/OpenMP/nvptx_target_simd_codegen.cpp
cfe/trunk/test/OpenMP/nvptx_target_teams_distribute_simd_codegen.cpp
cfe/trunk/test/OpenMP/target_parallel_if_codegen.cpp
cfe/trunk/test/OpenMP/target_parallel_num_threads_codegen.cpp

Modified: cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp?rev=358595&r1=358594&r2=358595&view=diff
==
--- cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGOpenMPRuntime.cpp Wed Apr 17 09:53:08 2019
@@ -6657,6 +6657,47 @@ static llvm::Value *getNumThreads(CodeGe
   CGF.getContext(), CS->getCapturedStmt());
   if (const auto *Dir = dyn_cast_or_null(Child)) {
 if (isOpenMPParallelDirective(Dir->getDirectiveKind())) {
+  llvm::Value *NumThreads = nullptr;
+  llvm::Value *CondVal = nullptr;
+  // Handle if clause. If if clause present, the number of threads is
+  // calculated as  ? ( ?  : 0 ) : 1.
+  if (Dir->hasClausesOfKind()) {
+CGOpenMPInnerExprInfo CGInfo(CGF, *CS);
+CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(CGF, &CGInfo);
+const OMPIfClause *IfClause = nullptr;
+for (const auto *C : Dir->getClausesOfKind()) {
+  if (C->getNameModifier() == OMPD_unknown ||
+  C->getNameModifier() == OMPD_parallel) {
+IfClause = C;
+break;
+  }
+}
+if (IfClause) {
+  const Expr *Cond = IfClause->getCondition();
+  bool Result;
+  if (Cond->EvaluateAsBooleanCondition(Result, CGF.getContext())) {
+if (!Result)
+  return CGF.Builder.getInt32(1);
+  } else {
+CodeGenFunction::LexicalScope Scope(CGF, Cond->getSourceRange());
+if (const auto *PreInit =
+cast_or_null(IfClause->getPreInitStmt())) {
+  for (const auto *I : PreInit->decls()) {
+if (!I->hasAttr()) {
+  CGF.EmitVarDecl(cast(*I));
+} else {
+  CodeGenFunction::AutoVarEmission Emission =
+  CGF.EmitAutoVarAlloca(cast(*I));
+  CGF.EmitAutoVarCleanups(Emission);
+}
+  }
+}
+CondVal = CGF.EvaluateExprAsBool(Cond);
+  }
+}
+  }
+  // Check the value of num_threads clause iff if clause was not specified
+  // or is not evaluated to false.
   if (Dir->hasClausesOfKind()) {
 CGOpenMPInnerExprInfo CGInfo(CGF, *CS);
 CodeGenFunction::CGCapturedStmtRAII CapInfoRAII(CGF, &CGInfo);
@@ -6676,19 +6717,23 @@ static llvm::Value *getNumThreads(CodeGe
 }
   }
 }
-llvm::Value *NumThreads =
-CGF.EmitScalarExpr(NumThreadsClause->getNumThreads());
+NumThreads = CGF.EmitScalarExpr(NumThreadsClause->getNumThreads());
 NumThreads = CGF.Builder.CreateIntCast(NumThreads, CGF.Int32Ty,
-   /*IsSigned=*/true);
-return DefaultThreadLimitVal
-   ? CGF.Builder.CreateSelect(
- CGF.Builder.CreateICmpULT(DefaultThreadLimitVal,
-   NumThreads),
- DefaultThreadLimitVal, NumThreads)
-   : NumThreads;
+   /*IsSigned=*/false);
+if (DefaultThreadLimitVal)
+  NumThreads = CGF.Builder.CreateSelect(
+  CGF.Builder.CreateICmpULT(DefaultThreadLimitVal, NumThreads),
+  DefaultThreadLimitVal, NumThreads);
+  } else {
+NumThreads = DefaultThreadLimitVal ? DefaultThreadLimitVal
+   : CGF.Builder.getInt32(0);
+  }
+  // Process condition of the if clause.
+  if (CondVal) {
+NumThreads = CGF.Builder.CreateSelect(CondVal, NumThreads,
+  CGF.Builder.getInt32(1));
   }
-  return DefaultThreadLimitVal ? DefaultThreadLimitVal
-   : CGF.Builder.getInt32(0);
+  return NumThreads;
 }
 if (isOpenMPSimdDirective(Dir->getDirectiveKind()))
   return CGF.Builder.getInt32(1);
@@ -6748,7 +6793,7 @@ emitNumThreadsForTargetDirecti

[PATCH] D58236: Make address space conversions a bit stricter.

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.

Alright, this seems reasonable to me.


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

https://reviews.llvm.org/D58236



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

[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 195589.
ahatanak marked 2 inline comments as done.
ahatanak added a comment.

Remove a potentially expensive check.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60736

Files:
  include/clang/AST/DeclBase.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseObjc.cpp
  lib/Sema/SemaDeclObjC.cpp
  lib/Sema/SemaExpr.cpp
  test/SemaObjC/warn-implicit-self-in-block.m
  test/SemaObjCXX/warn-implicit-self-in-block.mm

Index: test/SemaObjCXX/warn-implicit-self-in-block.mm
===
--- /dev/null
+++ test/SemaObjCXX/warn-implicit-self-in-block.mm
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -x objective-c++ -std=c++11 -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
+// rdar://11194874
+
+typedef void (^BlockTy)();
+
+void noescapeFunc(__attribute__((noescape)) BlockTy);
+void escapeFunc(BlockTy);
+
+@interface Root @end
+
+@interface I : Root
+{
+  int _bar;
+}
+@end
+
+@implementation I
+  - (void)foo{
+  ^{
+   _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+   }();
+  }
+
+  - (void)testNested{
+noescapeFunc(^{
+  (void)_bar;
+  escapeFunc(^{
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+noescapeFunc(^{
+  (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+});
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+  });
+  (void)_bar;
+});
+  }
+
+  - (void)testLambdaInBlock{
+noescapeFunc(^{ [&](){ (void)_bar; }(); });
+escapeFunc(^{ [&](){ (void)_bar; }(); }); // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+  }
+@end
Index: test/SemaObjC/warn-implicit-self-in-block.m
===
--- test/SemaObjC/warn-implicit-self-in-block.m
+++ /dev/null
@@ -1,18 +0,0 @@
-// RUN: %clang_cc1 -x objective-c -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
-// rdar://11194874
-
-@interface Root @end
-
-@interface I : Root
-{
-  int _bar;
-}
-@end
-
-@implementation I
-  - (void)foo{
-  ^{
-   _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
-   }();
-  }
-@end
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -2575,11 +2575,9 @@
 !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc))
   getCurFunction()->recordUseOfWeak(Result);
   }
-  if (getLangOpts().ObjCAutoRefCount) {
-if (CurContext->isClosure())
-  Diag(Loc, diag::warn_implicitly_retains_self)
-<< FixItHint::CreateInsertion(Loc, "self->");
-  }
+  if (getLangOpts().ObjCAutoRefCount)
+if (const BlockDecl *BD = CurContext->getInnerMostBlockDecl())
+  ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
 
   return Result;
 }
Index: lib/Sema/SemaDeclObjC.cpp
===
--- lib/Sema/SemaDeclObjC.cpp
+++ lib/Sema/SemaDeclObjC.cpp
@@ -359,6 +359,7 @@
 /// ActOnStartOfObjCMethodDef - This routine sets up parameters; invisible
 /// and user declared, in the method definition's AST.
 void Sema::ActOnStartOfObjCMethodDef(Scope *FnBodyScope, Decl *D) {
+  ImplicitlyRetainedSelfLocs.clear();
   assert((getCurMethodDecl() == nullptr) && "Methodparsing confused");
   ObjCMethodDecl *MDecl = dyn_cast_or_null(D);
 
@@ -494,6 +495,35 @@
   }
 }
 
+void Sema::ActOnEndOfObjCMethodDef() {
+  llvm::DenseMap EscapeInfo;
+
+  auto IsOrNestedInEscapingBlock = [&](const BlockDecl *BD) {
+if (EscapeInfo.count(BD))
+  return EscapeInfo[BD];
+
+bool R = false;
+const BlockDecl *CurBD = BD;
+
+do {
+  R = R || !CurBD->doesNotEscape();
+  if (R)
+break;
+  CurBD = CurBD->getParent()->getInnerMostBlockDecl();
+} while (CurBD);
+
+return EscapeInfo[BD] = R;
+  };
+
+  // If the location where 'self' is implicitly retained is inside a escaping
+  // block, emit a diagnostic.
+  for (const std::pair &P :
+   ImplicitlyRetainedSelfLocs)
+if (IsOrNestedInEscapingBlock(P.second))
+  Diag(P.first, diag::warn_implicitly_retains_self)
+  << FixItHint::CreateInsertion(P.first, "self->");
+}
+
 namespace {
 
 // Callback to only accept typo corrections that are Objective-C classes.
Index: lib/Parse/ParseObjc.cpp
===
--- lib/Parse/ParseO

[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:2580
+if (const BlockDecl *BD = CurContext->getInnerMostBlockDecl())
+  if (!getDiagnostics().isIgnored(diag::warn_implicitly_retains_self, 
Loc))
+ImplicitlyRetainedSelfLocs.push_back({Loc, BD});

rjmccall wrote:
> IIRC this check can be expensive enough that it's probably not worth doing if 
> you expect these entries to typically not result in diagnostics.
`DiagStateMap::lookup` is doing a binary search. Is that what makes this check 
expensive?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60736



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

[PATCH] D60778: Make precompiled headers reproducible by switching OpenCL extension to std::map

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D60778#1470176 , @lebedev.ri wrote:

> In D60778#1470152 , @Anastasia wrote:
>
> > In D60778#1468825 , @lebedev.ri 
> > wrote:
> >
> > > > Not sure how to test this though? I guess we can trust that std::map is 
> > > > always sorted just as it says in its description.
> > >
> > > You could add a test that contains several entries in `OpenCLTypeExtMap` 
> > > and several entries in `OpenCLDeclExtMap`.
> > >  In that test, write te PCH, and then apply `llvm-bcanalyzer` to that 
> > > PCH. It should dump it the bitcode in textual dump.
> > >  And then simply apply FileCheck to that textual dump, with CHECK lines 
> > > requiring the specific order of these entries.
> > >  If the order is not deterministic in PCH, then that test would (should!) 
> > > fail sporadically.
> > >  At least that is my guess.
> >
> >
> > Ok I could do that, but I guess it can then fail on the commits that didn't 
> > actually trigger the issue. Would it not be a problem?
>
>
> Why would it fail if this fixes the issue?


But once something is changed and it's no longer deterministic, we might not be 
able to detect this necessarily on the offending commit with such test?

> Re randomness - see https://reviews.llvm.org/D59401#change-QMkBH7328Dyv


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

https://reviews.llvm.org/D60778



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

[PATCH] D59371: [LibTooling] Add Stencil library for format-string style codegen.

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:45
+  /// Evaluates this part to a string and appends it to `result`.
+  virtual llvm::Error eval(const ast_matchers::MatchFinder::MatchResult &Match,
+   std::string *Result) const = 0;

ymandel wrote:
> sbenza wrote:
> > Why not use `llvm::Expected` as the return type?
> so that each stencil part can append to a single string that we construct 
> when evaluating the whole stencil.  this was an (attempted?) optimization. if 
> concatenating is about as efficient, I'd prefer that approach. What do you 
> advise?
I think its fine as it is then.
I don't really think the performance would matter that much, but I don't see a 
reason to add all those copies unnecessarily.
Mention that `Result` is in an unspecified state in case of error.



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:127
+  // Allow Stencils to operate as std::function, for compatibility with
+  // Transformer's TextGenerator. Calls `eval()` and asserts on failure.
+  std::string operator()(const ast_matchers::MatchFinder::MatchResult &) const;

ymandel wrote:
> sbenza wrote:
> > "asserts" as it only fails in debug mode?
> I thought `assert()` also fails in normal mode, based on my reading of the 
> llvm docs, but it's not explicit about this: 
> http://llvm.org/docs/ProgrammersManual.html#programmatic-errors
> 
> FWIW, I'm on the fence whether `eval()` should actually have the same 
> signature and similarly just crash the program on errors. Its not clear 
> there's any value to propogating the errors up the stack via `Expected`.
`assert` follows `NDEBUG`, which is not explicitly related to `-Ox`, but they 
usually go together.

Don't make it crash, please.
Otherwise it would be hard to have a refactoring service or alike.



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:101
+S.Parts.reserve(sizeof...(Parts));
+auto Unused = {(S.push(std::forward(Parts)), true)...};
+(void)Unused;

We could simplify this code if you change `void push(T)` to instead 
`StencilPart wrap(T)`.
Then this could be:


```
Stencil S;
S.Parts = {wrap(std::forward(Parts))...};
return S;
```



Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:78
+
+template <> bool isEqualData(const RawTextData &A, const RawTextData &B) {
+  return A.Text == B.Text;

Any particular reason to use function template specialization instead of 
function overloading?
The former is rare and thus harder to reason about.

(same for `evalData` below)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59371



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

[PATCH] D60455: [SYCL] Add support for SYCL device attributes

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

In D60455#1470030 , @Anastasia wrote:

>




> I am not sure we need to add a keyword actually, the attribute can just be 
> added in AST since it's not supposed to be used in the source code?

The attribute is used by the SYCL headers to mark the functions to be outlined 
to the device.
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L267
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L295
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L308
https://github.com/intel/llvm/blob/sycl/sycl/include/CL/sycl/handler.hpp#L325

> My understanding of SYCL kernel is that it mainly matches OpenCL kernel 
> functionality because the original intent of SYCL was to provide single 
> source functionality on top of OpenCL.

Yes, this was the idea, when OpenCL was announced in November 2008, to build 
some higher-level programming models on top of it.
Now the SYCL standard is evolving to something more general to bring 
heterogeneous computing to modern C++.
So we could reuse in the same way some attributes from OpenMP 5 or CUDA if the 
Clang/LLVM community thinks it is better.

> But I am not an expert in SYCL to confirm that.

I am pretty sure that the SYCL standard committee would love some active 
participation from ARM. ;-)

> I think what we are missing currently is a thorough analysis/comparison 
> between SYCL device mode and OpenCL kernel language mode to understand what's 
> the best implementation strategy. That would apply to many other features: 
> kernel function restrictions, address spaces, vectors, special types, etc.

That would make definitely sense when we target OpenCL.

> I still see no point in polluting our code base with extra code that just 
> does the same thing. It will save us a lot of time to just work cooperatively 
> on the same problem and even improve readability of the code. But of course 
> this can only be done if there is no need to diverge the implementation 
> significantly.

Yes. Probably that even when the target is not OpenCL, the general principles 
remain similar. Probably the same for CUDA & OpenMP 5 too...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60455



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

[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard edited reviewers, added: ostannard; removed: olista01.
ostannard added a comment.

For the auto-upgrader, these limited FPUs only exist for microcontrollers, 
where I doubt any users are keeping bitcode files around for a long time, so 
I'd be fine with not doing this. I've had a skim through the auto-upgrader 
code, and I don't see any other target-specific attributes which are handled 
there, though there are some target-specific intrinsics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60691



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

[PATCH] D60800: [MS] Emit S_HEAPALLOCSITE debug info

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

I don't really know about the functionality here, just adding a few comments on 
the code itself.




Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1966
+  QualType PointeeTy = D.getTypePtr()->getPointeeType();
+  llvm::DIType *DI = getOrCreateType(PointeeTy, getOrCreateFile(Loc));
+  CI->setMetadata("heapallocsite", DI);

I don't really know this code, but does this also work for void pointers, i.e. 
the if statement in the old code was unnecessary?



Comment at: clang/test/CodeGen/debug-info-codeview-heapallocsite.c:19
+// FIXME: Currently not testing that call_alloc_void has metadata because the
+// metadata is not set when the type is void.
+

Is it not set with your new code though?



Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1078
+  MCSymbol *EndLabel = std::get<1>(HeapAllocSite);
+  DIType *DITy = cast(std::get<2>(HeapAllocSite));
+

Is the cast necessary? Couldn't the tuple member be made a DIType* in the first 
place?



Comment at: llvm/test/CodeGen/X86/label-heapallocsite.ll:1
+; RUN: llc -O0 < %s | FileCheck %s
+; FIXME: Add test for llc with optimizations once it is implemented.

Does llc have a "-fast-isel" flag or similar that could be used instead, to 
make it more clear that it's fast-isel that's significant for the test?



Comment at: llvm/test/CodeGen/X86/label-heapallocsite.ll:16
+; ModuleID = 'heapallocsite.c'
+source_filename = "heapallocsite.c"
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"

I guess the ModuleID and source_filename are unnecessary, so I'd dro pthem.



Comment at: llvm/test/CodeGen/X86/label-heapallocsite.ll:51
+attributes #0 = { noinline nounwind optnone 
"correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" 
"less-precise-fpmad"="false" "min-legal-vector-width"="0" 
"no-frame-pointer-elim"="false" "no-infs-fp-math"="false" 
"no-jump-tables"="false" "no-nans-fp-math"="false" 
"no-signed-zeros-fp-math"="false" "no-trapping-math"="false" 
"stack-protector-buffer-size"="8" "target-features"="+cx8,+mmx,+sse,+sse2,+x87" 
"unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { "correctly-rounded-divide-sqrt-fp-math"="false" 
"disable-tail-calls"="false" "less-precise-fpmad"="false" 
"no-frame-pointer-elim"="false" "no-infs-fp-math"="false" 
"no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" 
"no-trapping-math"="false" "stack-protector-buffer-size"="8" 
"target-features"="+cx8,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" 
"use-soft-float"="false" }
+

Both sets of attributes seem unnecessary so could probably be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60800



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

[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/DeclBase.h:1798
+  /// innermost enclosing BlockDecl or null if there are no enclosing blocks.
+  const BlockDecl *getInnerMostBlockDecl() const {
+const DeclContext *Ctx = this;

"innermost" is one word, so this should be `getInnermostBlockDecl`.



Comment at: lib/Sema/SemaExpr.cpp:2580
+if (const BlockDecl *BD = CurContext->getInnerMostBlockDecl())
+  if (!getDiagnostics().isIgnored(diag::warn_implicitly_retains_self, 
Loc))
+ImplicitlyRetainedSelfLocs.push_back({Loc, BD});

ahatanak wrote:
> rjmccall wrote:
> > IIRC this check can be expensive enough that it's probably not worth doing 
> > if you expect these entries to typically not result in diagnostics.
> `DiagStateMap::lookup` is doing a binary search. Is that what makes this 
> check expensive?
Yeah.  It's not immensely expensive, but adding some entries to a vector in the 
fast path and then processing them later probably makes more sense.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60736



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

[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.

[Dave Green via Phabricator via cfe-commits]

dmgreen added a comment.

I don't think we here care about auto-updating, not supporting bitcode/lto 
libraries.




Comment at: llvm/lib/Target/ARM/ARMFastISel.cpp:1362
+  if (Ty->isDoubleTy() && (!Subtarget->hasVFP2Base() || !Subtarget->hasFP64()))
+ return false;
 

Some of the formatting here got a little messed up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60691



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

[PATCH] D60516: [LTO] Add plumbing to save stats during LTO on Darwin.

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

LGTM with a minor fix needed below.

Darwin still uses the old LTO API, which is why the lto Config based handling 
in the new LTO API (LTO.h/LTO.cpp) are not being used on this path.




Comment at: llvm/lib/LTO/LTOCodeGenerator.cpp:101
+"lto-stats-file",
+cl::desc("With PGO, include profile count in optimization remarks"),
+cl::Hidden);

Wrong description for this option


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60516



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

[PATCH] D60728: [clang] [test] Add a (xfailing) test for PR41027

[Kamil Rytarowski via Phabricator via cfe-commits]

krytarowski added a comment.

In D60728#1469794 , @hans wrote:

> In D60728#1468713 , @krytarowski 
> wrote:
>
> > In D60728#1468486 , @hans wrote:
> >
> > > What's the value in checking in this xfail'ed test without an actual fix 
> > > for the problem?
> >
> >
> > Raise awareness about the problem.
>
>
> I don't think that works. No one is reading through the test files of the 
> repository.


Well, I think we should treat it from the other side around.

Could we please revert it and backport revert to 8.0.1? The author(s) can 
improve the implementation and get this pr41027 test to check if a new version 
works.

Right now this blocks upgrades on NetBSD as a number of CPUs is affected (but 
not x86, so it was overlooked before 8.0 by others than @joerg).


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

https://reviews.llvm.org/D60728



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

[PATCH] D60516: [LTO] Add plumbing to save stats during LTO on Darwin.

[Steven Wu via Phabricator via cfe-commits]

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

LGTM with one additional small comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60516



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

[PATCH] D60516: [LTO] Add plumbing to save stats during LTO on Darwin.

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

forgot to save the inline comments.




Comment at: llvm/lib/LTO/LTOCodeGenerator.cpp:601
   // If statistics were requested, print them out after codegen.
-  if (llvm::AreStatisticsEnabled())
+  if (llvm::AreStatisticsEnabled() && !StatsFile)
 llvm::PrintStatistics();

You can simplify the logic a bit here.
```
if (StatsFile)
...
else if (llvm::AreStatisticsEnabled())
...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60516



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

[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 195601.
ahatanak marked an inline comment as done.
ahatanak added a comment.

Rename function.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60736

Files:
  include/clang/AST/DeclBase.h
  include/clang/Sema/Sema.h
  lib/Parse/ParseObjc.cpp
  lib/Sema/SemaDeclObjC.cpp
  lib/Sema/SemaExpr.cpp
  test/SemaObjC/warn-implicit-self-in-block.m
  test/SemaObjCXX/warn-implicit-self-in-block.mm

Index: test/SemaObjCXX/warn-implicit-self-in-block.mm
===
--- /dev/null
+++ test/SemaObjCXX/warn-implicit-self-in-block.mm
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -x objective-c++ -std=c++11 -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
+// rdar://11194874
+
+typedef void (^BlockTy)();
+
+void noescapeFunc(__attribute__((noescape)) BlockTy);
+void escapeFunc(BlockTy);
+
+@interface Root @end
+
+@interface I : Root
+{
+  int _bar;
+}
+@end
+
+@implementation I
+  - (void)foo{
+  ^{
+   _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+   }();
+  }
+
+  - (void)testNested{
+noescapeFunc(^{
+  (void)_bar;
+  escapeFunc(^{
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+noescapeFunc(^{
+  (void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+});
+(void)_bar; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+  });
+  (void)_bar;
+});
+  }
+
+  - (void)testLambdaInBlock{
+noescapeFunc(^{ [&](){ (void)_bar; }(); });
+escapeFunc(^{ [&](){ (void)_bar; }(); }); // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
+  }
+@end
Index: test/SemaObjC/warn-implicit-self-in-block.m
===
--- test/SemaObjC/warn-implicit-self-in-block.m
+++ /dev/null
@@ -1,18 +0,0 @@
-// RUN: %clang_cc1 -x objective-c -fobjc-arc -fblocks -Wimplicit-retain-self -verify %s
-// rdar://11194874
-
-@interface Root @end
-
-@interface I : Root
-{
-  int _bar;
-}
-@end
-
-@implementation I
-  - (void)foo{
-  ^{
-   _bar = 3; // expected-warning {{block implicitly retains 'self'; explicitly mention 'self' to indicate this is intended behavior}}
-   }();
-  }
-@end
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -2575,11 +2575,9 @@
 !Diags.isIgnored(diag::warn_arc_repeated_use_of_weak, Loc))
   getCurFunction()->recordUseOfWeak(Result);
   }
-  if (getLangOpts().ObjCAutoRefCount) {
-if (CurContext->isClosure())
-  Diag(Loc, diag::warn_implicitly_retains_self)
-<< FixItHint::CreateInsertion(Loc, "self->");
-  }
+  if (getLangOpts().ObjCAutoRefCount)
+if (const BlockDecl *BD = CurContext->getInnermostBlockDecl())
+  ImplicitlyRetainedSelfLocs.push_back({Loc, BD});
 
   return Result;
 }
Index: lib/Sema/SemaDeclObjC.cpp
===
--- lib/Sema/SemaDeclObjC.cpp
+++ lib/Sema/SemaDeclObjC.cpp
@@ -359,6 +359,7 @@
 /// ActOnStartOfObjCMethodDef - This routine sets up parameters; invisible
 /// and user declared, in the method definition's AST.
 void Sema::ActOnStartOfObjCMethodDef(Scope *FnBodyScope, Decl *D) {
+  ImplicitlyRetainedSelfLocs.clear();
   assert((getCurMethodDecl() == nullptr) && "Methodparsing confused");
   ObjCMethodDecl *MDecl = dyn_cast_or_null(D);
 
@@ -494,6 +495,35 @@
   }
 }
 
+void Sema::ActOnEndOfObjCMethodDef() {
+  llvm::DenseMap EscapeInfo;
+
+  auto IsOrNestedInEscapingBlock = [&](const BlockDecl *BD) {
+if (EscapeInfo.count(BD))
+  return EscapeInfo[BD];
+
+bool R = false;
+const BlockDecl *CurBD = BD;
+
+do {
+  R = R || !CurBD->doesNotEscape();
+  if (R)
+break;
+  CurBD = CurBD->getParent()->getInnermostBlockDecl();
+} while (CurBD);
+
+return EscapeInfo[BD] = R;
+  };
+
+  // If the location where 'self' is implicitly retained is inside a escaping
+  // block, emit a diagnostic.
+  for (const std::pair &P :
+   ImplicitlyRetainedSelfLocs)
+if (IsOrNestedInEscapingBlock(P.second))
+  Diag(P.first, diag::warn_implicitly_retains_self)
+  << FixItHint::CreateInsertion(P.first, "self->");
+}
+
 namespace {
 
 // Callback to only accept typo corrections that are Objective-C classes.
Index: lib/Parse/ParseObjc.cpp
===
--- lib/Parse/ParseObjc.cpp
+++ lib/Parse

[PATCH] D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/clang/ and include/

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

In D59168#1469186 , @jdenny wrote:

> Is there anything I can do to help this patch make progress?


I think it's ready to land. I was just waiting if anyone else wants to chime 
in, but it doesn't seem like.

I was also thinking about alternative names for the library path, specifically 
for headers we use `include/c++` but for libraries we'll now use 
`lib/clang/` which is not very consistent. I was considering 
`lib/c++/` instead but that wouldn't work for libomp and I didn't come 
up with any other better alternatives.


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

https://reviews.llvm.org/D59168



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

[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces 🔍

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479
+/// \endcode
+AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface,
+  internal::Matcher,

aaron.ballman wrote:
> stephanemoore wrote:
> > aaron.ballman wrote:
> > > stephanemoore wrote:
> > > > I am still uncertain about the naming.
> > > > 
> > > > `isSubclassOf` seemed too generic as that could apply to C++ classes.
> > > > `objcIsSubclassOf` seemed unconventional as a matcher name.
> > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed 
> > > > awkwardly lengthy.
> > > > Creating a new namespace `clang::ast_matchers::objc` seemed 
> > > > unprecedented.
> > > > 
> > > > I am happy to change the name if you think another name would be more 
> > > > appropriate.
> > > Does ObjC use the term "derived" by any chance? We already have 
> > > `isDerivedFrom`, so I'm wondering if we can use that to also match on an 
> > > `ObjCInterfaceDecl`?
> > Objective-C doesn't generally use the term "derived" (for example, see 
> > archive of [Programming With Objective-C > Defining 
> > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)).
> >  With that said, I don't think it's unreasonable or incorrect to use the 
> > term "derived" to describe inheritance in Objective-C. The behavior of this 
> > matcher is also consistent with the behavior of `isDerivedFrom`. In order 
> > to change `isDerivedFrom`, I would also need to update 
> > `isSameOrDerivedFrom`. That would probably be a good thing so that 
> > derivation matching feature set is consistent for C++ and Objective-C 
> > language variants.
> > 
> > Let me take a crack at merging this into `isDerivedFrom`.
> I agree that if we go with `isDerivedFrom`, you should update 
> `isSameOrDerivedFrom` at the same time.
`isSubclassOf` sounds right to me, and since ObjC and C++ class hierarchies 
can't mix, it _should_ be okay, right? They're analogous concepts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60543



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

[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces 🔍

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479
+/// \endcode
+AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface,
+  internal::Matcher,

jordan_rose wrote:
> aaron.ballman wrote:
> > stephanemoore wrote:
> > > aaron.ballman wrote:
> > > > stephanemoore wrote:
> > > > > I am still uncertain about the naming.
> > > > > 
> > > > > `isSubclassOf` seemed too generic as that could apply to C++ classes.
> > > > > `objcIsSubclassOf` seemed unconventional as a matcher name.
> > > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed 
> > > > > awkwardly lengthy.
> > > > > Creating a new namespace `clang::ast_matchers::objc` seemed 
> > > > > unprecedented.
> > > > > 
> > > > > I am happy to change the name if you think another name would be more 
> > > > > appropriate.
> > > > Does ObjC use the term "derived" by any chance? We already have 
> > > > `isDerivedFrom`, so I'm wondering if we can use that to also match on 
> > > > an `ObjCInterfaceDecl`?
> > > Objective-C doesn't generally use the term "derived" (for example, see 
> > > archive of [Programming With Objective-C > Defining 
> > > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)).
> > >  With that said, I don't think it's unreasonable or incorrect to use the 
> > > term "derived" to describe inheritance in Objective-C. The behavior of 
> > > this matcher is also consistent with the behavior of `isDerivedFrom`. In 
> > > order to change `isDerivedFrom`, I would also need to update 
> > > `isSameOrDerivedFrom`. That would probably be a good thing so that 
> > > derivation matching feature set is consistent for C++ and Objective-C 
> > > language variants.
> > > 
> > > Let me take a crack at merging this into `isDerivedFrom`.
> > I agree that if we go with `isDerivedFrom`, you should update 
> > `isSameOrDerivedFrom` at the same time.
> `isSubclassOf` sounds right to me, and since ObjC and C++ class hierarchies 
> can't mix, it _should_ be okay, right? They're analogous concepts.
> isSubclassOf sounds right to me, and since ObjC and C++ class hierarchies 
> can't mix, it _should_ be okay, right? They're analogous concepts.

In the AST matchers, we try to overload the matchers that have similar 
behavior. My concern is that a user will reach for `isSubclassOf()` when they 
really mean `isDerivedFrom()` or vice versa, and only through annoying error 
messages learns about their mistake. Given that we already have 
`isDerivedFrom()` and renaming it would break code, I was trying to determine 
whether using that name for both C++ derivation and ObjC derivation would be 
acceptable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60543



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

[PATCH] D59371: [LibTooling] Add Stencil library for format-string style codegen.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 195602.
ymandel marked 9 inline comments as done.
ymandel added a comment.

Responded to comments, including changing call operator to return 
Expected instead of string.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59371

Files:
  clang/include/clang/Tooling/Refactoring/Stencil.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Stencil.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/StencilTest.cpp

Index: clang/unittests/Tooling/StencilTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/StencilTest.cpp
@@ -0,0 +1,223 @@
+//===- unittest/Tooling/StencilTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Stencil.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/FixIt.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace tooling;
+using namespace ast_matchers;
+
+namespace {
+using ::testing::AllOf;
+using ::testing::Eq;
+using ::testing::HasSubstr;
+using MatchResult = MatchFinder::MatchResult;
+using tooling::stencil::node;
+using tooling::stencil::sNode;
+using tooling::stencil::text;
+
+// In tests, we can't directly match on llvm::Expected since its accessors
+// mutate the object. So, we collapse it to an Optional.
+static llvm::Optional toOptional(llvm::Expected V) {
+  if (V)
+return *V;
+  ADD_FAILURE() << "Losing error in conversion to IsSomething: "
+<< llvm::toString(V.takeError());
+  return llvm::None;
+}
+
+// A very simple matcher for llvm::Optional values.
+MATCHER_P(IsSomething, ValueMatcher, "") {
+  if (!arg)
+return false;
+  return ::testing::ExplainMatchResult(ValueMatcher, *arg, result_listener);
+}
+
+// Create a valid translation-unit from a statement.
+static std::string wrapSnippet(llvm::Twine StatementCode) {
+  return ("auto stencil_test_snippet = []{" + StatementCode + "};").str();
+}
+
+static DeclarationMatcher wrapMatcher(const StatementMatcher &Matcher) {
+  return varDecl(hasName("stencil_test_snippet"),
+ hasDescendant(compoundStmt(hasAnySubstatement(Matcher;
+}
+
+struct TestMatch {
+  // The AST unit from which `result` is built. We bundle it because it backs
+  // the result. Users are not expected to access it.
+  std::unique_ptr AstUnit;
+  // The result to use in the test. References `ast_unit`.
+  MatchResult Result;
+};
+
+// Matches `Matcher` against the statement `StatementCode` and returns the
+// result. Handles putting the statement inside a function and modifying the
+// matcher correspondingly. `Matcher` should match `StatementCode` exactly --
+// that is, produce exactly one match.
+static llvm::Optional matchStmt(llvm::Twine StatementCode,
+   StatementMatcher Matcher) {
+  auto AstUnit = buildASTFromCode(wrapSnippet(StatementCode));
+  if (AstUnit == nullptr) {
+ADD_FAILURE() << "AST construction failed";
+return llvm::None;
+  }
+  ASTContext &Context = AstUnit->getASTContext();
+  auto Matches = ast_matchers::match(wrapMatcher(Matcher), Context);
+  // We expect a single, exact match for the statement.
+  if (Matches.size() != 1) {
+ADD_FAILURE() << "Wrong number of matches: " << Matches.size();
+return llvm::None;
+  }
+  return TestMatch{std::move(AstUnit), MatchResult(Matches[0], &Context)};
+}
+
+class StencilTest : public ::testing::Test {
+protected:
+  // Verifies that the given stencil fails when evaluated on a valid match
+  // result. Binds a statement to "stmt", a (non-member) ctor-initializer to
+  // "init", an expression to "expr" and a (nameless) declaration to "decl".
+  void testError(const Stencil &Stencil,
+ ::testing::Matcher Matcher) {
+const std::string Snippet = R"cc(
+  struct A {};
+  class F : public A {
+   public:
+F(int) {}
+  };
+  F(1);
+)cc";
+auto StmtMatch = matchStmt(
+Snippet,
+stmt(hasDescendant(
+ cxxConstructExpr(
+ hasDeclaration(decl(hasDescendant(cxxCtorInitializer(
+   isBaseInitializer())
+   .bind("init")))
+.bind("decl")))
+ .bind("expr")))
+.bind("stmt"));
+ASSERT_TRUE(StmtMatch);
+if (auto ResultOrErr = Stencil.eval(StmtMatch->Result)) {
+  A

[PATCH] D60736: [Sema][ObjC] Don't warn about a block implicitly retaining self if the block is marked noescape

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.



Comment at: lib/Parse/ParseObjc.cpp:3696-3699
+
+  Actions.ActOnEndOfObjCMethodDef();
+
   // Clean up the remaining EOF token.

Any reason not to do this check in `ActOnFinishFunctionBody` (which is called 
by ParseFunctionStatementBody)? Seems like there are some similar diagnostics 
implemented there.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60736



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

[PATCH] D59371: [LibTooling] Add Stencil library for format-string style codegen.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:127
+  // Allow Stencils to operate as std::function, for compatibility with
+  // Transformer's TextGenerator. Calls `eval()` and asserts on failure.
+  std::string operator()(const ast_matchers::MatchFinder::MatchResult &) const;

sbenza wrote:
> ymandel wrote:
> > sbenza wrote:
> > > "asserts" as it only fails in debug mode?
> > I thought `assert()` also fails in normal mode, based on my reading of the 
> > llvm docs, but it's not explicit about this: 
> > http://llvm.org/docs/ProgrammersManual.html#programmatic-errors
> > 
> > FWIW, I'm on the fence whether `eval()` should actually have the same 
> > signature and similarly just crash the program on errors. Its not clear 
> > there's any value to propogating the errors up the stack via `Expected`.
> `assert` follows `NDEBUG`, which is not explicitly related to `-Ox`, but they 
> usually go together.
> 
> Don't make it crash, please.
> Otherwise it would be hard to have a refactoring service or alike.
Right, you've convinced me. For some background: TextGenerator (in Transformer) 
originally return an Expected, but Ilya suggested that I simplify 
to just a string and use assertions.  The logic being that these are 
compiler-style tools. However, I agree that this simply doesn't work for 
running these in servers, which is a very real concern. It's also trivial to 
imagine a user mistyping a stencil (say, in web UI) which results in a failure 
in stencil eval. We want to return a nice error to the user, not crash the 
server.

So, I've adjusted the signature. I will (separately) change the definition of 
TextGenerator in Transformer.



Comment at: clang/include/clang/Tooling/Refactoring/Stencil.h:101
+S.Parts.reserve(sizeof...(Parts));
+auto Unused = {(S.push(std::forward(Parts)), true)...};
+(void)Unused;

sbenza wrote:
> We could simplify this code if you change `void push(T)` to instead 
> `StencilPart wrap(T)`.
> Then this could be:
> 
> 
> ```
> Stencil S;
> S.Parts = {wrap(std::forward(Parts))...};
> return S;
> ```
Much better. I believe the current design came about when StencilPart was move 
only. Thanks!



Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:78
+
+template <> bool isEqualData(const RawTextData &A, const RawTextData &B) {
+  return A.Text == B.Text;

sbenza wrote:
> Any particular reason to use function template specialization instead of 
> function overloading?
> The former is rare and thus harder to reason about.
> 
> (same for `evalData` below)
Stylistic -- it forces you to explicitly declare the "overload set" so to 
speak.  However, i'm not particularly attached -- if you think this a bad idea 
given its rarity, I'm happy to use overload sets. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59371



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

[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces 🔍

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479
+/// \endcode
+AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface,
+  internal::Matcher,

aaron.ballman wrote:
> jordan_rose wrote:
> > aaron.ballman wrote:
> > > stephanemoore wrote:
> > > > aaron.ballman wrote:
> > > > > stephanemoore wrote:
> > > > > > I am still uncertain about the naming.
> > > > > > 
> > > > > > `isSubclassOf` seemed too generic as that could apply to C++ 
> > > > > > classes.
> > > > > > `objcIsSubclassOf` seemed unconventional as a matcher name.
> > > > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed 
> > > > > > awkwardly lengthy.
> > > > > > Creating a new namespace `clang::ast_matchers::objc` seemed 
> > > > > > unprecedented.
> > > > > > 
> > > > > > I am happy to change the name if you think another name would be 
> > > > > > more appropriate.
> > > > > Does ObjC use the term "derived" by any chance? We already have 
> > > > > `isDerivedFrom`, so I'm wondering if we can use that to also match on 
> > > > > an `ObjCInterfaceDecl`?
> > > > Objective-C doesn't generally use the term "derived" (for example, see 
> > > > archive of [Programming With Objective-C > Defining 
> > > > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)).
> > > >  With that said, I don't think it's unreasonable or incorrect to use 
> > > > the term "derived" to describe inheritance in Objective-C. The behavior 
> > > > of this matcher is also consistent with the behavior of 
> > > > `isDerivedFrom`. In order to change `isDerivedFrom`, I would also need 
> > > > to update `isSameOrDerivedFrom`. That would probably be a good thing so 
> > > > that derivation matching feature set is consistent for C++ and 
> > > > Objective-C language variants.
> > > > 
> > > > Let me take a crack at merging this into `isDerivedFrom`.
> > > I agree that if we go with `isDerivedFrom`, you should update 
> > > `isSameOrDerivedFrom` at the same time.
> > `isSubclassOf` sounds right to me, and since ObjC and C++ class hierarchies 
> > can't mix, it _should_ be okay, right? They're analogous concepts.
> > isSubclassOf sounds right to me, and since ObjC and C++ class hierarchies 
> > can't mix, it _should_ be okay, right? They're analogous concepts.
> 
> In the AST matchers, we try to overload the matchers that have similar 
> behavior. My concern is that a user will reach for `isSubclassOf()` when they 
> really mean `isDerivedFrom()` or vice versa, and only through annoying error 
> messages learns about their mistake. Given that we already have 
> `isDerivedFrom()` and renaming it would break code, I was trying to determine 
> whether using that name for both C++ derivation and ObjC derivation would be 
> acceptable.
Ah, I see what you mean. Yes, I guess it's more important to be consistent than 
to perfectly match the terminology. You will certainly confuse an ObjC-only 
developer at first by using "non-standard" terminology, but any developer has 
to learn a certain amount of compiler-isms anyway using AST matchers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60543



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

[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces 🔍

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479
+/// \endcode
+AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface,
+  internal::Matcher,

jordan_rose wrote:
> aaron.ballman wrote:
> > jordan_rose wrote:
> > > aaron.ballman wrote:
> > > > stephanemoore wrote:
> > > > > aaron.ballman wrote:
> > > > > > stephanemoore wrote:
> > > > > > > I am still uncertain about the naming.
> > > > > > > 
> > > > > > > `isSubclassOf` seemed too generic as that could apply to C++ 
> > > > > > > classes.
> > > > > > > `objcIsSubclassOf` seemed unconventional as a matcher name.
> > > > > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed 
> > > > > > > awkwardly lengthy.
> > > > > > > Creating a new namespace `clang::ast_matchers::objc` seemed 
> > > > > > > unprecedented.
> > > > > > > 
> > > > > > > I am happy to change the name if you think another name would be 
> > > > > > > more appropriate.
> > > > > > Does ObjC use the term "derived" by any chance? We already have 
> > > > > > `isDerivedFrom`, so I'm wondering if we can use that to also match 
> > > > > > on an `ObjCInterfaceDecl`?
> > > > > Objective-C doesn't generally use the term "derived" (for example, 
> > > > > see archive of [Programming With Objective-C > Defining 
> > > > > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)).
> > > > >  With that said, I don't think it's unreasonable or incorrect to use 
> > > > > the term "derived" to describe inheritance in Objective-C. The 
> > > > > behavior of this matcher is also consistent with the behavior of 
> > > > > `isDerivedFrom`. In order to change `isDerivedFrom`, I would also 
> > > > > need to update `isSameOrDerivedFrom`. That would probably be a good 
> > > > > thing so that derivation matching feature set is consistent for C++ 
> > > > > and Objective-C language variants.
> > > > > 
> > > > > Let me take a crack at merging this into `isDerivedFrom`.
> > > > I agree that if we go with `isDerivedFrom`, you should update 
> > > > `isSameOrDerivedFrom` at the same time.
> > > `isSubclassOf` sounds right to me, and since ObjC and C++ class 
> > > hierarchies can't mix, it _should_ be okay, right? They're analogous 
> > > concepts.
> > > isSubclassOf sounds right to me, and since ObjC and C++ class hierarchies 
> > > can't mix, it _should_ be okay, right? They're analogous concepts.
> > 
> > In the AST matchers, we try to overload the matchers that have similar 
> > behavior. My concern is that a user will reach for `isSubclassOf()` when 
> > they really mean `isDerivedFrom()` or vice versa, and only through annoying 
> > error messages learns about their mistake. Given that we already have 
> > `isDerivedFrom()` and renaming it would break code, I was trying to 
> > determine whether using that name for both C++ derivation and ObjC 
> > derivation would be acceptable.
> Ah, I see what you mean. Yes, I guess it's more important to be consistent 
> than to perfectly match the terminology. You will certainly confuse an 
> ObjC-only developer at first by using "non-standard" terminology, but any 
> developer has to learn a certain amount of compiler-isms anyway using AST 
> matchers.
Okay, so it sounds like it wouldn't be hugely problematic to go with 
`isDerivedFrom()`, just that it may sound a bit confusing at first to an ObjC 
developer. Are there some words you think we should add to the documentation to 
help an ObjC developer searching for this functionality?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60543



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

[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces 🔍

[Jordan Rose via Phabricator via cfe-commits]

jordan_rose added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479
+/// \endcode
+AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface,
+  internal::Matcher,

aaron.ballman wrote:
> jordan_rose wrote:
> > aaron.ballman wrote:
> > > jordan_rose wrote:
> > > > aaron.ballman wrote:
> > > > > stephanemoore wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > stephanemoore wrote:
> > > > > > > > I am still uncertain about the naming.
> > > > > > > > 
> > > > > > > > `isSubclassOf` seemed too generic as that could apply to C++ 
> > > > > > > > classes.
> > > > > > > > `objcIsSubclassOf` seemed unconventional as a matcher name.
> > > > > > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` seemed 
> > > > > > > > awkwardly lengthy.
> > > > > > > > Creating a new namespace `clang::ast_matchers::objc` seemed 
> > > > > > > > unprecedented.
> > > > > > > > 
> > > > > > > > I am happy to change the name if you think another name would 
> > > > > > > > be more appropriate.
> > > > > > > Does ObjC use the term "derived" by any chance? We already have 
> > > > > > > `isDerivedFrom`, so I'm wondering if we can use that to also 
> > > > > > > match on an `ObjCInterfaceDecl`?
> > > > > > Objective-C doesn't generally use the term "derived" (for example, 
> > > > > > see archive of [Programming With Objective-C > Defining 
> > > > > > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)).
> > > > > >  With that said, I don't think it's unreasonable or incorrect to 
> > > > > > use the term "derived" to describe inheritance in Objective-C. The 
> > > > > > behavior of this matcher is also consistent with the behavior of 
> > > > > > `isDerivedFrom`. In order to change `isDerivedFrom`, I would also 
> > > > > > need to update `isSameOrDerivedFrom`. That would probably be a good 
> > > > > > thing so that derivation matching feature set is consistent for C++ 
> > > > > > and Objective-C language variants.
> > > > > > 
> > > > > > Let me take a crack at merging this into `isDerivedFrom`.
> > > > > I agree that if we go with `isDerivedFrom`, you should update 
> > > > > `isSameOrDerivedFrom` at the same time.
> > > > `isSubclassOf` sounds right to me, and since ObjC and C++ class 
> > > > hierarchies can't mix, it _should_ be okay, right? They're analogous 
> > > > concepts.
> > > > isSubclassOf sounds right to me, and since ObjC and C++ class 
> > > > hierarchies can't mix, it _should_ be okay, right? They're analogous 
> > > > concepts.
> > > 
> > > In the AST matchers, we try to overload the matchers that have similar 
> > > behavior. My concern is that a user will reach for `isSubclassOf()` when 
> > > they really mean `isDerivedFrom()` or vice versa, and only through 
> > > annoying error messages learns about their mistake. Given that we already 
> > > have `isDerivedFrom()` and renaming it would break code, I was trying to 
> > > determine whether using that name for both C++ derivation and ObjC 
> > > derivation would be acceptable.
> > Ah, I see what you mean. Yes, I guess it's more important to be consistent 
> > than to perfectly match the terminology. You will certainly confuse an 
> > ObjC-only developer at first by using "non-standard" terminology, but any 
> > developer has to learn a certain amount of compiler-isms anyway using AST 
> > matchers.
> Okay, so it sounds like it wouldn't be hugely problematic to go with 
> `isDerivedFrom()`, just that it may sound a bit confusing at first to an ObjC 
> developer. Are there some words you think we should add to the documentation 
> to help an ObjC developer searching for this functionality?
I think just including "subclass" is sufficient. For example, the current doc 
comment is

```
/// Matches C++ classes that are directly or indirectly derived from
/// a class matching \c Base.
```

and it could be changed to something like

```
/// Matches C++ classes that are directly or indirectly derived from
/// a class matching \c Base, or Objective-C classes that directly or
/// indirectly subclass a class matching \c Base.
```

A little clunky, but you get what I mean.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60543



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

[PATCH] D60543: [clang] Add matcher for subclasses of Objective-C interfaces 🔍

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:1479
+/// \endcode
+AST_MATCHER_P(ObjCInterfaceDecl, isSubclassOfInterface,
+  internal::Matcher,

jordan_rose wrote:
> aaron.ballman wrote:
> > jordan_rose wrote:
> > > aaron.ballman wrote:
> > > > jordan_rose wrote:
> > > > > aaron.ballman wrote:
> > > > > > stephanemoore wrote:
> > > > > > > aaron.ballman wrote:
> > > > > > > > stephanemoore wrote:
> > > > > > > > > I am still uncertain about the naming.
> > > > > > > > > 
> > > > > > > > > `isSubclassOf` seemed too generic as that could apply to C++ 
> > > > > > > > > classes.
> > > > > > > > > `objcIsSubclassOf` seemed unconventional as a matcher name.
> > > > > > > > > `isSubclassOfObjCInterface` and `isSubclassOfObjCClass` 
> > > > > > > > > seemed awkwardly lengthy.
> > > > > > > > > Creating a new namespace `clang::ast_matchers::objc` seemed 
> > > > > > > > > unprecedented.
> > > > > > > > > 
> > > > > > > > > I am happy to change the name if you think another name would 
> > > > > > > > > be more appropriate.
> > > > > > > > Does ObjC use the term "derived" by any chance? We already have 
> > > > > > > > `isDerivedFrom`, so I'm wondering if we can use that to also 
> > > > > > > > match on an `ObjCInterfaceDecl`?
> > > > > > > Objective-C doesn't generally use the term "derived" (for 
> > > > > > > example, see archive of [Programming With Objective-C > Defining 
> > > > > > > Classes](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/DefiningClasses/DefiningClasses.html#//apple_ref/doc/uid/TP40011210-CH3-SW1)).
> > > > > > >  With that said, I don't think it's unreasonable or incorrect to 
> > > > > > > use the term "derived" to describe inheritance in Objective-C. 
> > > > > > > The behavior of this matcher is also consistent with the behavior 
> > > > > > > of `isDerivedFrom`. In order to change `isDerivedFrom`, I would 
> > > > > > > also need to update `isSameOrDerivedFrom`. That would probably be 
> > > > > > > a good thing so that derivation matching feature set is 
> > > > > > > consistent for C++ and Objective-C language variants.
> > > > > > > 
> > > > > > > Let me take a crack at merging this into `isDerivedFrom`.
> > > > > > I agree that if we go with `isDerivedFrom`, you should update 
> > > > > > `isSameOrDerivedFrom` at the same time.
> > > > > `isSubclassOf` sounds right to me, and since ObjC and C++ class 
> > > > > hierarchies can't mix, it _should_ be okay, right? They're analogous 
> > > > > concepts.
> > > > > isSubclassOf sounds right to me, and since ObjC and C++ class 
> > > > > hierarchies can't mix, it _should_ be okay, right? They're analogous 
> > > > > concepts.
> > > > 
> > > > In the AST matchers, we try to overload the matchers that have similar 
> > > > behavior. My concern is that a user will reach for `isSubclassOf()` 
> > > > when they really mean `isDerivedFrom()` or vice versa, and only through 
> > > > annoying error messages learns about their mistake. Given that we 
> > > > already have `isDerivedFrom()` and renaming it would break code, I was 
> > > > trying to determine whether using that name for both C++ derivation and 
> > > > ObjC derivation would be acceptable.
> > > Ah, I see what you mean. Yes, I guess it's more important to be 
> > > consistent than to perfectly match the terminology. You will certainly 
> > > confuse an ObjC-only developer at first by using "non-standard" 
> > > terminology, but any developer has to learn a certain amount of 
> > > compiler-isms anyway using AST matchers.
> > Okay, so it sounds like it wouldn't be hugely problematic to go with 
> > `isDerivedFrom()`, just that it may sound a bit confusing at first to an 
> > ObjC developer. Are there some words you think we should add to the 
> > documentation to help an ObjC developer searching for this functionality?
> I think just including "subclass" is sufficient. For example, the current doc 
> comment is
> 
> ```
> /// Matches C++ classes that are directly or indirectly derived from
> /// a class matching \c Base.
> ```
> 
> and it could be changed to something like
> 
> ```
> /// Matches C++ classes that are directly or indirectly derived from
> /// a class matching \c Base, or Objective-C classes that directly or
> /// indirectly subclass a class matching \c Base.
> ```
> 
> A little clunky, but you get what I mean.
> 
Fantastic, that makes sense to me. Thank you for weighing in!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60543



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

[clang-tools-extra] r358605 - [clangd] Recognize "don't include me directly" pattern, and suppress include insertion.

[Sam McCall via cfe-commits]

Author: sammccall
Date: Wed Apr 17 11:33:07 2019
New Revision: 358605

URL: http://llvm.org/viewvc/llvm-project?rev=358605&view=rev
Log:
[clangd] Recognize "don't include me directly" pattern, and suppress include 
insertion.

Summary:
Typically used with umbrella headers, e.g. GTK:

 #if !defined (__GTK_H_INSIDE__) && !defined (GTK_COMPILATION)
 #error "Only  can be included directly."
 #endif

Heuristic is fairly conservative, a quick code search over github showed
a fair number of hits and few/no false positives. (Not all were umbrella
headers, but I'd be happy avoiding include insertion for all of them).

We may want to relax the heuristic later to catch more cases.

Reviewers: ioeric

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

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

Modified:
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.h
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp

Modified: clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp?rev=358605&r1=358604&r2=358605&view=diff
==
--- clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp Wed Apr 17 
11:33:07 2019
@@ -128,48 +128,6 @@ bool shouldCollectIncludePath(index::Sym
   }
 }
 
-bool isSelfContainedHeader(FileID FID, const SourceManager &SM,
-   const Preprocessor &PP) {
-  const FileEntry *FE = SM.getFileEntryForID(FID);
-  if (!FE)
-return false;
-  return PP.getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE);
-}
-
-/// Gets a canonical include (URI of the header or  or "header") for
-/// header of \p FID (which should usually be the *expansion* file).
-/// Returns None if includes should not be inserted for this file.
-llvm::Optional
-getIncludeHeader(llvm::StringRef QName, const SourceManager &SM,
- const Preprocessor &PP, FileID FID,
- const SymbolCollector::Options &Opts) {
-  const FileEntry *FE = SM.getFileEntryForID(FID);
-  if (!FE || FE->getName().empty())
-return llvm::None;
-  llvm::StringRef Filename = FE->getName();
-  // If a file is mapped by canonical headers, use that mapping, regardless
-  // of whether it's an otherwise-good header (header guards etc).
-  if (Opts.Includes) {
-llvm::StringRef Canonical = Opts.Includes->mapHeader(Filename, QName);
-// If we had a mapping, always use it.
-if (Canonical.startswith("<") || Canonical.startswith("\""))
-  return Canonical.str();
-if (Canonical != Filename)
-  return toURI(SM, Canonical, Opts);
-  }
-  if (!isSelfContainedHeader(FID, SM, PP)) {
-// A .inc or .def file is often included into a real header to define
-// symbols (e.g. LLVM tablegen files).
-if (Filename.endswith(".inc") || Filename.endswith(".def"))
-  return getIncludeHeader(QName, SM, PP,
-  SM.getFileID(SM.getIncludeLoc(FID)), Opts);
-// Conservatively refuse to insert #includes to files without guards.
-return llvm::None;
-  }
-  // Standard case: just insert the file itself.
-  return toURI(SM, Filename, Opts);
-}
-
 // Return the symbol range of the token at \p TokLoc.
 std::pair
 getTokenRange(SourceLocation TokLoc, const SourceManager &SM,
@@ -452,9 +410,8 @@ bool SymbolCollector::handleMacroOccuren
 
   std::string Include;
   if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) {
-if (auto Header =
-getIncludeHeader(Name->getName(), SM, *PP,
- SM.getDecomposedExpansionLoc(DefLoc).first, Opts))
+if (auto Header = getIncludeHeader(
+Name->getName(), SM.getDecomposedExpansionLoc(DefLoc).first))
   Include = std::move(*Header);
   }
   S.Signature = Signature;
@@ -533,6 +490,7 @@ void SymbolCollector::finish() {
   ReferencedMacros.clear();
   DeclRefs.clear();
   FilesToIndexCache.clear();
+  HeaderIsSelfContainedCache.clear();
 }
 
 const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
@@ -602,8 +560,7 @@ const Symbol *SymbolCollector::addDeclar
 // Use the expansion location to get the #include header since this is
 // where the symbol is exposed.
 if (auto Header = getIncludeHeader(
-QName, SM, *PP,
-SM.getDecomposedExpansionLoc(ND.getLocation()).first, Opts))
+QName, SM.getDecomposedExpansionLoc(ND.getLocation()).first))
   Include = std::move(*Header);
   }
   if (!Include.empty())
@@ -639,5 +596,59 @@ void SymbolCollector::addDefinition(cons
   Symbols.insert(S);
 }
 
+/// Gets a canonical include (URI of the header or  or "header") for
+/// header of \p FID (which should usually be the *expansion* file)

[PATCH] D60815: [clangd] Recognize "don't include me directly" pattern, and suppress include insertion.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE358605: [clangd] Recognize "don't include me 
directly" pattern, and suppress include… (authored by sammccall, committed 
by ).

Changed prior to commit:
  https://reviews.llvm.org/D60815?vs=195562&id=195607#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60815

Files:
  clangd/index/SymbolCollector.cpp
  clangd/index/SymbolCollector.h
  unittests/clangd/SymbolCollectorTests.cpp

Index: clangd/index/SymbolCollector.cpp
===
--- clangd/index/SymbolCollector.cpp
+++ clangd/index/SymbolCollector.cpp
@@ -128,48 +128,6 @@
   }
 }
 
-bool isSelfContainedHeader(FileID FID, const SourceManager &SM,
-   const Preprocessor &PP) {
-  const FileEntry *FE = SM.getFileEntryForID(FID);
-  if (!FE)
-return false;
-  return PP.getHeaderSearchInfo().isFileMultipleIncludeGuarded(FE);
-}
-
-/// Gets a canonical include (URI of the header or  or "header") for
-/// header of \p FID (which should usually be the *expansion* file).
-/// Returns None if includes should not be inserted for this file.
-llvm::Optional
-getIncludeHeader(llvm::StringRef QName, const SourceManager &SM,
- const Preprocessor &PP, FileID FID,
- const SymbolCollector::Options &Opts) {
-  const FileEntry *FE = SM.getFileEntryForID(FID);
-  if (!FE || FE->getName().empty())
-return llvm::None;
-  llvm::StringRef Filename = FE->getName();
-  // If a file is mapped by canonical headers, use that mapping, regardless
-  // of whether it's an otherwise-good header (header guards etc).
-  if (Opts.Includes) {
-llvm::StringRef Canonical = Opts.Includes->mapHeader(Filename, QName);
-// If we had a mapping, always use it.
-if (Canonical.startswith("<") || Canonical.startswith("\""))
-  return Canonical.str();
-if (Canonical != Filename)
-  return toURI(SM, Canonical, Opts);
-  }
-  if (!isSelfContainedHeader(FID, SM, PP)) {
-// A .inc or .def file is often included into a real header to define
-// symbols (e.g. LLVM tablegen files).
-if (Filename.endswith(".inc") || Filename.endswith(".def"))
-  return getIncludeHeader(QName, SM, PP,
-  SM.getFileID(SM.getIncludeLoc(FID)), Opts);
-// Conservatively refuse to insert #includes to files without guards.
-return llvm::None;
-  }
-  // Standard case: just insert the file itself.
-  return toURI(SM, Filename, Opts);
-}
-
 // Return the symbol range of the token at \p TokLoc.
 std::pair
 getTokenRange(SourceLocation TokLoc, const SourceManager &SM,
@@ -452,9 +410,8 @@
 
   std::string Include;
   if (Opts.CollectIncludePath && shouldCollectIncludePath(S.SymInfo.Kind)) {
-if (auto Header =
-getIncludeHeader(Name->getName(), SM, *PP,
- SM.getDecomposedExpansionLoc(DefLoc).first, Opts))
+if (auto Header = getIncludeHeader(
+Name->getName(), SM.getDecomposedExpansionLoc(DefLoc).first))
   Include = std::move(*Header);
   }
   S.Signature = Signature;
@@ -533,6 +490,7 @@
   ReferencedMacros.clear();
   DeclRefs.clear();
   FilesToIndexCache.clear();
+  HeaderIsSelfContainedCache.clear();
 }
 
 const Symbol *SymbolCollector::addDeclaration(const NamedDecl &ND, SymbolID ID,
@@ -602,8 +560,7 @@
 // Use the expansion location to get the #include header since this is
 // where the symbol is exposed.
 if (auto Header = getIncludeHeader(
-QName, SM, *PP,
-SM.getDecomposedExpansionLoc(ND.getLocation()).first, Opts))
+QName, SM.getDecomposedExpansionLoc(ND.getLocation()).first))
   Include = std::move(*Header);
   }
   if (!Include.empty())
@@ -639,5 +596,59 @@
   Symbols.insert(S);
 }
 
+/// Gets a canonical include (URI of the header or  or "header") for
+/// header of \p FID (which should usually be the *expansion* file).
+/// Returns None if includes should not be inserted for this file.
+llvm::Optional
+SymbolCollector::getIncludeHeader(llvm::StringRef QName, FileID FID) {
+  const SourceManager &SM = ASTCtx->getSourceManager();
+  const FileEntry *FE = SM.getFileEntryForID(FID);
+  if (!FE || FE->getName().empty())
+return llvm::None;
+  llvm::StringRef Filename = FE->getName();
+  // If a file is mapped by canonical headers, use that mapping, regardless
+  // of whether it's an otherwise-good header (header guards etc).
+  if (Opts.Includes) {
+llvm::StringRef Canonical = Opts.Includes->mapHeader(Filename, QName);
+// If we had a mapping, always use it.
+if (Canonical.startswith("<") || Canonical.startswith("\""))
+  return Canonical.str();
+if (Canonical != Filename)
+  return toURI(SM, Canonical, Opts);
+  }
+  if (!isSelfContainedHeader(FID)) {
+// A .inc or .def file is often included into a real header to 

[PATCH] D59371: [LibTooling] Add Stencil library for format-string style codegen.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 195606.
ymandel added a comment.

Convert template specialization to overload sets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59371

Files:
  clang/include/clang/Tooling/Refactoring/Stencil.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Stencil.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/StencilTest.cpp

Index: clang/unittests/Tooling/StencilTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/StencilTest.cpp
@@ -0,0 +1,223 @@
+//===- unittest/Tooling/StencilTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Stencil.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/FixIt.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace tooling;
+using namespace ast_matchers;
+
+namespace {
+using ::testing::AllOf;
+using ::testing::Eq;
+using ::testing::HasSubstr;
+using MatchResult = MatchFinder::MatchResult;
+using tooling::stencil::node;
+using tooling::stencil::sNode;
+using tooling::stencil::text;
+
+// In tests, we can't directly match on llvm::Expected since its accessors
+// mutate the object. So, we collapse it to an Optional.
+static llvm::Optional toOptional(llvm::Expected V) {
+  if (V)
+return *V;
+  ADD_FAILURE() << "Losing error in conversion to IsSomething: "
+<< llvm::toString(V.takeError());
+  return llvm::None;
+}
+
+// A very simple matcher for llvm::Optional values.
+MATCHER_P(IsSomething, ValueMatcher, "") {
+  if (!arg)
+return false;
+  return ::testing::ExplainMatchResult(ValueMatcher, *arg, result_listener);
+}
+
+// Create a valid translation-unit from a statement.
+static std::string wrapSnippet(llvm::Twine StatementCode) {
+  return ("auto stencil_test_snippet = []{" + StatementCode + "};").str();
+}
+
+static DeclarationMatcher wrapMatcher(const StatementMatcher &Matcher) {
+  return varDecl(hasName("stencil_test_snippet"),
+ hasDescendant(compoundStmt(hasAnySubstatement(Matcher;
+}
+
+struct TestMatch {
+  // The AST unit from which `result` is built. We bundle it because it backs
+  // the result. Users are not expected to access it.
+  std::unique_ptr AstUnit;
+  // The result to use in the test. References `ast_unit`.
+  MatchResult Result;
+};
+
+// Matches `Matcher` against the statement `StatementCode` and returns the
+// result. Handles putting the statement inside a function and modifying the
+// matcher correspondingly. `Matcher` should match `StatementCode` exactly --
+// that is, produce exactly one match.
+static llvm::Optional matchStmt(llvm::Twine StatementCode,
+   StatementMatcher Matcher) {
+  auto AstUnit = buildASTFromCode(wrapSnippet(StatementCode));
+  if (AstUnit == nullptr) {
+ADD_FAILURE() << "AST construction failed";
+return llvm::None;
+  }
+  ASTContext &Context = AstUnit->getASTContext();
+  auto Matches = ast_matchers::match(wrapMatcher(Matcher), Context);
+  // We expect a single, exact match for the statement.
+  if (Matches.size() != 1) {
+ADD_FAILURE() << "Wrong number of matches: " << Matches.size();
+return llvm::None;
+  }
+  return TestMatch{std::move(AstUnit), MatchResult(Matches[0], &Context)};
+}
+
+class StencilTest : public ::testing::Test {
+protected:
+  // Verifies that the given stencil fails when evaluated on a valid match
+  // result. Binds a statement to "stmt", a (non-member) ctor-initializer to
+  // "init", an expression to "expr" and a (nameless) declaration to "decl".
+  void testError(const Stencil &Stencil,
+ ::testing::Matcher Matcher) {
+const std::string Snippet = R"cc(
+  struct A {};
+  class F : public A {
+   public:
+F(int) {}
+  };
+  F(1);
+)cc";
+auto StmtMatch = matchStmt(
+Snippet,
+stmt(hasDescendant(
+ cxxConstructExpr(
+ hasDeclaration(decl(hasDescendant(cxxCtorInitializer(
+   isBaseInitializer())
+   .bind("init")))
+.bind("decl")))
+ .bind("expr")))
+.bind("stmt"));
+ASSERT_TRUE(StmtMatch);
+if (auto ResultOrErr = Stencil.eval(StmtMatch->Result)) {
+  ADD_FAILURE() << "Expected failure but succeeded: " << *ResultOrErr;
+} else {
+

[PATCH] D59371: [LibTooling] Add Stencil library for format-string style codegen.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked an inline comment as done.
ymandel added inline comments.



Comment at: clang/lib/Tooling/Refactoring/Stencil.cpp:78
+
+template <> bool isEqualData(const RawTextData &A, const RawTextData &B) {
+  return A.Text == B.Text;

ymandel wrote:
> sbenza wrote:
> > Any particular reason to use function template specialization instead of 
> > function overloading?
> > The former is rare and thus harder to reason about.
> > 
> > (same for `evalData` below)
> Stylistic -- it forces you to explicitly declare the "overload set" so to 
> speak.  However, i'm not particularly attached -- if you think this a bad 
> idea given its rarity, I'm happy to use overload sets. WDYT?
Converted to normal overload sets (as per off-thread chat).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59371



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

[PATCH] D60568: [OpenMP] Add support for registering requires directives with the runtime

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 195608.
gtbercea marked an inline comment as done.
gtbercea added a comment.

- Add support for declare target routines.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60568

Files:
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CGOpenMPRuntime.h
  lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  lib/CodeGen/CGOpenMPRuntimeNVPTX.h
  lib/CodeGen/CodeGenModule.cpp
  test/OpenMP/openmp_offload_registration.cpp

Index: test/OpenMP/openmp_offload_registration.cpp
===
--- test/OpenMP/openmp_offload_registration.cpp
+++ test/OpenMP/openmp_offload_registration.cpp
@@ -26,7 +26,7 @@
 // CHECK: [[DESC:@.+]] = internal constant [[DSCTY]] { i32 2, [[DEVTY]]* getelementptr inbounds ([2 x [[DEVTY]]], [2 x [[DEVTY]]]* [[IMAGES]], i32 0, i32 0), [[ENTTY]]* [[ENTBEGIN]], [[ENTTY]]* [[ENTEND]] }, comdat($[[REGFN]])
 
 // Check target registration is registered as a Ctor.
-// CHECK: appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 0, void ()* @[[REGFN]], i8* bitcast (void ()* @[[REGFN]] to i8*) }]
+// CHECK: appending global [2 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 0, void ()* @.omp_offloading.requires_reg, i8* null }, { i32, void ()*, i8* } { i32 0, void ()* @[[REGFN]], i8* bitcast (void ()* @[[REGFN]] to i8*) }]
 
 // Check presence of foo() and the outlined target region
 // CHECK: define void [[FOO:@.+]]()
@@ -34,6 +34,11 @@
 
 // Check registration and unregistration code.
 
+// CHECK: define internal void @.omp_offloading.requires_reg()
+// CHECK: call void @__tgt_register_requires(i64 0)
+// CHECK: ret void
+// CHECK: declare void @__tgt_register_requires(i64)
+
 // CHECK: define internal void @[[UNREGFN:.+]](i8*)
 // CHECK-SAME: comdat($[[REGFN]]) {
 // CHECK: call i32 @__tgt_unregister_lib([[DSCTY]]* [[DESC]])
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -410,6 +410,10 @@
   AddGlobalCtor(CudaCtorFunction);
   }
   if (OpenMPRuntime) {
+if (llvm::Function *OpenMPRequiresDirectiveRegFun =
+OpenMPRuntime->emitRequiresDirectiveRegFun()) {
+  AddGlobalCtor(OpenMPRequiresDirectiveRegFun, 0);
+}
 if (llvm::Function *OpenMPRegistrationFunction =
 OpenMPRuntime->emitRegistrationFunction()) {
   auto ComdatKey = OpenMPRegistrationFunction->hasComdat() ?
Index: lib/CodeGen/CGOpenMPRuntimeNVPTX.h
===
--- lib/CodeGen/CGOpenMPRuntimeNVPTX.h
+++ lib/CodeGen/CGOpenMPRuntimeNVPTX.h
@@ -383,7 +383,7 @@
 
   /// Perform check on requires decl to ensure that target architecture
   /// supports unified addressing
-  void checkArchForUnifiedAddressing(const OMPRequiresDecl *D) const override;
+  void checkArchForUnifiedAddressing(const OMPRequiresDecl *D) override;
 
   /// Returns default address space for the constant firstprivates, __constant__
   /// address space by default.
Index: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
===
--- lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
+++ lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
@@ -4942,7 +4942,7 @@
 /// Check to see if target architecture supports unified addressing which is
 /// a restriction for OpenMP requires clause "unified_shared_memory".
 void CGOpenMPRuntimeNVPTX::checkArchForUnifiedAddressing(
-const OMPRequiresDecl *D) const {
+const OMPRequiresDecl *D) {
   for (const OMPClause *Clause : D->clauselists()) {
 if (Clause->getClauseKind() == OMPC_unified_shared_memory) {
   switch (getCudaArch(CGM)) {
@@ -4987,6 +4987,7 @@
   }
 }
   }
+  CGOpenMPRuntime::checkArchForUnifiedAddressing(D);
 }
 
 /// Get number of SMs and number of blocks per SM.
Index: lib/CodeGen/CGOpenMPRuntime.h
===
--- lib/CodeGen/CGOpenMPRuntime.h
+++ lib/CodeGen/CGOpenMPRuntime.h
@@ -636,6 +636,17 @@
   /// must be emitted.
   llvm::SmallDenseSet DeferredGlobalVariables;
 
+  /// Flag for keeping track of weather a requires unified_shared_memory
+  /// directive is present.
+  bool HasRequiresUnifiedSharedMemory = false;
+
+  /// Flag for keeping track of weather a target region has been emitted.
+  bool HasEmittedTargetRegion = false;
+
+  /// Flag for keeping track of weather a device routine has been emitted.
+  /// Device routines are specific to the
+  bool HasEmittedDeclareTargetRegion = false;
+
   /// Creates and registers offloading binary descriptor for the current
   /// compilation unit. The function that does the registration is returned.
   llvm::Function *createOffloadingBinaryDescriptorRegistration();
@@ -1429,6 +1440,10 @@
   /// \param GD Global to scan.
   virtual bool emitTargetGlobal(Globa

[PATCH] D60568: [OpenMP] Add support for registering requires directives with the runtime

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea updated this revision to Diff 195609.
gtbercea marked an inline comment as done.
gtbercea added a comment.

- Update error message.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60568

Files:
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CGOpenMPRuntime.h
  lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  lib/CodeGen/CGOpenMPRuntimeNVPTX.h
  lib/CodeGen/CodeGenModule.cpp
  test/OpenMP/openmp_offload_registration.cpp

Index: test/OpenMP/openmp_offload_registration.cpp
===
--- test/OpenMP/openmp_offload_registration.cpp
+++ test/OpenMP/openmp_offload_registration.cpp
@@ -26,7 +26,7 @@
 // CHECK: [[DESC:@.+]] = internal constant [[DSCTY]] { i32 2, [[DEVTY]]* getelementptr inbounds ([2 x [[DEVTY]]], [2 x [[DEVTY]]]* [[IMAGES]], i32 0, i32 0), [[ENTTY]]* [[ENTBEGIN]], [[ENTTY]]* [[ENTEND]] }, comdat($[[REGFN]])
 
 // Check target registration is registered as a Ctor.
-// CHECK: appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 0, void ()* @[[REGFN]], i8* bitcast (void ()* @[[REGFN]] to i8*) }]
+// CHECK: appending global [2 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 0, void ()* @.omp_offloading.requires_reg, i8* null }, { i32, void ()*, i8* } { i32 0, void ()* @[[REGFN]], i8* bitcast (void ()* @[[REGFN]] to i8*) }]
 
 // Check presence of foo() and the outlined target region
 // CHECK: define void [[FOO:@.+]]()
@@ -34,6 +34,11 @@
 
 // Check registration and unregistration code.
 
+// CHECK: define internal void @.omp_offloading.requires_reg()
+// CHECK: call void @__tgt_register_requires(i64 0)
+// CHECK: ret void
+// CHECK: declare void @__tgt_register_requires(i64)
+
 // CHECK: define internal void @[[UNREGFN:.+]](i8*)
 // CHECK-SAME: comdat($[[REGFN]]) {
 // CHECK: call i32 @__tgt_unregister_lib([[DSCTY]]* [[DESC]])
Index: lib/CodeGen/CodeGenModule.cpp
===
--- lib/CodeGen/CodeGenModule.cpp
+++ lib/CodeGen/CodeGenModule.cpp
@@ -410,6 +410,10 @@
   AddGlobalCtor(CudaCtorFunction);
   }
   if (OpenMPRuntime) {
+if (llvm::Function *OpenMPRequiresDirectiveRegFun =
+OpenMPRuntime->emitRequiresDirectiveRegFun()) {
+  AddGlobalCtor(OpenMPRequiresDirectiveRegFun, 0);
+}
 if (llvm::Function *OpenMPRegistrationFunction =
 OpenMPRuntime->emitRegistrationFunction()) {
   auto ComdatKey = OpenMPRegistrationFunction->hasComdat() ?
Index: lib/CodeGen/CGOpenMPRuntimeNVPTX.h
===
--- lib/CodeGen/CGOpenMPRuntimeNVPTX.h
+++ lib/CodeGen/CGOpenMPRuntimeNVPTX.h
@@ -383,7 +383,7 @@
 
   /// Perform check on requires decl to ensure that target architecture
   /// supports unified addressing
-  void checkArchForUnifiedAddressing(const OMPRequiresDecl *D) const override;
+  void checkArchForUnifiedAddressing(const OMPRequiresDecl *D) override;
 
   /// Returns default address space for the constant firstprivates, __constant__
   /// address space by default.
Index: lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
===
--- lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
+++ lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
@@ -4942,7 +4942,7 @@
 /// Check to see if target architecture supports unified addressing which is
 /// a restriction for OpenMP requires clause "unified_shared_memory".
 void CGOpenMPRuntimeNVPTX::checkArchForUnifiedAddressing(
-const OMPRequiresDecl *D) const {
+const OMPRequiresDecl *D) {
   for (const OMPClause *Clause : D->clauselists()) {
 if (Clause->getClauseKind() == OMPC_unified_shared_memory) {
   switch (getCudaArch(CGM)) {
@@ -4987,6 +4987,7 @@
   }
 }
   }
+  CGOpenMPRuntime::checkArchForUnifiedAddressing(D);
 }
 
 /// Get number of SMs and number of blocks per SM.
Index: lib/CodeGen/CGOpenMPRuntime.h
===
--- lib/CodeGen/CGOpenMPRuntime.h
+++ lib/CodeGen/CGOpenMPRuntime.h
@@ -636,6 +636,17 @@
   /// must be emitted.
   llvm::SmallDenseSet DeferredGlobalVariables;
 
+  /// Flag for keeping track of weather a requires unified_shared_memory
+  /// directive is present.
+  bool HasRequiresUnifiedSharedMemory = false;
+
+  /// Flag for keeping track of weather a target region has been emitted.
+  bool HasEmittedTargetRegion = false;
+
+  /// Flag for keeping track of weather a device routine has been emitted.
+  /// Device routines are specific to the
+  bool HasEmittedDeclareTargetRegion = false;
+
   /// Creates and registers offloading binary descriptor for the current
   /// compilation unit. The function that does the registration is returned.
   llvm::Function *createOffloadingBinaryDescriptorRegistration();
@@ -1429,6 +1440,10 @@
   /// \param GD Global to scan.
   virtual bool emitTargetGlobal(GlobalDecl GD);
 
+  ///

[PATCH] D60719: Demonstrate how to fix freestanding for memcpy

[Tim Northover via Phabricator via cfe-commits]

t.p.northover added a comment.

> IIUC freestanding environment should not rely on memcpy being present so my 
> take on it was that by "fixing" freestanding I could have my cake and eat it 
> too.

The formal situation is that freestanding implementations are only required to 
provide language support stuff like `va_list`. They're allowed to give more of 
the standard library if they want though, as implementation defined behaviour.

In practice, everyone I know provides the basic `string.h` functions and the 
compiler is pretty explicit about relying on them being present for 
correctness. They're part of the surrounding environment a user is expected to 
supply (much like the various exception handling callbacks if you want C++ 
exceptions, but always required).

For the people actually using freestanding I think they're mostly considered 
important enough that they're implemented in assembly anyway so this isn't 
really a burden, but...

> Ultimately I'm interested in implementing libc's mem function in C++. Let's 
> take memcpy for instance

Ah, that is an entirely different problem from what I thought you were trying 
to do. In that case I'm all in favour of some solution, but would start 
thinking along the lines of `-fno-builtin-memcpy` options (it's possible that 
already does what you want, but can't get LLVM to form a `memcpy` from quick 
tests to check).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60719



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

[PATCH] D60568: [OpenMP] Add support for registering requires directives with the runtime

[Gheorghe-Teodor Bercea via Phabricator via cfe-commits]

gtbercea marked 2 inline comments as done.
gtbercea added inline comments.



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8975
+CGM.Error(Clause->getBeginLoc(),
+  "Target region emitted before requires directive.");
+  HasRequiresUnifiedSharedMemory = true;

ABataev wrote:
> The message speaks about target region but points to the clause. You need to 
> correct the message.
"A target region was emitted before this requires directive." ?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60568



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