[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

rjmccall wrote:
> Hmm.  So adding a signed integer to an unsigned fixed-point type always 
> converts the integer to unsigned?  That's a little weird, but only because 
> the fixed-point rule seems to be the other way.  Anyway, I assume it's not a 
> bug in the spec.
`handleFixedPointConversion` only calculates the result type of the expression, 
not the calculation type. The final result type of the operation will be the 
unsigned fixed-point type, but the calculation itself will be done in a signed 
type with enough precision to represent both the signed integer and the 
unsigned fixed-point type. 

Though, if the result ends up being negative, the final result is undefined 
unless the type is saturating. I don't think there is a test for the saturating 
case (signed integer + unsigned saturating fixed-point) in the 
SaturatedAddition tests.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



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

[PATCH] D55065: [clangd] Drop injected class name when class scope is not explicitly specified.

[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, thanks!




Comment at: unittests/clangd/CodeCompleteTests.cpp:419
 
+TEST(CompletionTest, SkipInjectedWhenUnqualified) {
+  EXPECT_THAT(completions("struct X { void f() { X^ }};").Completions,

Could you also add a test for the inheritance of injected class name? For ex:

```
struct X {};
struct T : private X {};
struct F : public T {
X^ <-- this is inavlid and we should not suggest X here, since it is 
inaccessible.
::X^ <-- this should be Ok.
};
```


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55065



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

[PATCH] D55065: [clangd] Drop injected class name when class scope is not explicitly specified.

[Eric Liu via Phabricator via cfe-commits]

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



Comment at: unittests/clangd/CodeCompleteTests.cpp:419
 
+TEST(CompletionTest, SkipInjectedWhenUnqualified) {
+  EXPECT_THAT(completions("struct X { void f() { X^ }};").Completions,

kadircet wrote:
> Could you also add a test for the inheritance of injected class name? For ex:
> 
> ```
> struct X {};
> struct T : private X {};
> struct F : public T {
> X^ <-- this is inavlid and we should not suggest X here, since it is 
> inaccessible.
> ::X^ <-- this should be Ok.
> };
> ```
Happy to improve the test coverage. But I couldn't see how this tests changes 
in this patch. It seems something sema access should be testing and a bit out 
of scope? Could you elaborate on the intention? Thanks!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55065



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

[PATCH] D55065: [clangd] Drop injected class name when class scope is not explicitly specified.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: unittests/clangd/CodeCompleteTests.cpp:419
 
+TEST(CompletionTest, SkipInjectedWhenUnqualified) {
+  EXPECT_THAT(completions("struct X { void f() { X^ }};").Completions,

ioeric wrote:
> kadircet wrote:
> > Could you also add a test for the inheritance of injected class name? For 
> > ex:
> > 
> > ```
> > struct X {};
> > struct T : private X {};
> > struct F : public T {
> > X^ <-- this is inavlid and we should not suggest X here, since it is 
> > inaccessible.
> > ::X^ <-- this should be Ok.
> > };
> > ```
> Happy to improve the test coverage. But I couldn't see how this tests changes 
> in this patch. It seems something sema access should be testing and a bit out 
> of scope? Could you elaborate on the intention? Thanks!
Ah you are right, it should be rather handled within sema to mark the first one 
as not accessible.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55065



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

[clang-tools-extra] r347968 - [clangd] Fix junk output in clangd vscode plugin

[Haojian Wu via cfe-commits]

Author: hokein
Date: Fri Nov 30 01:14:52 2018
New Revision: 347968

URL: http://llvm.org/viewvc/llvm-project?rev=347968&view=rev
Log:
[clangd] Fix junk output in clangd vscode plugin

Summary:
When using the vscode clangd plugin, lots and lots of junk output is printed to 
the output window, which constantly reopens itself.
Example output:

I[11:13:17.733] <-- textDocument/codeAction(4)
I[11:13:17.733] --> reply:textDocument/codeAction(4) 0 ms
I[11:13:17.937] <-- textDocument/codeAction(5)
I[11:13:17.937] --> reply:textDocument/codeAction(5) 0 ms
I[11:13:18.557] <-- textDocument/hover(6)
I[11:13:18.606] --> reply:textDocument/hover(6) 48 ms

This should prevent that from happening.

Patch by James Findley!

Reviewers: ioeric, ilya-biryukov, hokein

Reviewed By: ioeric

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

Tags: #clang-tools-extra

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

Modified:
clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts

Modified: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts?rev=347968&r1=347967&r2=347968&view=diff
==
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts 
(original)
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts Fri 
Nov 30 01:14:52 2018
@@ -54,7 +54,9 @@ export function activate(context: vscode
 code2Protocol: (value: vscode.Uri) => value.toString(),
 protocol2Code: (value: string) =>
 vscode.Uri.file(realpathSync(vscode.Uri.parse(value).fsPath))
-}
+},
+// Do not switch to output window when clangd returns output
+revealOutputChannelOn: vscodelc.RevealOutputChannelOn.Never
 };
 
   const clangdClient = new vscodelc.LanguageClient('Clang Language Server', 
serverOptions, clientOptions);


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

[PATCH] D55052: Fix junk output in clangd vscode plugin

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL347968: [clangd] Fix junk output in clangd vscode plugin 
(authored by hokein, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55052?vs=175897&id=176049#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55052

Files:
  clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts


Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
@@ -54,7 +54,9 @@
 code2Protocol: (value: vscode.Uri) => value.toString(),
 protocol2Code: (value: string) =>
 vscode.Uri.file(realpathSync(vscode.Uri.parse(value).fsPath))
-}
+},
+// Do not switch to output window when clangd returns output
+revealOutputChannelOn: vscodelc.RevealOutputChannelOn.Never
 };
 
   const clangdClient = new vscodelc.LanguageClient('Clang Language Server', 
serverOptions, clientOptions);


Index: clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
===
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/extension.ts
@@ -54,7 +54,9 @@
 code2Protocol: (value: vscode.Uri) => value.toString(),
 protocol2Code: (value: string) =>
 vscode.Uri.file(realpathSync(vscode.Uri.parse(value).fsPath))
-}
+},
+// Do not switch to output window when clangd returns output
+revealOutputChannelOn: vscodelc.RevealOutputChannelOn.Never
 };
 
   const clangdClient = new vscodelc.LanguageClient('Clang Language Server', serverOptions, clientOptions);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r347969 - [clangd] Bump vscode-clangd v0.0.8

[Haojian Wu via cfe-commits]

Author: hokein
Date: Fri Nov 30 01:18:31 2018
New Revision: 347969

URL: http://llvm.org/viewvc/llvm-project?rev=347969&view=rev
Log:
[clangd] Bump vscode-clangd v0.0.8

Modified:
clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json

Modified: clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json?rev=347969&r1=347968&r2=347969&view=diff
==
--- clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json (original)
+++ clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json Fri Nov 
30 01:18:31 2018
@@ -2,7 +2,7 @@
 "name": "vscode-clangd",
 "displayName": "vscode-clangd",
 "description": "Clang Language Server",
-"version": "0.0.7",
+"version": "0.0.8",
 "publisher": "llvm-vs-code-extensions",
 "homepage": "https://clang.llvm.org/extra/clangd.html";,
 "engines": {


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

r347970 - Fix a use-after-scope bug.

[Haojian Wu via cfe-commits]

Author: hokein
Date: Fri Nov 30 01:23:01 2018
New Revision: 347970

URL: http://llvm.org/viewvc/llvm-project?rev=347970&view=rev
Log:
Fix a use-after-scope bug.

Modified:

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

Modified: 
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp?rev=347970&r1=347969&r2=347970&view=diff
==
--- 
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
 (original)
+++ 
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
 Fri Nov 30 01:23:01 2018
@@ -31,7 +31,7 @@ static bool isNumericLiteralExpression(c
 /// If type represents a pointer to CXXRecordDecl,
 /// and is not a typedef, return the decl name.
 /// Otherwise, return the serialization of type.
-static StringRef getPrettyTypeName(QualType QT) {
+static std::string getPrettyTypeName(QualType QT) {
   QualType PT = QT->getPointeeType();
   if (!PT.isNull() && !QT->getAs())
 if (const auto *RD = PT->getAsCXXRecordDecl())


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

[PATCH] D55052: Fix junk output in clangd vscode plugin

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

> Yeah, there are still modes where clangd behaves badly there. Note that the 
> output from that pane is rarely useful anyway as clangd keeps producing 
> errors about accessing non-open files, which would confuse people even more.
>  Overall we never designed this output to be readable by the users, so it's 
> not really useful anyway. There are ways to communicate with the users in the 
> protocol, we should be using them instead to be more user-friendly

SG.

@jfindley, thanks for the patch. I made a new release v0.0.8 of vscode-clangd 
extension, please upgrade to it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55052



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

[PATCH] D54995: [MemoryBuffer] By default assume that all files are volatile to prevent unintended file locks

[Ivan Donchevskii via Phabricator via cfe-commits]

yvvan added a comment.

No success with unmapping. Buffers are hold by PCMCache in Preprocessor (and 
the same one in ASTReader) and if I clean them then SourceManger crashed sooner 
or later on some call that gets data from external files.
So far I have no steps to reproduce the lock on user file so I don't currently 
know if I try something else.


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

https://reviews.llvm.org/D54995



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

[PATCH] D54905: [AddressSanitizer] Add flag to disable linking with CXX runtime

[Eugene Leviant via Phabricator via cfe-commits]

evgeny777 added a comment.

> How do you do this now?

Sometimes with this patch, sometimes simply issuing linker command line manually

> Is that related to https://github.com/google/sanitizers/issues/295 ?

I don't think so. I'm experiencing link error, not C++ standard discrepancy.

> Would turning asan operator new/delete into weak symbols help?

The new operator is already a weak symbol in libcxx, so it looks like an ODR 
violation, doesn't it?


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

https://reviews.llvm.org/D54905



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

[PATCH] D55061: [clangd] Penalize destructor and overloaded operators in code completion.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.



Comment at: clangd/Quality.h:74
 Keyword,
+Operator,
   } Category = Unknown;

Maybe name it `OverloadedOperator` to avoid confusion with other non-overloaded 
operators like `?:`



Comment at: unittests/clangd/QualityTests.cpp:428
+
+TEST(QualityTests, OperatorQuality) {
+  auto Header = TestTU::withHeaderCode(R"cpp(

The test name `*Quality` is a bit confusing. At the first glance, I'd expect to 
see some tests about ranking, but it just verifies that the new symbol category 
is added, maybe name it something like `OperatorSymbolCategory`.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55061



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

[PATCH] D54999: [clangd] Populate include graph during static indexing action.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: unittests/clangd/IndexActionTests.cpp:168
+  std::string MainFilePath = testPath("main.cpp");
+  std::pair CommonHeader = {testPath("common.h"),
+  R"cpp(

kadircet wrote:
> ilya-biryukov wrote:
> > Maybe have two variables instead? Would format better and `HeaderPath` is 
> > arguably more readable than `Header.first`
> Is it that important?
I think it is of minor importance.

Creating a pair that is deconstructed at every use site anyway hurts 
readability.
If the purpose is grouping the variables together, putting them close to each 
other and giving them similar names solves the same purpose.

I would also expect the raw strings to format better and not have a very long 
indent if we put them into separate variables.



Comment at: unittests/clangd/IndexActionTests.cpp:225
+  ASSERT_TRUE(IndexFile.Sources);
+  auto Nodes = toMap(*IndexFile.Sources);
+

kadircet wrote:
> ilya-biryukov wrote:
> > Why not `auto Nodes = toMap(*runIndexingAction(MainFilePath).Sources)`?
> > Optional has an assertion, so we'll catch empty results anyway, no need to 
> > make the test more verbose
> because toMap doesn't modify the contents of the nodes(to make sure we don't 
> change behaviour during the process), and all the strings are still referring 
> to the keys of the graph. Therefore the graph needs to be kept alive.
I missed this. It makes sense, thanks for clarifying. It's ok to have the 
boilerplate then.
Would still remove `ASSERT_TRUE(IndexFile.Sources)`, asserts in optional will 
catch those anyway. But up to you.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54999



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

mikael updated this revision to Diff 176063.
Herald added subscribers: arphaman, eraman.

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

https://reviews.llvm.org/D54862

Files:
  include/clang/AST/DeclCXX.h
  include/clang/AST/Type.h
  lib/AST/DeclCXX.cpp
  lib/AST/ItaniumMangle.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGClass.cpp
  lib/CodeGen/CGDeclCXX.cpp
  lib/Index/USRGeneration.cpp
  lib/Parse/ParseCXXInlineMethods.cpp
  lib/Sema/SemaCodeComplete.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaTemplateDeduction.cpp
  lib/Sema/SemaTemplateInstantiateDecl.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCLCXX/addrspace-of-this.cl
  tools/libclang/CIndex.cpp

Index: tools/libclang/CIndex.cpp
===
--- tools/libclang/CIndex.cpp
+++ tools/libclang/CIndex.cpp
@@ -8370,7 +8370,7 @@
   const Decl *D = cxcursor::getCursorDecl(C);
   const CXXMethodDecl *Method =
   D ? dyn_cast_or_null(D->getAsFunction()) : nullptr;
-  return (Method && (Method->getTypeQualifiers() & Qualifiers::Const)) ? 1 : 0;
+  return (Method && Method->getTypeQualifiers().hasConst()) ? 1 : 0;
 }
 
 unsigned clang_CXXMethod_isDefaulted(CXCursor C) {
Index: test/CodeGenOpenCLCXX/addrspace-of-this.cl
===
--- /dev/null
+++ test/CodeGenOpenCLCXX/addrspace-of-this.cl
@@ -0,0 +1,129 @@
+// RUN: %clang_cc1 %s -triple spir-unknown-unknown -cl-std=c++ -emit-llvm -pedantic -verify -O0 -o - | FileCheck %s
+// expected-no-diagnostics
+
+// Test that the 'this' pointer is in the __generic address space.
+
+// FIXME: Add support for __constant address space.
+
+class C {
+public:
+  int v;
+  C() { v = 2; }
+  C(const C &c) { v = c.v; }
+  C &operator=(const C &c) {
+v = c.v;
+return *this;
+  }
+  int get() { return v; }
+};
+
+__global C c;
+
+__kernel void test__global() {
+  int i = c.get();
+  C c1(c);
+  C c2;
+  c2 = c1;
+}
+
+// CHECK-LABEL: @__cxx_global_var_init()
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*)) #4
+
+// Test that the address space is __generic for the constructor
+// CHECK-LABEL: @_ZN1CC1Ev(%class.C addrspace(4)* %this)
+// CHECK: entry:
+// CHECK:   %this.addr = alloca %class.C addrspace(4)*, align 4
+// CHECK:   store %class.C addrspace(4)* %this, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   %this1 = load %class.C addrspace(4)*, %class.C addrspace(4)** %this.addr, align 4
+// CHECK:   call void @_ZN1CC2Ev(%class.C addrspace(4)* %this1) #4
+// CHECK:   ret void
+
+// CHECK-LABEL: @_Z12test__globalv()
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: %call = call i32 @_ZN1C3getEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking copy-constructor.
+// CHECK: %0 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK: call void @_ZN1CC1ERU3AS4KS_(%class.C addrspace(4)* %0, %class.C addrspace(4)* dereferenceable(4) addrspacecast (%class.C addrspace(1)* @c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking a constructor.
+// CHECK:   %1 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   call void @_ZN1CC1Ev(%class.C addrspace(4)* %1) #4
+
+// Test the address space of 'this' when invoking assignment operator.
+// CHECK:   %2 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK:   %3 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:   %call1 = call dereferenceable(4) %class.C addrspace(4)* @_ZN1CaSERU3AS4KS_(%class.C addrspace(4)* %3, %class.C addrspace(4)* dereferenceable(4) %2)
+
+#define TEST(AS) \
+  __kernel void test##AS() { \
+AS C c;  \
+int i = c.get(); \
+C c1(c); \
+C c2;\
+c2 = c1; \
+  }
+
+TEST(__local)
+
+// CHECK-LABEL: _Z11test__localv
+// CHECK: @__cxa_guard_acquire
+
+// Test the address space of 'this' when invoking a method.
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking copy-constructor.
+// CHECK: %call = call i32 @_ZN1C3getEv(%class.C addrspace(4)* addrspacecast (%class.C addrspace(3)* @_ZZ11test__localvE1c to %class.C addrspace(4)*))
+
+// Test the address space of 'this' when invoking a constructor.
+// CHECK: %3 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK: call void @_ZN1CC1Ev(%class.C addrspace(4)* %3)
+
+// Test the address space of 'this' when invoking assignment operator.
+// CHECK:  %4 = addrspacecast %class.C* %c1 to %class.C addrspace(4)*
+// CHECK:  %5 = addrspacecast %class.C* %c2 to %class.C addrspace(4)*
+// CHECK:  %call1 = cal

[PATCH] D55051: [Analyzer] [HOTFIX!] SValBuilder crash when `aggressive-binary-operation-simplification` enabled

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

baloghadamsoftware updated this revision to Diff 176064.
baloghadamsoftware added a comment.

Test added.


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

https://reviews.llvm.org/D55051

Files:
  lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  test/Analysis/svalbuilder-rearrange-comparisons.c


Index: test/Analysis/svalbuilder-rearrange-comparisons.c
===
--- test/Analysis/svalbuilder-rearrange-comparisons.c
+++ test/Analysis/svalbuilder-rearrange-comparisons.c
@@ -979,3 +979,20 @@
   short a = x - 1U;
   return a - y;
 }
+
+unsigned gu();
+unsigned fu() {
+  unsigned x = gu();
+  // Assert that no overflows occur in this test file.
+  // Assuming that concrete integers are also within that range.
+  assert(x <= ((unsigned)UINT_MAX / 4));
+  return x;
+}
+
+void unsigned_concrete_int_no_crash() {
+  unsigned x = fu() + 1U, y = fu() + 1U;
+  clang_analyzer_denote(x - 1U, "$x");
+  clang_analyzer_denote(y - 1U, "$y");
+  clang_analyzer_express(y); // expected-warning {{$y}}
+  clang_analyzer_express(x == y); // expected-warning {{$x + 1U == $y + 1U}}
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -475,9 +475,6 @@
 SingleTy = ResultTy;
 if (LSym->getType() != SingleTy)
   return None;
-// Substracting unsigned integers is a nightmare.
-if (!SingleTy->isSignedIntegerOrEnumerationType())
-  return None;
   } else {
 // Don't rearrange other operations.
 return None;
@@ -485,6 +482,10 @@
 
   assert(!SingleTy.isNull() && "We should have figured out the type by now!");
 
+  // Substracting unsigned integers is a nightmare.
+  if (!SingleTy->isSignedIntegerOrEnumerationType())
+return None;
+
   SymbolRef RSym = Rhs.getAsSymbol();
   if (!RSym || RSym->getType() != SingleTy)
 return None;


Index: test/Analysis/svalbuilder-rearrange-comparisons.c
===
--- test/Analysis/svalbuilder-rearrange-comparisons.c
+++ test/Analysis/svalbuilder-rearrange-comparisons.c
@@ -979,3 +979,20 @@
   short a = x - 1U;
   return a - y;
 }
+
+unsigned gu();
+unsigned fu() {
+  unsigned x = gu();
+  // Assert that no overflows occur in this test file.
+  // Assuming that concrete integers are also within that range.
+  assert(x <= ((unsigned)UINT_MAX / 4));
+  return x;
+}
+
+void unsigned_concrete_int_no_crash() {
+  unsigned x = fu() + 1U, y = fu() + 1U;
+  clang_analyzer_denote(x - 1U, "$x");
+  clang_analyzer_denote(y - 1U, "$y");
+  clang_analyzer_express(y); // expected-warning {{$y}}
+  clang_analyzer_express(x == y); // expected-warning {{$x + 1U == $y + 1U}}
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -475,9 +475,6 @@
 SingleTy = ResultTy;
 if (LSym->getType() != SingleTy)
   return None;
-// Substracting unsigned integers is a nightmare.
-if (!SingleTy->isSignedIntegerOrEnumerationType())
-  return None;
   } else {
 // Don't rearrange other operations.
 return None;
@@ -485,6 +482,10 @@
 
   assert(!SingleTy.isNull() && "We should have figured out the type by now!");
 
+  // Substracting unsigned integers is a nightmare.
+  if (!SingleTy->isSignedIntegerOrEnumerationType())
+return None;
+
   SymbolRef RSym = Rhs.getAsSymbol();
   if (!RSym || RSym->getType() != SingleTy)
 return None;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55054: [clang] Fill RealPathName for virtual files.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: include/clang/Basic/FileManager.h:177
+  /// Fills the RealPathName entry in UFE by converting the given filename to 
an
+  /// absolute path, returns true if FileName was modified.
+  bool fillRealPathName(FileEntry *UFE, llvm::StringRef FileName);

The FIXME inside the function suggests we might change the name or semantics of 
RealPathName, so maybe simply refer to the field by name and avoid mentioning 
the semantics of the function.



Comment at: unittests/Basic/FileManagerTest.cpp:237
   ASSERT_TRUE(file->isValid());
   // "real path name" reveals whether the file was actually opened.
   EXPECT_EQ("", file->tryGetRealPathName());

This test seems to rely on the old behavior. Is there a way to test the file 
wasn't open other than looking at RealPathName?



Comment at: unittests/Basic/FileManagerTest.cpp:238
   // "real path name" reveals whether the file was actually opened.
   EXPECT_EQ("", file->tryGetRealPathName());
 

Should we fill-in the RealPathName in that case as well?
I'm not sure what the semantics should be. WDYT?



Comment at: unittests/Basic/FileManagerTest.cpp:253
   file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
-  EXPECT_EQ("", file->tryGetRealPathName());
 }

We should find a way to test file was not "opened" (I think it actually does 
stat the file now, right?)



Comment at: unittests/Basic/FileManagerTest.cpp:327
+
+  EXPECT_EQ(file1->tryGetRealPathName(), file2->tryGetRealPathName());
 }

After a check `EXPECT_EQ(file1, file2)`, this is clearly true.
Maybe add a smaller test with a single call to `getVirtualFile()` and an 
assertion that the real path is set?
Alternatively, add an assertion to this test right after the first call to 
`getVirtualFile()`.



Repository:
  rC Clang

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

https://reviews.llvm.org/D55054



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

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

[Mikael Nilsson via Phabricator via cfe-commits]

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



Comment at: lib/Sema/SemaOverload.cpp:5254
+  if (!Context.hasSameType(From->getType(), DestType)) {
+if (From->getType().getAddressSpace() != DestType.getAddressSpace())
+  From = ImpCastExprToType(From, DestType, CK_AddressSpaceConversion,

Note that this change allowed me to revert some of the CG changes.


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

https://reviews.llvm.org/D54862



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 176066.
donat.nagy added a comment.

Handle ternary operators, improve documentation


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757

Files:
  clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tidy/bugprone/BranchCloneCheck.h
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-branch-clone.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-branch-clone.cpp

Index: test/clang-tidy/bugprone-branch-clone.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-branch-clone.cpp
@@ -0,0 +1,1017 @@
+// RUN: %check_clang_tidy %s bugprone-branch-clone %t
+
+void test_basic1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic2(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+  }
+
+  out++;
+}
+
+void test_basic3(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic4(int in, int &out) {
+  if (in > 77) {
+out--;
+  }
+  else {
+out++;
+  }
+}
+
+void test_basic5(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++;
+out++;
+  }
+}
+
+void test_basic6(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++, out++;
+  }
+}
+
+void test_basic7(int in, int &out) {
+  if (in > 77) {
+out++;
+out++;
+  }
+  else
+out++;
+
+  out++;
+}
+
+void test_basic8(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+out++;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+out++;
+out++;
+  }
+
+  if (in % 2)
+out++;
+}
+
+void test_basic9(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+  }
+}
+
+// If we remove the braces from the previous example, the check recognizes it
+// as an `else if`.
+void test_basic10(int in, int &out) {
+  if (in > 77)
+if (in % 2)
+  out++;
+else
+  out--;
+  else
+if (in % 2)
+  out++;
+else
+  out--;
+
+}
+
+void test_basic11(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  }
+}
+
+void test_basic12(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+  }
+}
+
+void test_basic13(int in, int &out) {
+  if (in > 77) {
+// Empty compound statement is not identical to null statement.
+  } else;
+}
+
+// We use a comparison that ignores redundant parentheses:
+void test_basic14(int in, int &out) {
+  if (in > 77)
+out += 2;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+(out) += (2);
+}
+
+void test_basic15(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+((out += 2));
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out += 2;
+}
+
+// ..but does not apply additional simplifications: 
+void test_basic16(int in, int &out) {
+  if (in > 77)
+out += 2;
+  else
+out += 1 + 1;
+}
+
+// ..and does not forget important parentheses:
+int test_basic17(int a, int b, int c, int mode) {
+  if (mode>8)
+return (a + b) * c;
+  else
+return a + b * c;
+}
+
+//===//

[PATCH] D54999: [clangd] Populate include graph during static indexing action.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 176065.
kadircet marked 4 inline comments as done.
kadircet added a comment.

- Address comments.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54999

Files:
  clangd/Headers.h
  clangd/index/IndexAction.cpp
  clangd/index/IndexAction.h
  unittests/clangd/CMakeLists.txt
  unittests/clangd/IndexActionTests.cpp

Index: unittests/clangd/IndexActionTests.cpp
===
--- /dev/null
+++ unittests/clangd/IndexActionTests.cpp
@@ -0,0 +1,229 @@
+//===-- IndexActionTests.cpp  ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "TestFS.h"
+#include "index/IndexAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using ::testing::AllOf;
+using ::testing::ElementsAre;
+using ::testing::Not;
+using ::testing::Pair;
+using ::testing::UnorderedElementsAre;
+using ::testing::UnorderedPointwise;
+
+std::string toUri(llvm::StringRef Path) { return URI::create(Path).toString(); }
+
+MATCHER(IsTU, "") { return arg.IsTU; }
+
+MATCHER_P(HasDigest, Digest, "") { return arg.Digest == Digest; }
+
+MATCHER(HasSameURI, "") {
+  llvm::StringRef URI = testing::get<0>(arg);
+  const std::string &Path = testing::get<1>(arg);
+  return toUri(Path) == URI;
+}
+
+testing::Matcher
+IncludesAre(const std::vector &Includes) {
+  return ::testing::Field(&IncludeGraphNode::DirectIncludes,
+  UnorderedPointwise(HasSameURI(), Includes));
+}
+
+void checkNodesAreInitialized(const IndexFileIn &IndexFile,
+  const std::vector &Paths) {
+  for (llvm::StringRef Path : Paths) {
+auto URI = toUri(Path);
+const auto &Node = IndexFile.Sources->lookup(URI);
+// Uninitialized nodes will have an empty URI.
+EXPECT_EQ(Node.URI.data(), IndexFile.Sources->find(URI)->getKeyData());
+  }
+}
+
+std::map toMap(const IncludeGraph &IG) {
+  std::map Nodes;
+  for (auto &I : IG)
+Nodes.emplace(I.getKey(), I.getValue());
+  return Nodes;
+}
+
+class IndexActionTest : public ::testing::Test {
+public:
+  IndexActionTest() : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem) {}
+
+  IndexFileIn
+  runIndexingAction(llvm::StringRef MainFilePath,
+const std::vector &ExtraArgs = {}) {
+IndexFileIn IndexFile;
+IntrusiveRefCntPtr Files(
+new FileManager(FileSystemOptions(), InMemoryFileSystem));
+
+auto Action = createStaticIndexingAction(
+SymbolCollector::Options(),
+[&](SymbolSlab S) { IndexFile.Symbols = std::move(S); },
+[&](RefSlab R) { IndexFile.Refs = std::move(R); },
+[&](IncludeGraph IG) { IndexFile.Sources = std::move(IG); });
+
+std::vector Args = {"index_action", "-fsyntax-only",
+ "-xc++","-std=c++11",
+ "-iquote",  testRoot()};
+Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+Args.push_back(MainFilePath);
+
+tooling::ToolInvocation Invocation(
+Args, Action.release(), Files.get(),
+std::make_shared());
+
+Invocation.run();
+
+checkNodesAreInitialized(IndexFile, FilePaths);
+return IndexFile;
+  }
+
+  void addFile(llvm::StringRef Path, llvm::StringRef Content) {
+InMemoryFileSystem->addFile(Path, 0,
+llvm::MemoryBuffer::getMemBuffer(Content));
+FilePaths.push_back(Path);
+  }
+
+protected:
+  std::vector FilePaths;
+  IntrusiveRefCntPtr InMemoryFileSystem;
+};
+
+TEST_F(IndexActionTest, CollectIncludeGraph) {
+  std::string MainFilePath = testPath("main.cpp");
+  std::string MainCode = "#include \"level1.h\"";
+  std::string Level1HeaderPath = testPath("level1.h");
+  std::string Level1HeaderCode = "#include \"level2.h\"";
+  std::string Level2HeaderPath = testPath("level2.h");
+  std::string Level2HeaderCode = "";
+
+  addFile(MainFilePath, MainCode);
+  addFile(Level1HeaderPath, Level1HeaderCode);
+  addFile(Level2HeaderPath, Level2HeaderCode);
+
+  IndexFileIn IndexFile = runIndexingAction(MainFilePath);
+  auto Nodes = toMap(*IndexFile.Sources);
+
+  EXPECT_THAT(Nodes,
+  UnorderedElementsAre(
+  Pair(toUri(MainFilePath),
+   AllOf(IsTU(), IncludesAre({Level1HeaderPath}),
+ HasDigest(digest(MainCode,
+  Pair(toUri(Level1HeaderPath),
+   AllOf(Not(IsTU()), IncludesAre({Level2HeaderPath}),
+ HasDigest(digest(Level1HeaderCode,
+

r347981 - lyzer] [HOTFIX!] SValBuilder crash when `aggressive-binary-operation-simplification` enabled

[Adam Balogh via cfe-commits]

Author: baloghadamsoftware
Date: Fri Nov 30 02:37:44 2018
New Revision: 347981

URL: http://llvm.org/viewvc/llvm-project?rev=347981&view=rev
Log:
lyzer] [HOTFIX!] SValBuilder crash when 
`aggressive-binary-operation-simplification` enabled

During the review of D41938 a condition check with an early exit accidentally
slipped into a branch, leaving the other branch unprotected. This may result in
an assertion later on. This hotfix moves this contition check outside of the
branch.

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


Modified:
cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
cfe/trunk/test/Analysis/svalbuilder-rearrange-comparisons.c

Modified: cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp?rev=347981&r1=347980&r2=347981&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp Fri Nov 30 02:37:44 
2018
@@ -475,9 +475,6 @@ static Optional tryRearrange(Pro
 SingleTy = ResultTy;
 if (LSym->getType() != SingleTy)
   return None;
-// Substracting unsigned integers is a nightmare.
-if (!SingleTy->isSignedIntegerOrEnumerationType())
-  return None;
   } else {
 // Don't rearrange other operations.
 return None;
@@ -485,6 +482,10 @@ static Optional tryRearrange(Pro
 
   assert(!SingleTy.isNull() && "We should have figured out the type by now!");
 
+  // Rearrange signed symbolic expressions only
+  if (!SingleTy->isSignedIntegerOrEnumerationType())
+return None;
+
   SymbolRef RSym = Rhs.getAsSymbol();
   if (!RSym || RSym->getType() != SingleTy)
 return None;

Modified: cfe/trunk/test/Analysis/svalbuilder-rearrange-comparisons.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/svalbuilder-rearrange-comparisons.c?rev=347981&r1=347980&r2=347981&view=diff
==
--- cfe/trunk/test/Analysis/svalbuilder-rearrange-comparisons.c (original)
+++ cfe/trunk/test/Analysis/svalbuilder-rearrange-comparisons.c Fri Nov 30 
02:37:44 2018
@@ -979,3 +979,20 @@ int mixed_integer_types(int x, int y) {
   short a = x - 1U;
   return a - y;
 }
+
+unsigned gu();
+unsigned fu() {
+  unsigned x = gu();
+  // Assert that no overflows occur in this test file.
+  // Assuming that concrete integers are also within that range.
+  assert(x <= ((unsigned)UINT_MAX / 4));
+  return x;
+}
+
+void unsigned_concrete_int_no_crash() {
+  unsigned x = fu() + 1U, y = fu() + 1U;
+  clang_analyzer_denote(x - 1U, "$x");
+  clang_analyzer_denote(y - 1U, "$y");
+  clang_analyzer_express(y); // expected-warning {{$y}}
+  clang_analyzer_express(x == y); // expected-warning {{$x + 1U == $y + 1U}}
+}


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

[PATCH] D55051: [Analyzer] [HOTFIX!] SValBuilder crash when `aggressive-binary-operation-simplification` enabled

[Balogh , Ádám 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 rC347981: lyzer] [HOTFIX!] SValBuilder crash when 
`aggressive-binary-operation… (authored by baloghadamsoftware, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55051?vs=176064&id=176067#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55051

Files:
  lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  test/Analysis/svalbuilder-rearrange-comparisons.c


Index: test/Analysis/svalbuilder-rearrange-comparisons.c
===
--- test/Analysis/svalbuilder-rearrange-comparisons.c
+++ test/Analysis/svalbuilder-rearrange-comparisons.c
@@ -979,3 +979,20 @@
   short a = x - 1U;
   return a - y;
 }
+
+unsigned gu();
+unsigned fu() {
+  unsigned x = gu();
+  // Assert that no overflows occur in this test file.
+  // Assuming that concrete integers are also within that range.
+  assert(x <= ((unsigned)UINT_MAX / 4));
+  return x;
+}
+
+void unsigned_concrete_int_no_crash() {
+  unsigned x = fu() + 1U, y = fu() + 1U;
+  clang_analyzer_denote(x - 1U, "$x");
+  clang_analyzer_denote(y - 1U, "$y");
+  clang_analyzer_express(y); // expected-warning {{$y}}
+  clang_analyzer_express(x == y); // expected-warning {{$x + 1U == $y + 1U}}
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -475,9 +475,6 @@
 SingleTy = ResultTy;
 if (LSym->getType() != SingleTy)
   return None;
-// Substracting unsigned integers is a nightmare.
-if (!SingleTy->isSignedIntegerOrEnumerationType())
-  return None;
   } else {
 // Don't rearrange other operations.
 return None;
@@ -485,6 +482,10 @@
 
   assert(!SingleTy.isNull() && "We should have figured out the type by now!");
 
+  // Rearrange signed symbolic expressions only
+  if (!SingleTy->isSignedIntegerOrEnumerationType())
+return None;
+
   SymbolRef RSym = Rhs.getAsSymbol();
   if (!RSym || RSym->getType() != SingleTy)
 return None;


Index: test/Analysis/svalbuilder-rearrange-comparisons.c
===
--- test/Analysis/svalbuilder-rearrange-comparisons.c
+++ test/Analysis/svalbuilder-rearrange-comparisons.c
@@ -979,3 +979,20 @@
   short a = x - 1U;
   return a - y;
 }
+
+unsigned gu();
+unsigned fu() {
+  unsigned x = gu();
+  // Assert that no overflows occur in this test file.
+  // Assuming that concrete integers are also within that range.
+  assert(x <= ((unsigned)UINT_MAX / 4));
+  return x;
+}
+
+void unsigned_concrete_int_no_crash() {
+  unsigned x = fu() + 1U, y = fu() + 1U;
+  clang_analyzer_denote(x - 1U, "$x");
+  clang_analyzer_denote(y - 1U, "$y");
+  clang_analyzer_express(y); // expected-warning {{$y}}
+  clang_analyzer_express(x == y); // expected-warning {{$x + 1U == $y + 1U}}
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -475,9 +475,6 @@
 SingleTy = ResultTy;
 if (LSym->getType() != SingleTy)
   return None;
-// Substracting unsigned integers is a nightmare.
-if (!SingleTy->isSignedIntegerOrEnumerationType())
-  return None;
   } else {
 // Don't rearrange other operations.
 return None;
@@ -485,6 +482,10 @@
 
   assert(!SingleTy.isNull() && "We should have figured out the type by now!");
 
+  // Rearrange signed symbolic expressions only
+  if (!SingleTy->isSignedIntegerOrEnumerationType())
+return None;
+
   SymbolRef RSym = Rhs.getAsSymbol();
   if (!RSym || RSym->getType() != SingleTy)
 return None;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 8 inline comments as done.
donat.nagy added a comment.

At the end of the documentation I stated that the check examines the code after 
macro expansion. Is this note clear enough?




Comment at: test/clang-tidy/bugprone-branch-clone.cpp:4
+void test_basic1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else 
branches [bugprone-branch-clone]

Eugene.Zelenko wrote:
> donat.nagy wrote:
> > JonasToth wrote:
> > > donat.nagy wrote:
> > > > JonasToth wrote:
> > > > > could you please add tests for ternary operators?
> > > > Currently the check does not handle ternary operators, but I added some 
> > > > checks reflecting this.
> > > Thank you. Could you please add a short note to the user-facing 
> > > documentation that these are not handled.
> > In fact, I could easily extend the functionality of the checker to cover 
> > ternary operators. Would it be an useful addition?
> Sure, it'll be great extension of functionality.
I added support for ternary operators and documented this fact.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757



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

[PATCH] D55062: [clangd] Partition include graph on auto-index.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/index/Background.cpp:184
+// We keep only the node "Path" and its edges.
+IncludeGraph getSubGraph(llvm::StringRef Path, const IncludeGraph &FullGraph) {
+  IncludeGraph IG;

We should make the function static or put it into the anonymous namespace.
While here the same should be done for `URIToFileCache`.



Comment at: clangd/index/Background.cpp:187
+
+  std::string FileURI = URI::create(Path).toString();
+  auto Entry = IG.try_emplace(FileURI).first;

NIT: Maybe accept a URI directly? E.g. if we ever have clients that already 
store a URI they won't need to do double conversions.



Comment at: clangd/index/Background.cpp:194
+  for (auto &Include : Node.DirectIncludes) {
+auto I = IG.try_emplace(Include).first;
+I->getValue().URI = I->getKey();

Maybe add a comment mentioning we do this merely to intern the strings?
This might be hard to follow on the first read.



Comment at: clangd/index/Background.cpp:384
 
   log("Indexed {0} ({1} symbols, {2} refs)", Inputs.CompileCommand.Filename,
+  Index.Symbols->size(), Index.Refs->numRefs());

Maybe also log the stats from the include graph?
It's probably less useful than symbols and refs count, but still...



Comment at: clangd/index/Background.cpp:387
+  SPAN_ATTACH(Tracer, "symbols", int(Index.Symbols->size()));
+  SPAN_ATTACH(Tracer, "refs", int(Index.Refs->numRefs()));
 

Same here, maybe also attach some stats from the include graph?



Comment at: unittests/clangd/BackgroundIndexTests.cpp:192
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());

This sets a compile command for a directory. Should it be `"root/A.cc"` or am I 
missing something?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55062



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

[PATCH] D55051: [Analyzer] [HOTFIX!] SValBuilder crash when `aggressive-binary-operation-simplification` enabled

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

baloghadamsoftware added a comment.

Committed because I was notified that the deadline for 7.0.1 is today.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55051



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

[PATCH] D55029: set default max-page-size to 4KB in lld for Android Aarch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D55029#1313120 , @ruiu wrote:

> LGTM. Please commit.
>
> Peter, I wonder if you are fine with the default 64KiB page size with lld, 
> especially given that lld always round up the text segment size to the 
> maximum page size on disk and fill the padding with trap instructions. On 
> average, that should increase the executable size by 32 KiB compared to other 
> linkers. I don't think that size is necessarily bad, because we are doing 
> that for a security purpose, but I wonder if people are OK with that.


I think the default is fine at 64KiB . Going back to 4KiB risks breaking 
programs that currently use default options on platforms that have chosen 64KiB 
which I think is a step too far. So far the concern about ELF file size has 
come from Android, where we have a separate target in clang where it is fairly 
easy to pass a 4KiB page size by default. There is a chance that this may 
change in the future as more general AArch64 linux platforms start being 
deployed on devices with limited storage. I guess at that point we could 
consider implementing common-page-size if it were a problem to pass 4KiB page 
size.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55029



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

[PATCH] D54796: [clangd] C++ API for emitting file status

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

> @ilya-biryukov, I hope the current patch is not too big for you to review, 
> happy to chat offline if you want (sam and I had a lot of discussions before 
> he is OOO).

Picking it up. Mostly NITs from my side and a few questions to better 
understand the design. Also happy to chat offline.




Comment at: clangd/ClangdServer.h:40
 
+// FIXME: find a better name.
 class DiagnosticsConsumer {

ClangdServerCallbacks? xD



Comment at: clangd/ClangdServer.h:49
+  /// Called whenever the file status is updated.
+  virtual void onFileUpdated(PathRef File, const TUStatus &Status){};
 };

Have we thought about the way we might expose statuses via the LSP?
The `TUStatus` seems a bit too clangd-specific (i.e. `BuildingPreamble`, 
`ReuseAST` is not something that makes sense in the protocol). Which might be 
fine on the `ClangdServer` layer, but it feels like we should generalize before 
sending it over via the LSP



Comment at: clangd/TUScheduler.cpp:209
+  /// Updates the TUStatus and emits it.
+  void EmitTUStatus(TUAction FAction,
+const TUStatus::BuildDetails *Detail = nullptr);

Naming: `emitTUStatus`?



Comment at: clangd/TUScheduler.cpp:242
+  /// Status of the TU.
+  TUStatus Status;
 

Maybe add a `GUARDED_BY(DiagMu)` comment?
Also maybe put closer to the mutex itself, e.g. right before ReportDiagnostics.



Comment at: clangd/TUScheduler.cpp:584
+Status.Action = std::move(Action);
+if (Details) {
+  Status.Details = *Details;

Code style: remove braces around a single-statement if branch



Comment at: clangd/TUScheduler.cpp:637
+  if (Requests.empty()) {
+EmitTUStatus({TUAction::Idle, /*Name*/ ""});
+  }

NIT: remove braces?



Comment at: clangd/TUScheduler.h:60
+BuildingPreamble, // The preamble of the TU is being built.
+BuildingFile, // The TU is being built.
+Idle, // Indicates the worker thread is idle, and ready to run any upcoming

What's the fundamental difference between `BuildingFile` and `RunningAction`?
We will often rebuild ASTs while running various actions that read the preamble.

Maybe we could squash those two together? One can view diagnostics as an action 
on the AST, similar to a direct LSP request like findReferences.



Comment at: clangd/TUScheduler.h:64
+  };
+  State S;
+  /// The name of the action currently running, e.g. Update, GoToDef, Hover.

Maybe set default value to avoid unexpected undefined behavior in case someone 
forget to initialize the field?



Comment at: clangd/TUScheduler.h:73
+  struct BuildDetails {
+/// clang fails to build the TU.
+bool buildFailed = false;

NIT: maybe clarify: `indicates whether clang failed...` or similar?



Comment at: clangd/TUScheduler.h:74
+/// clang fails to build the TU.
+bool buildFailed = false;
+/// indicates whether we reuse the prebuilt AST.

Naming: should this be `BuildFailed`?



Comment at: clangd/TUScheduler.h:75
+bool buildFailed = false;
+/// indicates whether we reuse the prebuilt AST.
+bool reuseAST = false;

NIT: should this be `reused`?



Comment at: clangd/TUScheduler.h:76
+/// indicates whether we reuse the prebuilt AST.
+bool reuseAST = false;
+  };

`ReuseAST`?



Comment at: unittests/clangd/TUSchedulerTests.cpp:723
+// We wait long enough that the document gets removed.
+std::this_thread::sleep_for(std::chrono::milliseconds(50));
+  }

It seems we could make this test deterministic if we:
1. create a `Notification`
2. make it ready after calling `removeDocument`.
3. wait for it to become ready at the point where we have `sleep_for` now.



Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54796



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

[clang-tools-extra] r347982 - [clangd] Drop injected class name when class scope is not explicitly specified.

[Eric Liu via cfe-commits]

Author: ioeric
Date: Fri Nov 30 03:12:40 2018
New Revision: 347982

URL: http://llvm.org/viewvc/llvm-project?rev=347982&view=rev
Log:
[clangd] Drop injected class name when class scope is not explicitly specified.

Summary: E.g. allow injected "A::A" in `using A::A^` but not in "A^".

Reviewers: kadircet

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

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

Modified:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=347982&r1=347981&r2=347982&view=diff
==
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Fri Nov 30 03:12:40 2018
@@ -656,6 +656,13 @@ bool contextAllowsIndex(enum CodeComplet
   llvm_unreachable("unknown code completion context");
 }
 
+static bool isInjectedClass(const NamedDecl &D) {
+  if (auto *R = dyn_cast_or_null(&D))
+if (R->isInjectedClassName())
+  return true;
+  return false;
+}
+
 // Some member calls are blacklisted because they're so rarely useful.
 static bool isBlacklistedMember(const NamedDecl &D) {
   // Destructor completion is rarely useful, and works inconsistently.
@@ -663,9 +670,8 @@ static bool isBlacklistedMember(const Na
   if (D.getKind() == Decl::CXXDestructor)
 return true;
   // Injected name may be useful for A::foo(), but who writes A::A::foo()?
-  if (auto *R = dyn_cast_or_null(&D))
-if (R->isInjectedClassName())
-  return true;
+  if (isInjectedClass(D))
+return true;
   // Explicit calls to operators are also rare.
   auto NameKind = D.getDeclName().getNameKind();
   if (NameKind == DeclarationName::CXXOperatorName ||
@@ -744,6 +750,11 @@ struct CompletionRecorder : public CodeC
   !Context.getBaseType().isNull() // is this a member-access context?
   && isBlacklistedMember(*Result.Declaration))
 continue;
+  // Skip injected class name when no class scope is not explicitly set.
+  // E.g. show injected A::A in `using A::A^` but not in "A^".
+  if (Result.Declaration && !Context.getCXXScopeSpecifier().hasValue() &&
+  isInjectedClass(*Result.Declaration))
+continue;
   // We choose to never append '::' to completion results in clangd.
   Result.StartsNestedNameSpecifier = false;
   Results.push_back(Result);

Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=347982&r1=347981&r2=347982&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Fri Nov 30 
03:12:40 2018
@@ -416,6 +416,11 @@ TEST(CompletionTest, InjectedTypename) {
   Has("X"));
 }
 
+TEST(CompletionTest, SkipInjectedWhenUnqualified) {
+  EXPECT_THAT(completions("struct X { void f() { X^ }};").Completions,
+  ElementsAre(Named("X"), Named("~X")));
+}
+
 TEST(CompletionTest, Snippets) {
   clangd::CodeCompleteOptions Opts;
   auto Results = completions(


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

[PATCH] D55065: [clangd] Drop injected class name when class scope is not explicitly specified.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL347982: [clangd] Drop injected class name when class scope 
is not explicitly specified. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55065

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


Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -416,6 +416,11 @@
   Has("X"));
 }
 
+TEST(CompletionTest, SkipInjectedWhenUnqualified) {
+  EXPECT_THAT(completions("struct X { void f() { X^ }};").Completions,
+  ElementsAre(Named("X"), Named("~X")));
+}
+
 TEST(CompletionTest, Snippets) {
   clangd::CodeCompleteOptions Opts;
   auto Results = completions(
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -656,6 +656,13 @@
   llvm_unreachable("unknown code completion context");
 }
 
+static bool isInjectedClass(const NamedDecl &D) {
+  if (auto *R = dyn_cast_or_null(&D))
+if (R->isInjectedClassName())
+  return true;
+  return false;
+}
+
 // Some member calls are blacklisted because they're so rarely useful.
 static bool isBlacklistedMember(const NamedDecl &D) {
   // Destructor completion is rarely useful, and works inconsistently.
@@ -663,9 +670,8 @@
   if (D.getKind() == Decl::CXXDestructor)
 return true;
   // Injected name may be useful for A::foo(), but who writes A::A::foo()?
-  if (auto *R = dyn_cast_or_null(&D))
-if (R->isInjectedClassName())
-  return true;
+  if (isInjectedClass(D))
+return true;
   // Explicit calls to operators are also rare.
   auto NameKind = D.getDeclName().getNameKind();
   if (NameKind == DeclarationName::CXXOperatorName ||
@@ -744,6 +750,11 @@
   !Context.getBaseType().isNull() // is this a member-access context?
   && isBlacklistedMember(*Result.Declaration))
 continue;
+  // Skip injected class name when no class scope is not explicitly set.
+  // E.g. show injected A::A in `using A::A^` but not in "A^".
+  if (Result.Declaration && !Context.getCXXScopeSpecifier().hasValue() &&
+  isInjectedClass(*Result.Declaration))
+continue;
   // We choose to never append '::' to completion results in clangd.
   Result.StartsNestedNameSpecifier = false;
   Results.push_back(Result);


Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -416,6 +416,11 @@
   Has("X"));
 }
 
+TEST(CompletionTest, SkipInjectedWhenUnqualified) {
+  EXPECT_THAT(completions("struct X { void f() { X^ }};").Completions,
+  ElementsAre(Named("X"), Named("~X")));
+}
+
 TEST(CompletionTest, Snippets) {
   clangd::CodeCompleteOptions Opts;
   auto Results = completions(
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -656,6 +656,13 @@
   llvm_unreachable("unknown code completion context");
 }
 
+static bool isInjectedClass(const NamedDecl &D) {
+  if (auto *R = dyn_cast_or_null(&D))
+if (R->isInjectedClassName())
+  return true;
+  return false;
+}
+
 // Some member calls are blacklisted because they're so rarely useful.
 static bool isBlacklistedMember(const NamedDecl &D) {
   // Destructor completion is rarely useful, and works inconsistently.
@@ -663,9 +670,8 @@
   if (D.getKind() == Decl::CXXDestructor)
 return true;
   // Injected name may be useful for A::foo(), but who writes A::A::foo()?
-  if (auto *R = dyn_cast_or_null(&D))
-if (R->isInjectedClassName())
-  return true;
+  if (isInjectedClass(D))
+return true;
   // Explicit calls to operators are also rare.
   auto NameKind = D.getDeclName().getNameKind();
   if (NameKind == DeclarationName::CXXOperatorName ||
@@ -744,6 +750,11 @@
   !Context.getBaseType().isNull() // is this a member-access context?
   && isBlacklistedMember(*Result.Declaration))
 continue;
+  // Skip injected class name when no class scope is not explicitly set.
+  // E.g. show injected A::A in `using A::A^` but not in "A^".
+  if (Result.Declaration && !Context.getCXXScopeSpecifier().hasValue(

[PATCH] D55061: [clangd] Penalize destructor and overloaded operators in code completion.

[Eric Liu via Phabricator via cfe-commits]

ioeric updated this revision to Diff 176075.
ioeric marked an inline comment as done.
ioeric added a comment.

- Rename test name.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55061

Files:
  clangd/Quality.cpp
  clangd/Quality.h
  unittests/clangd/QualityTests.cpp

Index: unittests/clangd/QualityTests.cpp
===
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -205,16 +205,22 @@
   EXPECT_GT(WithReferences.evaluate(), Default.evaluate());
   EXPECT_GT(ManyReferences.evaluate(), WithReferences.evaluate());
 
-  SymbolQualitySignals Keyword, Variable, Macro, Constructor, Function;
+  SymbolQualitySignals Keyword, Variable, Macro, Constructor, Function,
+  Destructor, Operator;
   Keyword.Category = SymbolQualitySignals::Keyword;
   Variable.Category = SymbolQualitySignals::Variable;
   Macro.Category = SymbolQualitySignals::Macro;
   Constructor.Category = SymbolQualitySignals::Constructor;
+  Destructor.Category = SymbolQualitySignals::Destructor;
+  Destructor.Category = SymbolQualitySignals::Destructor;
+  Operator.Category = SymbolQualitySignals::Operator;
   Function.Category = SymbolQualitySignals::Function;
   EXPECT_GT(Variable.evaluate(), Default.evaluate());
   EXPECT_GT(Keyword.evaluate(), Variable.evaluate());
   EXPECT_LT(Macro.evaluate(), Default.evaluate());
+  EXPECT_LT(Operator.evaluate(), Default.evaluate());
   EXPECT_LT(Constructor.evaluate(), Function.evaluate());
+  EXPECT_LT(Destructor.evaluate(), Constructor.evaluate());
 }
 
 TEST(QualityTests, SymbolRelevanceSignalsSanity) {
@@ -385,11 +391,12 @@
   EXPECT_TRUE(Rel.IsInstanceMember);
 }
 
-TEST(QualityTests, ConstructorQuality) {
+TEST(QualityTests, ConstructorDestructor) {
   auto Header = TestTU::withHeaderCode(R"cpp(
 class Foo {
 public:
   Foo(int);
+  ~Foo();
 };
   )cpp");
   auto Symbols = Header.headerSymbols();
@@ -399,15 +406,43 @@
 return (ND.getQualifiedNameAsString() == "Foo::Foo") &&
isa(&ND);
   });
+  const NamedDecl *DtorDecl = &findDecl(AST, [](const NamedDecl &ND) {
+return (ND.getQualifiedNameAsString() == "Foo::~Foo") &&
+   isa(&ND);
+  });
 
-  SymbolQualitySignals Q;
-  Q.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
-  EXPECT_EQ(Q.Category, SymbolQualitySignals::Constructor);
+  SymbolQualitySignals CtorQ;
+  CtorQ.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
+  EXPECT_EQ(CtorQ.Category, SymbolQualitySignals::Constructor);
 
-  Q.Category = SymbolQualitySignals::Unknown;
+  CtorQ.Category = SymbolQualitySignals::Unknown;
   const Symbol &CtorSym = findSymbol(Symbols, "Foo::Foo");
-  Q.merge(CtorSym);
-  EXPECT_EQ(Q.Category, SymbolQualitySignals::Constructor);
+  CtorQ.merge(CtorSym);
+  EXPECT_EQ(CtorQ.Category, SymbolQualitySignals::Constructor);
+
+  SymbolQualitySignals DtorQ;
+  DtorQ.merge(CodeCompletionResult(DtorDecl, /*Priority=*/0));
+  EXPECT_EQ(DtorQ.Category, SymbolQualitySignals::Destructor);
+}
+
+TEST(QualityTests, Operator) {
+  auto Header = TestTU::withHeaderCode(R"cpp(
+class Foo {
+public:
+  bool operator<(const Foo& f1, const Foo& f2);
+};
+  )cpp");
+  auto AST = Header.build();
+
+  const NamedDecl *Operator = &findDecl(AST, [](const NamedDecl &ND) {
+if (const auto *OD = dyn_cast(&ND))
+  if (OD->isOverloadedOperator())
+return true;
+return false;
+  });
+  SymbolQualitySignals Q;
+  Q.merge(CodeCompletionResult(Operator, /*Priority=*/0));
+  EXPECT_EQ(Q.Category, SymbolQualitySignals::Operator);
 }
 
 TEST(QualityTests, ItemWithFixItsRankedDown) {
Index: clangd/Quality.h
===
--- clangd/Quality.h
+++ clangd/Quality.h
@@ -68,8 +68,10 @@
 Type,
 Function,
 Constructor,
+Destructor,
 Namespace,
 Keyword,
+Operator,
   } Category = Unknown;
 
   void merge(const CodeCompletionResult &SemaCCResult);
Index: clangd/Quality.cpp
===
--- clangd/Quality.cpp
+++ clangd/Quality.cpp
@@ -62,6 +62,10 @@
 }
 
 static SymbolQualitySignals::SymbolCategory categorize(const NamedDecl &ND) {
+  if (const auto *FD = dyn_cast(&ND)) {
+if (FD->isOverloadedOperator())
+  return SymbolQualitySignals::Operator;
+  }
   class Switch
   : public ConstDeclVisitor {
   public:
@@ -75,6 +79,7 @@
 MAP(TypeAliasTemplateDecl, Type);
 MAP(ClassTemplateDecl, Type);
 MAP(CXXConstructorDecl, Constructor);
+MAP(CXXDestructorDecl, Destructor);
 MAP(ValueDecl, Variable);
 MAP(VarTemplateDecl, Variable);
 MAP(FunctionDecl, Function);
@@ -134,9 +139,10 @@
   case index::SymbolKind::InstanceProperty:
   case index::SymbolKind::ClassProperty:
   case index::SymbolKind::StaticProperty:
-  case index::SymbolKind::Destructor:
   case index::SymbolKind::Conv

[PATCH] D55061: [clangd] Penalize destructor and overloaded operators in code completion.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.



Comment at: clangd/Quality.h:74
 Keyword,
+Operator,
   } Category = Unknown;

hokein wrote:
> Maybe name it `OverloadedOperator` to avoid confusion with other 
> non-overloaded operators like `?:`
Ternary operator is not a symbol AFAIK?  It's an expression. Even if it's an 
operator, I think we would still want to categorize it as operator. 


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55061



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

[clang-tools-extra] r347983 - [clangd] Penalize destructor and overloaded operators in code completion.

[Eric Liu via cfe-commits]

Author: ioeric
Date: Fri Nov 30 03:17:15 2018
New Revision: 347983

URL: http://llvm.org/viewvc/llvm-project?rev=347983&view=rev
Log:
[clangd] Penalize destructor and overloaded operators in code completion.

Reviewers: hokein

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

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

Modified:
clang-tools-extra/trunk/clangd/Quality.cpp
clang-tools-extra/trunk/clangd/Quality.h
clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp

Modified: clang-tools-extra/trunk/clangd/Quality.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Quality.cpp?rev=347983&r1=347982&r2=347983&view=diff
==
--- clang-tools-extra/trunk/clangd/Quality.cpp (original)
+++ clang-tools-extra/trunk/clangd/Quality.cpp Fri Nov 30 03:17:15 2018
@@ -62,6 +62,10 @@ static bool hasUsingDeclInMainFile(const
 }
 
 static SymbolQualitySignals::SymbolCategory categorize(const NamedDecl &ND) {
+  if (const auto *FD = dyn_cast(&ND)) {
+if (FD->isOverloadedOperator())
+  return SymbolQualitySignals::Operator;
+  }
   class Switch
   : public ConstDeclVisitor {
   public:
@@ -75,6 +79,7 @@ static SymbolQualitySignals::SymbolCateg
 MAP(TypeAliasTemplateDecl, Type);
 MAP(ClassTemplateDecl, Type);
 MAP(CXXConstructorDecl, Constructor);
+MAP(CXXDestructorDecl, Destructor);
 MAP(ValueDecl, Variable);
 MAP(VarTemplateDecl, Variable);
 MAP(FunctionDecl, Function);
@@ -134,9 +139,10 @@ categorize(const index::SymbolInfo &D) {
   case index::SymbolKind::InstanceProperty:
   case index::SymbolKind::ClassProperty:
   case index::SymbolKind::StaticProperty:
-  case index::SymbolKind::Destructor:
   case index::SymbolKind::ConversionFunction:
 return SymbolQualitySignals::Function;
+  case index::SymbolKind::Destructor:
+return SymbolQualitySignals::Destructor;
   case index::SymbolKind::Constructor:
 return SymbolQualitySignals::Constructor;
   case index::SymbolKind::Variable:
@@ -231,10 +237,12 @@ float SymbolQualitySignals::evaluate() c
 Score *= 0.8f;
 break;
   case Macro:
+  case Destructor:
+  case Operator:
 Score *= 0.5f;
 break;
-  case Unknown:
   case Constructor: // No boost constructors so they are after class types.
+  case Unknown:
 break;
   }
 

Modified: clang-tools-extra/trunk/clangd/Quality.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Quality.h?rev=347983&r1=347982&r2=347983&view=diff
==
--- clang-tools-extra/trunk/clangd/Quality.h (original)
+++ clang-tools-extra/trunk/clangd/Quality.h Fri Nov 30 03:17:15 2018
@@ -68,8 +68,10 @@ struct SymbolQualitySignals {
 Type,
 Function,
 Constructor,
+Destructor,
 Namespace,
 Keyword,
+Operator,
   } Category = Unknown;
 
   void merge(const CodeCompletionResult &SemaCCResult);

Modified: clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp?rev=347983&r1=347982&r2=347983&view=diff
==
--- clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp Fri Nov 30 
03:17:15 2018
@@ -205,16 +205,22 @@ TEST(QualityTests, SymbolQualitySignalsS
   EXPECT_GT(WithReferences.evaluate(), Default.evaluate());
   EXPECT_GT(ManyReferences.evaluate(), WithReferences.evaluate());
 
-  SymbolQualitySignals Keyword, Variable, Macro, Constructor, Function;
+  SymbolQualitySignals Keyword, Variable, Macro, Constructor, Function,
+  Destructor, Operator;
   Keyword.Category = SymbolQualitySignals::Keyword;
   Variable.Category = SymbolQualitySignals::Variable;
   Macro.Category = SymbolQualitySignals::Macro;
   Constructor.Category = SymbolQualitySignals::Constructor;
+  Destructor.Category = SymbolQualitySignals::Destructor;
+  Destructor.Category = SymbolQualitySignals::Destructor;
+  Operator.Category = SymbolQualitySignals::Operator;
   Function.Category = SymbolQualitySignals::Function;
   EXPECT_GT(Variable.evaluate(), Default.evaluate());
   EXPECT_GT(Keyword.evaluate(), Variable.evaluate());
   EXPECT_LT(Macro.evaluate(), Default.evaluate());
+  EXPECT_LT(Operator.evaluate(), Default.evaluate());
   EXPECT_LT(Constructor.evaluate(), Function.evaluate());
+  EXPECT_LT(Destructor.evaluate(), Constructor.evaluate());
 }
 
 TEST(QualityTests, SymbolRelevanceSignalsSanity) {
@@ -385,11 +391,12 @@ TEST(QualityTests, IsInstanceMember) {
   EXPECT_TRUE(Rel.IsInstanceMember);
 }
 
-TEST(QualityTests, ConstructorQuality) {
+TEST(QualityTests, ConstructorDestructor) {
   auto Header = TestTU::withHeaderCode(R"cpp(
 class Foo {
 public:
   Foo(int);
+  ~Foo();
 };
 

[PATCH] D55061: [clangd] Penalize destructor and overloaded operators in code completion.

[Eric Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE347983: [clangd] Penalize destructor and overloaded 
operators in code completion. (authored by ioeric, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55061?vs=176075&id=176076#toc

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55061

Files:
  clangd/Quality.cpp
  clangd/Quality.h
  unittests/clangd/QualityTests.cpp

Index: unittests/clangd/QualityTests.cpp
===
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -205,16 +205,22 @@
   EXPECT_GT(WithReferences.evaluate(), Default.evaluate());
   EXPECT_GT(ManyReferences.evaluate(), WithReferences.evaluate());
 
-  SymbolQualitySignals Keyword, Variable, Macro, Constructor, Function;
+  SymbolQualitySignals Keyword, Variable, Macro, Constructor, Function,
+  Destructor, Operator;
   Keyword.Category = SymbolQualitySignals::Keyword;
   Variable.Category = SymbolQualitySignals::Variable;
   Macro.Category = SymbolQualitySignals::Macro;
   Constructor.Category = SymbolQualitySignals::Constructor;
+  Destructor.Category = SymbolQualitySignals::Destructor;
+  Destructor.Category = SymbolQualitySignals::Destructor;
+  Operator.Category = SymbolQualitySignals::Operator;
   Function.Category = SymbolQualitySignals::Function;
   EXPECT_GT(Variable.evaluate(), Default.evaluate());
   EXPECT_GT(Keyword.evaluate(), Variable.evaluate());
   EXPECT_LT(Macro.evaluate(), Default.evaluate());
+  EXPECT_LT(Operator.evaluate(), Default.evaluate());
   EXPECT_LT(Constructor.evaluate(), Function.evaluate());
+  EXPECT_LT(Destructor.evaluate(), Constructor.evaluate());
 }
 
 TEST(QualityTests, SymbolRelevanceSignalsSanity) {
@@ -385,11 +391,12 @@
   EXPECT_TRUE(Rel.IsInstanceMember);
 }
 
-TEST(QualityTests, ConstructorQuality) {
+TEST(QualityTests, ConstructorDestructor) {
   auto Header = TestTU::withHeaderCode(R"cpp(
 class Foo {
 public:
   Foo(int);
+  ~Foo();
 };
   )cpp");
   auto Symbols = Header.headerSymbols();
@@ -399,15 +406,43 @@
 return (ND.getQualifiedNameAsString() == "Foo::Foo") &&
isa(&ND);
   });
+  const NamedDecl *DtorDecl = &findDecl(AST, [](const NamedDecl &ND) {
+return (ND.getQualifiedNameAsString() == "Foo::~Foo") &&
+   isa(&ND);
+  });
 
-  SymbolQualitySignals Q;
-  Q.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
-  EXPECT_EQ(Q.Category, SymbolQualitySignals::Constructor);
+  SymbolQualitySignals CtorQ;
+  CtorQ.merge(CodeCompletionResult(CtorDecl, /*Priority=*/0));
+  EXPECT_EQ(CtorQ.Category, SymbolQualitySignals::Constructor);
 
-  Q.Category = SymbolQualitySignals::Unknown;
+  CtorQ.Category = SymbolQualitySignals::Unknown;
   const Symbol &CtorSym = findSymbol(Symbols, "Foo::Foo");
-  Q.merge(CtorSym);
-  EXPECT_EQ(Q.Category, SymbolQualitySignals::Constructor);
+  CtorQ.merge(CtorSym);
+  EXPECT_EQ(CtorQ.Category, SymbolQualitySignals::Constructor);
+
+  SymbolQualitySignals DtorQ;
+  DtorQ.merge(CodeCompletionResult(DtorDecl, /*Priority=*/0));
+  EXPECT_EQ(DtorQ.Category, SymbolQualitySignals::Destructor);
+}
+
+TEST(QualityTests, Operator) {
+  auto Header = TestTU::withHeaderCode(R"cpp(
+class Foo {
+public:
+  bool operator<(const Foo& f1, const Foo& f2);
+};
+  )cpp");
+  auto AST = Header.build();
+
+  const NamedDecl *Operator = &findDecl(AST, [](const NamedDecl &ND) {
+if (const auto *OD = dyn_cast(&ND))
+  if (OD->isOverloadedOperator())
+return true;
+return false;
+  });
+  SymbolQualitySignals Q;
+  Q.merge(CodeCompletionResult(Operator, /*Priority=*/0));
+  EXPECT_EQ(Q.Category, SymbolQualitySignals::Operator);
 }
 
 TEST(QualityTests, ItemWithFixItsRankedDown) {
Index: clangd/Quality.h
===
--- clangd/Quality.h
+++ clangd/Quality.h
@@ -68,8 +68,10 @@
 Type,
 Function,
 Constructor,
+Destructor,
 Namespace,
 Keyword,
+Operator,
   } Category = Unknown;
 
   void merge(const CodeCompletionResult &SemaCCResult);
Index: clangd/Quality.cpp
===
--- clangd/Quality.cpp
+++ clangd/Quality.cpp
@@ -62,6 +62,10 @@
 }
 
 static SymbolQualitySignals::SymbolCategory categorize(const NamedDecl &ND) {
+  if (const auto *FD = dyn_cast(&ND)) {
+if (FD->isOverloadedOperator())
+  return SymbolQualitySignals::Operator;
+  }
   class Switch
   : public ConstDeclVisitor {
   public:
@@ -75,6 +79,7 @@
 MAP(TypeAliasTemplateDecl, Type);
 MAP(ClassTemplateDecl, Type);
 MAP(CXXConstructorDecl, Constructor);
+MAP(CXXDestructorDecl, Destructor);
 MAP(ValueDecl, Variable);
 MAP(VarTemplateDecl, Variable);
 MAP(FunctionDecl, Function);
@@ -134,9 +139,10 @@
   case index::SymbolKind::Instan

[PATCH] D55054: [clang] Fill RealPathName for virtual files.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 176078.
kadircet marked 5 inline comments as done.
kadircet added a comment.

Address comments


Repository:
  rC Clang

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

https://reviews.llvm.org/D55054

Files:
  include/clang/Basic/FileManager.h
  lib/Basic/FileManager.cpp
  unittests/Basic/FileManagerTest.cpp

Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -235,22 +235,18 @@
   ASSERT_TRUE(file != nullptr);
   ASSERT_TRUE(file->isValid());
   // "real path name" reveals whether the file was actually opened.
-  EXPECT_EQ("", file->tryGetRealPathName());
+  EXPECT_FALSE(file->isOpenForTests());
 
   file = manager.getFile("/tmp/test", /*OpenFile=*/true);
   ASSERT_TRUE(file != nullptr);
   ASSERT_TRUE(file->isValid());
-#ifdef _WIN32
-  EXPECT_EQ("/tmp\\test", file->tryGetRealPathName());
-#else
-  EXPECT_EQ("/tmp/test", file->tryGetRealPathName());
-#endif
+  EXPECT_TRUE(file->isOpenForTests());
 
   // However we should never try to open a file previously opened as virtual.
   ASSERT_TRUE(manager.getVirtualFile("/tmp/testv", 5, 0));
   ASSERT_TRUE(manager.getFile("/tmp/testv", /*OpenFile=*/false));
   file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
-  EXPECT_EQ("", file->tryGetRealPathName());
+  EXPECT_FALSE(file->isOpenForTests());
 }
 
 // The following tests apply to Unix-like system only.
@@ -353,4 +349,19 @@
   EXPECT_EQ(Path, ExpectedResult);
 }
 
+// getVirtualFile should always fill the real path.
+TEST_F(FileManagerTest, getVirtualFileFillsRealPathName) {
+  // Inject fake files into the file system.
+  auto statCache = llvm::make_unique();
+  statCache->InjectDirectory("/tmp", 42);
+  statCache->InjectFile("/tmp/test", 43);
+  manager.addStatCache(std::move(statCache));
+
+  // Check for real path.
+  const FileEntry *file = manager.getVirtualFile("/tmp/test", 123, 1);
+  ASSERT_TRUE(file != nullptr);
+  ASSERT_TRUE(file->isValid());
+  EXPECT_EQ(file->tryGetRealPathName(), "/tmp/test");
+}
+
 } // anonymous namespace
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -293,16 +293,8 @@
   // If we opened the file for the first time, record the resulting info.
   // Do this even if the cache entry was valid, maybe we didn't previously open.
   if (F && !UFE.File) {
-if (auto PathName = F->getName()) {
-  llvm::SmallString<128> AbsPath(*PathName);
-  // This is not the same as `VFS::getRealPath()`, which resolves symlinks
-  // but can be very expensive on real file systems.
-  // FIXME: the semantic of RealPathName is unclear, and the name might be
-  // misleading. We need to clean up the interface here.
-  makeAbsolutePath(AbsPath);
-  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
-  UFE.RealPathName = AbsPath.str();
-}
+if (auto PathName = F->getName())
+  fillRealPathName(&UFE, *PathName);
 UFE.File = std::move(F);
 assert(!UFE.DeferredOpen && "we just opened it!");
   }
@@ -395,6 +387,7 @@
 UFE->UniqueID = Data.UniqueID;
 UFE->IsNamedPipe = Data.IsNamedPipe;
 UFE->InPCH = Data.InPCH;
+fillRealPathName(UFE, Data.Name);
   }
 
   if (!UFE) {
@@ -438,6 +431,18 @@
   return Changed;
 }
 
+bool FileManager::fillRealPathName(FileEntry *UFE, llvm::StringRef FileName) {
+  llvm::SmallString<128> AbsPath(FileName);
+  // This is not the same as `VFS::getRealPath()`, which resolves symlinks
+  // but can be very expensive on real file systems.
+  // FIXME: the semantic of RealPathName is unclear, and the name might be
+  // misleading. We need to clean up the interface here.
+  bool Changed = makeAbsolutePath(AbsPath);
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE->RealPathName = AbsPath.str();
+  return Changed;
+}
+
 llvm::ErrorOr>
 FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile,
   bool ShouldCloseOpenFile) {
Index: include/clang/Basic/FileManager.h
===
--- include/clang/Basic/FileManager.h
+++ include/clang/Basic/FileManager.h
@@ -104,6 +104,10 @@
   void closeFile() const {
 File.reset(); // rely on destructor to close File
   }
+
+  // Only for use in tests to see if deferred opens are happening, arther than
+  // relying on RealPathName being empty.
+  bool isOpenForTests() const { return File != nullptr; }
 };
 
 struct FileData;
@@ -173,6 +177,10 @@
   /// or a directory) as virtual directories.
   void addAncestorsAsVirtualDirs(StringRef Path);
 
+  /// Fills the RealPathName entry, returns true if FileName is the same as
+  /// RealPathName.
+  bool fillRealPathName(FileEntry *UFE, llvm::StringRef FileName);
+
 public:
   FileManager(cons

[PATCH] D55023: OpenCL: Improve vector printf warnings

[Anastasia Stulova via Phabricator via cfe-commits]

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

LGTM! Thanks!




Comment at: test/SemaOpenCL/printf-format-strings.cl:65
+{
+printf("%v4f\n", arg); // expected-warning {{format specifies type 'double 
__attribute__((ext_vector_type(4)))' but the argument has type 'float2' (vector 
of 2 'float' values)}}
 }

arsenm wrote:
> Anastasia wrote:
> > arsenm wrote:
> > > arsenm wrote:
> > > > arsenm wrote:
> > > > > Anastasia wrote:
> > > > > > So there is no way to print vector of float? What will happen on 
> > > > > > architectures that don't support doubles?
> > > > > > 
> > > > > > I guess it's the same for printf in general with the float type?
> > > > > There is, it's converted to a vector of doubles. This case warns 
> > > > > because the element count mismatches
> > > > The conversion is to float if doubles aren't supported
> > > The warning message is wrong though without doubles, and still says 
> > > double.
> > Can we add this explicitly to the test just for a record? I guess the 
> > warning message can't be changed to print the type correctly?
> > 
> > Also would it make sense to test other builtin types: double, char...?
> This should be easy to fix, but the way to test for doubles is enabled is 
> making this unnecessarily difficult. It requires threading OpenCLOptions 
> through all of the relevant functions to check for cl_khr_fp64. Why is this 
> separate from LangOptions?
I see. I think the problem is that it is set conditionally based on the targets 
in earlier CL versions. So it had to be a separated into target specific 
option. :(


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

https://reviews.llvm.org/D55023



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

[PATCH] D54947: [OpenCL][CodeGen] Fix replacing memcpy with addrspacecast

[Dmitry Sidorov via Phabricator via cfe-commits]

sidorovd updated this revision to Diff 176085.
sidorovd marked an inline comment as done.
sidorovd added a comment.

Removed redundant lines in a comment


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

https://reviews.llvm.org/D54947

Files:
  lib/CodeGen/CGCall.cpp
  test/CodeGenOpenCL/addr-space-struct-arg.cl


Index: test/CodeGenOpenCL/addr-space-struct-arg.cl
===
--- test/CodeGenOpenCL/addr-space-struct-arg.cl
+++ test/CodeGenOpenCL/addr-space-struct-arg.cl
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -finclude-default-header 
-ffake-address-space-map -triple i686-pc-darwin | FileCheck -enable-var-scope 
-check-prefixes=COM,X86 %s
-// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -finclude-default-header -triple 
amdgcn | FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN %s
-// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL2.0 -O0 
-finclude-default-header -triple amdgcn | FileCheck -enable-var-scope 
-check-prefixes=COM,AMDGCN,AMDGCN20 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -ffake-address-space-map -triple 
i686-pc-darwin | FileCheck -enable-var-scope -check-prefixes=COM,X86 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -triple amdgcn | FileCheck 
-enable-var-scope -check-prefixes=COM,AMDGCN %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL2.0 -O0 -triple amdgcn | 
FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN,AMDGCN20 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL1.2 -O0 -triple 
spir-unknown-unknown-unknown | FileCheck -enable-var-scope -check-prefixes=SPIR 
%s
+
+typedef int int2 __attribute__((ext_vector_type(2)));
 
 typedef struct {
   int cells[9];
@@ -130,6 +133,12 @@
   FuncOneMember(u);
 }
 
+// SPIR: call void @llvm.memcpy.p0i8.p1i8.i32
+// SPIR-NOT: addrspacecast
+kernel void KernelOneMemberSpir(global struct StructOneMember* u) {
+  FuncOneMember(*u);
+}
+
 // AMDGCN-LABEL: define amdgpu_kernel void @KernelLargeOneMember(
 // AMDGCN:  %[[U:.*]] = alloca %struct.LargeStructOneMember, align 8, 
addrspace(5)
 // AMDGCN:  store %struct.LargeStructOneMember %u.coerce, 
%struct.LargeStructOneMember addrspace(5)* %[[U]], align 8
Index: lib/CodeGen/CGCall.cpp
===
--- lib/CodeGen/CGCall.cpp
+++ lib/CodeGen/CGCall.cpp
@@ -3954,15 +3954,28 @@
 } else if (I->hasLValue()) {
   auto LV = I->getKnownLValue();
   auto AS = LV.getAddressSpace();
+
   if ((!ArgInfo.getIndirectByVal() &&
(LV.getAlignment() >=
-getContext().getTypeAlignInChars(I->Ty))) ||
-  (ArgInfo.getIndirectByVal() &&
-   ((AS != LangAS::Default && AS != LangAS::opencl_private &&
- AS != CGM.getASTAllocaAddressSpace() {
+getContext().getTypeAlignInChars(I->Ty {
+NeedCopy = true;
+  }
+  if (!getLangOpts().OpenCL) {
+if ((ArgInfo.getIndirectByVal() &&
+(AS != LangAS::Default &&
+ AS != CGM.getASTAllocaAddressSpace( {
+  NeedCopy = true;
+}
+  }
+  // For OpenCL even if RV is located in default or alloca address 
space
+  // we don't want to perform address space cast for it.
+  else if ((ArgInfo.getIndirectByVal() &&
+Addr.getType()->getAddressSpace() != IRFuncTy->
+  getParamType(FirstIRArg)->getPointerAddressSpace())) {
 NeedCopy = true;
   }
 }
+
 if (NeedCopy) {
   // Create an aligned temporary, and copy to it.
   Address AI = CreateMemTempWithoutCast(


Index: test/CodeGenOpenCL/addr-space-struct-arg.cl
===
--- test/CodeGenOpenCL/addr-space-struct-arg.cl
+++ test/CodeGenOpenCL/addr-space-struct-arg.cl
@@ -1,6 +1,9 @@
-// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -finclude-default-header -ffake-address-space-map -triple i686-pc-darwin | FileCheck -enable-var-scope -check-prefixes=COM,X86 %s
-// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -finclude-default-header -triple amdgcn | FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN %s
-// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL2.0 -O0 -finclude-default-header -triple amdgcn | FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN,AMDGCN20 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -ffake-address-space-map -triple i686-pc-darwin | FileCheck -enable-var-scope -check-prefixes=COM,X86 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -O0 -triple amdgcn | FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL2.0 -O0 -triple amdgcn | FileCheck -enable-var-scope -check-prefixes=COM,AMDGCN,AMDGCN20 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -cl-std=CL1.2 -O0 -triple spir-unknown-unknown-unknown | FileCheck -enable-var-scope -check-prefixes=SPIR %s
+
+typedef

[PATCH] D54947: [OpenCL][CodeGen] Fix replacing memcpy with addrspacecast

[Dmitry Sidorov via Phabricator via cfe-commits]

sidorovd added inline comments.



Comment at: lib/CodeGen/CGCall.cpp:3972
+  // we don't want to perform address space cast for it, since that
+  // leads to casting __private * (default addr space in OpenCL) to
+  // __global * which is not valid. Create memcpy call instead.

Anastasia wrote:
> sidorovd wrote:
> > Anastasia wrote:
> > > This statement is incorrect. Private is only default AS in CL versions 
> > > before 2.0.
> > > 
> > > I feel this can be expressed simpler. I guess the observation here is 
> > > that if addr space mismatches it has to generate a copy because even if 
> > > it would be a generic we can't get what the original specific addr space 
> > > was? So it's safe to generate a copy.
> > > This statement is incorrect. Private is only default AS in CL versions 
> > > before 2.0.
> > You are right, thanks!
> > 
> > Still I want to leave the comment almost as is, since it creates a link 
> > with a previous one on lines 3928-3935.
> I feel it doesn't add much value, but rather exactly says what code does. At 
> least this is definitely obvious from this code:
>   Create memcpy call if RV is located in an address space different than that 
> of
Agree


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

https://reviews.llvm.org/D54947



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

[PATCH] D55054: [clang] Fill RealPathName for virtual files.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: unittests/Basic/FileManagerTest.cpp:237
   ASSERT_TRUE(file->isValid());
   // "real path name" reveals whether the file was actually opened.
   EXPECT_EQ("", file->tryGetRealPathName());

ilya-biryukov wrote:
> This test seems to rely on the old behavior. Is there a way to test the file 
> wasn't open other than looking at RealPathName?
There is also the vfs::file stored inside the FileEntry but unfortunately there 
are no getters exposing this one. But I believe this behavior is still correct 
for the getFile case, since `RealPathName` will only be filled when we open the 
file.



Comment at: unittests/Basic/FileManagerTest.cpp:238
   // "real path name" reveals whether the file was actually opened.
   EXPECT_EQ("", file->tryGetRealPathName());
 

ilya-biryukov wrote:
> Should we fill-in the RealPathName in that case as well?
> I'm not sure what the semantics should be. WDYT?
I don't think so, since we need the vfs::file to query the real path, I believe 
it is still ok to leave them empty for real files that hasn't been opened yet.

IIUC, stating and opening are very similar in nature(from the perspective of 
FileManager), the only difference is there is no deferred stating for virtual 
files therefore it is like a normal file that will always be opened.



Comment at: unittests/Basic/FileManagerTest.cpp:253
   file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
-  EXPECT_EQ("", file->tryGetRealPathName());
 }

ilya-biryukov wrote:
> We should find a way to test file was not "opened" (I think it actually does 
> stat the file now, right?)
AFAIK, any file accessed through getVirtualFile is immediately opened, then 
later on accessing a file with the same name through getFile with 
openfile=false won't have any affect, since this file will be alive and marked 
as opened in the cache.

So I believe opening and stating is dual of each other. Therefore checking for 
one should be equivalent to check for other.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55054



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

[PATCH] D55062: [clangd] Partition include graph on auto-index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 176090.
kadircet marked 7 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55062

Files:
  clangd/index/Background.cpp
  unittests/clangd/BackgroundIndexTests.cpp

Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -93,7 +93,7 @@
   Cmd.Filename = testPath("root/A.cc");
   Cmd.Directory = testPath("root");
   Cmd.CommandLine = {"clang++", "-DA=1", testPath("root/A.cc")};
-  CDB.setCompileCommand(testPath("root"), Cmd);
+  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
 
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
   EXPECT_THAT(
@@ -103,7 +103,7 @@
 
   Cmd.Filename = testPath("root/B.cc");
   Cmd.CommandLine = {"clang++", Cmd.Filename};
-  CDB.setCompileCommand(testPath("root"), Cmd);
+  CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
 
   ASSERT_TRUE(Idx.blockUntilIdleForTest());
   // B_CC is dropped as we don't collect symbols from A.h in this compilation.
@@ -143,7 +143,7 @@
 OverlayCDB CDB(/*Base=*/nullptr);
 BackgroundIndex Idx(Context::empty(), "", FS, CDB,
 [&](llvm::StringRef) { return &MSS; });
-CDB.setCompileCommand(testPath("root"), Cmd);
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
 ASSERT_TRUE(Idx.blockUntilIdleForTest());
   }
   EXPECT_EQ(CacheHits, 0U);
@@ -165,5 +165,48 @@
   EXPECT_THAT(*ShardSource->Refs, RefsAre({FileURI("unittest:///root/A.cc")}));
 }
 
+TEST_F(BackgroundIndexTest, DirectIncludesTest) {
+  MockFSProvider FS;
+  FS.Files[testPath("root/B.h")] = "";
+  FS.Files[testPath("root/A.h")] = R"cpp(
+  #include "B.h"
+  void common();
+  void f_b();
+  class A_CC {};
+  )cpp";
+  std::string A_CC = "#include \"A.h\"\nvoid g() { (void)common; }";
+  FS.Files[testPath("root/A.cc")] = A_CC;
+
+  llvm::StringMap Storage;
+  size_t CacheHits = 0;
+  MemoryShardStorage MSS(Storage, CacheHits);
+
+  tooling::CompileCommand Cmd;
+  Cmd.Filename = testPath("root/A.cc");
+  Cmd.Directory = testPath("root");
+  Cmd.CommandLine = {"clang++", testPath("root/A.cc")};
+  {
+OverlayCDB CDB(/*Base=*/nullptr);
+BackgroundIndex Idx(Context::empty(), "", FS, CDB,
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root/A.cc"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());
+  }
+
+  auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+  EXPECT_TRUE(ShardSource->Sources);
+  EXPECT_EQ(ShardSource->Sources->size(), 2U); // A.cc, A.h
+  EXPECT_THAT(
+  ShardSource->Sources->lookup("unittest:///root/A.cc").DirectIncludes,
+  UnorderedElementsAre("unittest:///root/A.h"));
+
+  auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
+  EXPECT_TRUE(ShardHeader->Sources);
+  EXPECT_EQ(ShardHeader->Sources->size(), 2U); // A.h B.h
+  EXPECT_THAT(
+  ShardHeader->Sources->lookup("unittest:///root/A.h").DirectIncludes,
+  UnorderedElementsAre("unittest:///root/B.h"));
+}
+
 } // namespace clangd
 } // namespace clang
Index: clangd/index/Background.cpp
===
--- clangd/index/Background.cpp
+++ clangd/index/Background.cpp
@@ -35,6 +35,59 @@
 using namespace llvm;
 namespace clang {
 namespace clangd {
+namespace {
+// Resolves URI to file paths with cache.
+class URIToFileCache {
+public:
+  URIToFileCache(llvm::StringRef HintPath) : HintPath(HintPath) {}
+
+  llvm::StringRef resolve(llvm::StringRef FileURI) {
+auto I = URIToPathCache.try_emplace(FileURI);
+if (I.second) {
+  auto U = URI::parse(FileURI);
+  if (!U) {
+elog("Failed to parse URI {0}: {1}", FileURI, U.takeError());
+assert(false && "Failed to parse URI");
+return "";
+  }
+  auto Path = URI::resolve(*U, HintPath);
+  if (!Path) {
+elog("Failed to resolve URI {0}: {1}", FileURI, Path.takeError());
+assert(false && "Failed to resolve URI");
+return "";
+  }
+  I.first->second = *Path;
+}
+return I.first->second;
+  }
+
+private:
+  std::string HintPath;
+  llvm::StringMap URIToPathCache;
+};
+
+// We keep only the node "Path" and its edges.
+IncludeGraph getSubGraph(const URI &Uri, const IncludeGraph &FullGraph) {
+  IncludeGraph IG;
+
+  std::string FileURI = Uri.toString();
+  auto Entry = IG.try_emplace(FileURI).first;
+  auto &Node = Entry->getValue();
+  Node = FullGraph.lookup(Entry->getKey());
+  Node.URI = Entry->getKey();
+
+  // Since the strings in direct includes are references to the keys of the
+  // fullgraph, we need to create entries in our new sub-graph for those
+  // includes and change references to point to the new graph's keys.
+  for (auto &Include : Node.DirectIn

[PATCH] D55062: [clangd] Partition include graph on auto-index.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: unittests/clangd/BackgroundIndexTests.cpp:192
+[&](llvm::StringRef) { return &MSS; });
+CDB.setCompileCommand(testPath("root"), Cmd);
+ASSERT_TRUE(Idx.blockUntilIdleForTest());

ilya-biryukov wrote:
> This sets a compile command for a directory. Should it be `"root/A.cc"` or am 
> I missing something?
WOW! Thanks for noticing it :D It was working correctly before because we were 
deducing the file's absolute path from the compile command itself rather than 
what we've received by watching the CDB. Changed all references in the file.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55062



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

[PATCH] D55070: Replace FullComment member being visited with parameter

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55070



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

[PATCH] D55069: Extend the CommentVisitor with parameter types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D55069#1313591 , @steveire wrote:

> The follow-up patch didn't build anymore with the `&&`.
>
>   ../tools/clang/lib/AST/ASTDumper.cpp:2672:75: error: cannot bind rvalue 
> reference of type ‘const clang::comments::FullComment*&&’ to lvalue of type 
> ‘const clang::comments::FullComment* const’
>ConstCommentVisitor::visit(C, 
> FC);
>  
>


Huh, that's surprising. It's a perfect forwarding reference and that use would 
be binding an lvalue reference, not an rvalue reference (I believe -- it's a 
bit hard to tell given that the patch with the uses is split out). Did you 
update both `visit()` and `visitComment()`? I just noticed that StmtVisitor 
also does not use perfect forwarding, so perhaps this isn't critical, but I'm 
curious as to why this doesn't behave as expected. One of the reasons I think 
this may be important is with the JSON dumper -- it may pass around JSON values 
rather than references and rely on move semantics to make this work.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55069



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

[PATCH] D55083: Re-arrange content in FunctionDecl dump

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

The summary explains what it's doing, but not why it's doing it -- why is this 
change in behavior needed? Does this not break any tests?

Btw, as we work on this refactoring, I think a good approach will be to build 
up the base of tests around AST dumping so that we can be explicitly aware of 
any behavioral changes from the patches. We have some coverage, but it doesn't 
look to be particularly comprehensive. I am happy to contribute some of these 
tests. WDYT?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55083



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

[PATCH] D54796: [clangd] C++ API for emitting file status

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 176098.
hokein marked 11 inline comments as done.
hokein added a comment.

Fix nits.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54796

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -7,12 +7,13 @@
 //
 //===--===//
 
+#include "Annotations.h"
 #include "Context.h"
 #include "Matchers.h"
 #include "TUScheduler.h"
 #include "TestFS.h"
-#include "llvm/ADT/ScopeExit.h"
 #include "gmock/gmock.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "gtest/gtest.h"
 #include 
 #include 
@@ -23,6 +24,7 @@
 namespace {
 
 using ::testing::_;
+using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -30,6 +32,9 @@
 using ::testing::Pointee;
 using ::testing::UnorderedElementsAre;
 
+MATCHER_P(TUState, State, "") { return arg.Action.S == State; }
+MATCHER_P(ActionName, Name, "") { return arg.Action.Name == Name; }
+
 class TUSchedulerTests : public ::testing::Test {
 protected:
   ParseInputs getInputs(PathRef File, std::string Contents) {
@@ -658,6 +663,78 @@
   EXPECT_EQ(Counter.load(), 3);
 }
 
+TEST_F(TUSchedulerTests, TUStatus) {
+  class CaptureTUStatus : public DiagnosticsConsumer {
+  public:
+void onDiagnosticsReady(PathRef File,
+std::vector Diagnostics) override {}
+
+void onFileUpdated(PathRef File, const TUStatus &Status) override {
+  std::lock_guard Lock(Mutex);
+  AllStatus.push_back(Status);
+}
+
+std::vector AllStatus;
+
+  private:
+std::mutex Mutex;
+  } CaptureTUStatus;
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, CaptureTUStatus, ClangdServer::optsForTest());
+  Annotations Code("int m^ain () {}");
+
+  // We schedule the following tasks in the queue:
+  //   [Update] [GoToDefinition]
+  Server.addDocument(testPath("foo.cpp"), Code.code(), WantDiagnostics::Yes);
+  Server.findDefinitions(testPath("foo.cpp"), Code.point(),
+ [](Expected> Result) {
+   ASSERT_TRUE((bool)Result);
+ });
+
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+
+  EXPECT_THAT(
+  CaptureTUStatus.AllStatus,
+  ElementsAre(
+  // Statuses of "Update" action.
+  TUState(TUAction::Queued),
+  AllOf(TUState(TUAction::RunningAction), ActionName("Update")),
+  TUState(TUAction::BuildingPreamble), TUState(TUAction::BuildingFile),
+
+  // Statuses of "Definitions" action
+  AllOf(TUState(TUAction::RunningAction), ActionName("Definitions")),
+  TUState(TUAction::Idle)));
+}
+
+TEST_F(TUSchedulerTests, NoTUStatusEmittedForRemovedFile) {
+  class CaptureTUStatus : public DiagnosticsConsumer {
+  public:
+void onDiagnosticsReady(PathRef File,
+std::vector Diagnostics) override {}
+
+void onFileUpdated(PathRef File, const TUStatus &Status) override {
+  ASSERT_TRUE(FirstRequest)
+  << "Expected to see exacly one file status updated.";
+  if (FirstRequest) {
+ASSERT_TRUE(Status.Action.S == TUAction::Queued);
+FirstRequest = false;
+// We wait long enough that the document gets removed.
+std::this_thread::sleep_for(std::chrono::milliseconds(50));
+  }
+}
+std::atomic FirstRequest = {true};
+  } CaptureTUStatus;
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, CaptureTUStatus, ClangdServer::optsForTest());
+  Server.addDocument(testPath("foo.cpp"), "int main() {}",
+ WantDiagnostics::Yes);
+  Server.removeDocument(testPath("foo.cpp"));
+  ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for finishing";
+  ASSERT_TRUE(!CaptureTUStatus.FirstRequest);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -52,6 +52,37 @@
   unsigned MaxRetainedASTs = 3;
 };
 
+struct TUAction {
+  enum State {
+Unknown,
+Queued,   // The TU is pending in the thread task queue to be built.
+RunningAction,// Starting running actions on the TU.
+BuildingPreamble, // The preamble of the TU is being built.
+BuildingFile, // The TU is being built.
+Idle, // Indicates the worker thread is idle, and ready to run any upcoming
+  // actions.
+  };
+  TUAction() = default;
+  TUAction(State S, llvm::StringRef Name) : S(S), Name(Name) {};
+  State S = Unknown

[PATCH] D54796: [clangd] C++ API for emitting file status

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clangd/ClangdServer.h:49
+  /// Called whenever the file status is updated.
+  virtual void onFileUpdated(PathRef File, const TUStatus &Status){};
 };

ilya-biryukov wrote:
> Have we thought about the way we might expose statuses via the LSP?
> The `TUStatus` seems a bit too clangd-specific (i.e. `BuildingPreamble`, 
> `ReuseAST` is not something that makes sense in the protocol). Which might be 
> fine on the `ClangdServer` layer, but it feels like we should generalize 
> before sending it over via the LSP
> The TUStatus seems a bit too clangd-specific (i.e. BuildingPreamble, ReuseAST 
> is not something that makes sense in the protocol). 

Yes, this is by design. `TUStatus` presents the internal states of clangd, and 
would not be exposed via LSP.

Some ways of exposing status via the LSP
- define a separate structure e.g. `FileStatus` in LSP,  render `TUStatus` to 
it, and sending it to clients (probably we might want to add a new extension 
`textDocument/filestatus`)
- reuse some existing LSP methods (`window/showMessage`, `window/logMessage`), 
render `TUStatus` to one of these methods' params.



Comment at: clangd/TUScheduler.h:60
+BuildingPreamble, // The preamble of the TU is being built.
+BuildingFile, // The TU is being built.
+Idle, // Indicates the worker thread is idle, and ready to run any upcoming

ilya-biryukov wrote:
> What's the fundamental difference between `BuildingFile` and `RunningAction`?
> We will often rebuild ASTs while running various actions that read the 
> preamble.
> 
> Maybe we could squash those two together? One can view diagnostics as an 
> action on the AST, similar to a direct LSP request like findReferences.
They are two different states, you can think `RunningAction` means the AST 
worker starts processing a task (for example `Update`, `GoToDefinition`) and 
`BuildingFile` means building the AST (which is one possible step when 
processing the task).  

In the current implementation, `BuildingPreamble` and `BuildingFile` are only 
emitted when AST worker processes the `Update` task (as we are mostly 
interested in TU states of `ASTWorker::update`); for other AST tasks, we just 
emit `RunningAction` which should be enough.

Given the following requests in the worker queue:
`[Update]`
`[GoToDef]`
`[Update]`
`[CodeComplete]`

statuses we emitted are

`RunningAction(Update) BuildingPreamble BuildingFile`
`RunningAction(GoToDef)`
`RunningAction(Update)  BuildingPreamble BuildingFile`
`RunningAction(GetCurrentPreamble)`
`Idle`




Comment at: clangd/TUScheduler.h:64
+  };
+  State S;
+  /// The name of the action currently running, e.g. Update, GoToDef, Hover.

ilya-biryukov wrote:
> Maybe set default value to avoid unexpected undefined behavior in case 
> someone forget to initialize the field?
Done.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54796



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

[PATCH] D55068: NFC: Simplify dumpStmt child handling

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/AST/ASTDumper.cpp:1987
 
+ConstStmtVisitor::Visit(S);
+

steveire wrote:
> aaron.ballman wrote:
> > Was there something special about calling the Visit methods directly and 
> > bailing out? Your code certainly looks reasonable, but I wonder if the 
> > output is changed because it goes through the `ConstStmtVisitor` instead of 
> > directly dispatching.
> I don't think it could have changed. By my understanding of the 
> `StmtVisitor`, this has the same effect.
Yeah, I don't see how it would either, but the original code smells suspicious 
to me. How about adding some tests for DeclStmt and GenericSelectionExpr that 
demonstrates explicitly this isn't changing behavior?

In fact, it seems that we have some bugs in this area (with DeclStmt) already, 
so understanding what's changing may point out fixes. 
https://godbolt.org/z/tDJ-0H


Repository:
  rC Clang

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

https://reviews.llvm.org/D55068



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

r347987 - Fix warning about unused variable [NFC]

[Mikael Holmen via cfe-commits]

Author: uabelho
Date: Fri Nov 30 05:38:33 2018
New Revision: 347987

URL: http://llvm.org/viewvc/llvm-project?rev=347987&view=rev
Log:
Fix warning about unused variable [NFC]

Modified:

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

Modified: 
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp?rev=347987&r1=347986&r2=347987&view=diff
==
--- 
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
 (original)
+++ 
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
 Fri Nov 30 05:38:33 2018
@@ -137,7 +137,7 @@ static void generateDiagnosticsForCallLi
 } else {
   os << "function call";
 }
-  } else if (const auto *NE = dyn_cast(S)){
+  } else if (isa(S)){
 os << "Operator new";
   } else {
 assert(isa(S));


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

[PATCH] D53708: [ASTImporter] Add importer specific lookup

[Gabor Marton via Phabricator via cfe-commits]

martong marked 8 inline comments as done.
martong added inline comments.



Comment at: lib/AST/ASTImporter.cpp:7658
+ASTImporter::FoundDeclsTy
+ASTImporter::FindDeclsInToCtx(DeclContext *DC, DeclarationName Name) {
+  // We search in the redecl context because of transparent contexts.

a_sidorin wrote:
> Naming conventions require method names to start with a small letter.
Okay, I changed it. Unfortunately `ASTImporter` does not really follow the 
conventions. Most of the functions start with a capital letter, and some with a 
small case.
E.g.:
```
/// Retrieve the file manager that AST nodes are being imported from.
FileManager &getFromFileManager() const { return FromFileManager; }

/// Report a diagnostic in the "to" context.
DiagnosticBuilder ToDiag(SourceLocation Loc, unsigned DiagID);

```



Comment at: tools/clang-import-test/clang-import-test.cpp:12
 #include "clang/AST/ASTImporter.h"
+#include "clang/AST/ASTImporterLookupTable.h"
 #include "clang/AST/DeclObjC.h"

a_sidorin wrote:
> It looks like the only change done to this file is including a header. Are 
> there any related changes that should be added?
Thank you, this is a good catch, I removed it.



Comment at: unittests/AST/ASTImporterTest.cpp:3845
+  ASTImporterLookupTable LT(*ToTU);
+}
+

a_sidorin wrote:
> martong wrote:
> > a_sidorin wrote:
> > > Could you please explain what does this test do?
> > Well, it makes sure that we can build a lookup table for an empty file 
> > without any *assertion*. We may have an assertion in the ctor during 
> > traversing the AST. I consider this the most basic use case as it tests 
> > only the constructor.
> > However, if you find it weird I can remove it.
> Yes, I think it's better to remove it or check some invariants of an empty 
> lookup table.
Ok, I removed it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53708



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

[PATCH] D53708: [ASTImporter] Add importer specific lookup

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 176100.
martong marked 3 inline comments as done.
martong added a comment.

- Address Alexei's comments
- Rename FindDeclsInToCtx to findDeclsInToCtx
- Remove superfluous spaces from stringliterals
- Remove unused header
- Remove empty test
- Return nullptr and FindInDeclListOfDC -> findInDeclListOfDC
- Use SmallSetVector instead of SmallPtrSet


Repository:
  rC Clang

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

https://reviews.llvm.org/D53708

Files:
  include/clang/AST/ASTImporter.h
  include/clang/AST/ASTImporterLookupTable.h
  include/clang/CrossTU/CrossTranslationUnit.h
  lib/AST/ASTImporter.cpp
  lib/AST/ASTImporterLookupTable.cpp
  lib/AST/CMakeLists.txt
  lib/CrossTU/CrossTranslationUnit.cpp
  lib/Frontend/ASTMerge.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -14,6 +14,7 @@
 #include "MatchVerifier.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTImporter.h"
+#include "clang/AST/ASTImporterLookupTable.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
@@ -307,24 +308,27 @@
   Unit->enableSourceFileDiagnostics();
 }
 
-void lazyInitImporter(ASTUnit *ToAST) {
+void lazyInitImporter(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST) {
   assert(ToAST);
   if (!Importer) {
-Importer.reset(new ASTImporter(
-ToAST->getASTContext(), ToAST->getFileManager(),
-Unit->getASTContext(), Unit->getFileManager(), false));
+Importer.reset(
+new ASTImporter(ToAST->getASTContext(), ToAST->getFileManager(),
+Unit->getASTContext(), Unit->getFileManager(),
+false, &LookupTable));
   }
   assert(&ToAST->getASTContext() == &Importer->getToContext());
   createVirtualFileIfNeeded(ToAST, FileName, Code);
 }
 
-Decl *import(ASTUnit *ToAST, Decl *FromDecl) {
-  lazyInitImporter(ToAST);
+Decl *import(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST,
+ Decl *FromDecl) {
+  lazyInitImporter(LookupTable, ToAST);
   return Importer->Import(FromDecl);
- }
+}
 
-QualType import(ASTUnit *ToAST, QualType FromType) {
-  lazyInitImporter(ToAST);
+QualType import(ASTImporterLookupTable &LookupTable, ASTUnit *ToAST,
+QualType FromType) {
+  lazyInitImporter(LookupTable, ToAST);
   return Importer->Import(FromType);
 }
   };
@@ -338,13 +342,23 @@
   // vector is expanding, with the list we won't have these issues.
   std::list FromTUs;
 
-  void lazyInitToAST(Language ToLang) {
+  // Initialize the lookup table if not initialized already.
+  void lazyInitLookupTable(TranslationUnitDecl *ToTU) {
+assert(ToTU);
+if (!LookupTablePtr)
+  LookupTablePtr = llvm::make_unique(*ToTU);
+  }
+
+  void lazyInitToAST(Language ToLang, StringRef ToSrcCode, StringRef FileName) {
 if (ToAST)
   return;
 ArgVector ToArgs = getArgVectorForLanguage(ToLang);
+// Source code must be a valid live buffer through the tests lifetime.
+ToCode = ToSrcCode;
 // Build the AST from an empty file.
-ToAST = tooling::buildASTFromCodeWithArgs(/*Code=*/"", ToArgs, "empty.cc");
+ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, FileName);
 ToAST->enableSourceFileDiagnostics();
+lazyInitLookupTable(ToAST->getASTContext().getTranslationUnitDecl());
   }
 
   TU *findFromTU(Decl *From) {
@@ -358,6 +372,10 @@
 return &*It;
   }
 
+protected:
+
+  std::unique_ptr LookupTablePtr;
+
 public:
   // We may have several From context but only one To context.
   std::unique_ptr ToAST;
@@ -374,26 +392,23 @@
 FromTUs.emplace_back(FromSrcCode, InputFileName, FromArgs);
 TU &FromTU = FromTUs.back();
 
-ToCode = ToSrcCode;
 assert(!ToAST);
-ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
-ToAST->enableSourceFileDiagnostics();
+lazyInitToAST(ToLang, ToSrcCode, OutputFileName);
 
 ASTContext &FromCtx = FromTU.Unit->getASTContext();
 
-createVirtualFileIfNeeded(ToAST.get(), InputFileName, FromTU.Code);
-
 IdentifierInfo *ImportedII = &FromCtx.Idents.get(Identifier);
 assert(ImportedII && "Declaration with the given identifier "
  "should be specified in test!");
 DeclarationName ImportDeclName(ImportedII);
-SmallVector FoundDecls;
+SmallVector FoundDecls;
 FromCtx.getTranslationUnitDecl()->localUncachedLookup(ImportDeclName,
   FoundDecls);
 
 assert(FoundDecls.size() == 1);
 
-Decl *Imported = FromTU.import(ToAST.get(), FoundDecls.front());
+Decl *Imported =
+FromTU.import(*LookupT

[PATCH] D55101: [clang-tidy] Ignore namespaced and C++ member functions in google-objc-function-naming check 🙈

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!




Comment at: test/clang-tidy/google-objc-function-naming.mm:24-25
+
+class Baz {
+  public:
+int value() { return 0; }

Can you run this test file through clang-format?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55101



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

[PATCH] D53708: [ASTImporter] Add importer specific lookup

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Hey Alexei,

Thank you again for your comments. I changed to code accordingly.

Also, I recognized that we should use a `SmallSetVector` instead of a 
`SmallPtrSet`, because the latter results unpredictable iteration (pointers may 
have different values with each run). Though `SmallPtrSet` is more powerful 
than `SmallSetVector` it makes it really hard to debug false positive ODR 
warnings from the ASTImporter, because the warning may appear in one run but 
may not appear in the subsequent run. We need an iteration order in the order 
of insertions and that is produced by `SmallSetVector`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53708



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Hyrum Wright via Phabricator via cfe-commits]

hwright updated this revision to Diff 176103.
hwright added a comment.

Slightly simplify the fixit text


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

https://reviews.llvm.org/D54737

Files:
  clang-tidy/abseil/AbseilTidyModule.cpp
  clang-tidy/abseil/CMakeLists.txt
  clang-tidy/abseil/DurationComparisonCheck.cpp
  clang-tidy/abseil/DurationComparisonCheck.h
  clang-tidy/abseil/DurationFactoryFloatCheck.cpp
  clang-tidy/abseil/DurationFactoryScaleCheck.cpp
  clang-tidy/abseil/DurationRewriter.cpp
  clang-tidy/abseil/DurationRewriter.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/abseil-duration-comparison.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/abseil-duration-comparison.cpp

Index: test/clang-tidy/abseil-duration-comparison.cpp
===
--- /dev/null
+++ test/clang-tidy/abseil-duration-comparison.cpp
@@ -0,0 +1,189 @@
+// RUN: %check_clang_tidy %s abseil-duration-comparison %t
+
+// Mimic the implementation of absl::Duration
+namespace absl {
+
+class Duration {};
+class Time{};
+
+Duration Nanoseconds(long long);
+Duration Microseconds(long long);
+Duration Milliseconds(long long);
+Duration Seconds(long long);
+Duration Minutes(long long);
+Duration Hours(long long);
+
+#define GENERATE_DURATION_FACTORY_OVERLOADS(NAME) \
+  Duration NAME(float n); \
+  Duration NAME(double n);\
+  template\
+  Duration NAME(T n);
+
+GENERATE_DURATION_FACTORY_OVERLOADS(Nanoseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Microseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Milliseconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Seconds);
+GENERATE_DURATION_FACTORY_OVERLOADS(Minutes);
+GENERATE_DURATION_FACTORY_OVERLOADS(Hours);
+#undef GENERATE_DURATION_FACTORY_OVERLOADS
+
+using int64_t = long long int;
+
+double ToDoubleHours(Duration d);
+double ToDoubleMinutes(Duration d);
+double ToDoubleSeconds(Duration d);
+double ToDoubleMilliseconds(Duration d);
+double ToDoubleMicroseconds(Duration d);
+double ToDoubleNanoseconds(Duration d);
+int64_t ToInt64Hours(Duration d);
+int64_t ToInt64Minutes(Duration d);
+int64_t ToInt64Seconds(Duration d);
+int64_t ToInt64Milliseconds(Duration d);
+int64_t ToInt64Microseconds(Duration d);
+int64_t ToInt64Nanoseconds(Duration d);
+
+// Relational Operators
+constexpr bool operator<(Duration lhs, Duration rhs);
+constexpr bool operator>(Duration lhs, Duration rhs);
+constexpr bool operator>=(Duration lhs, Duration rhs);
+constexpr bool operator<=(Duration lhs, Duration rhs);
+constexpr bool operator==(Duration lhs, Duration rhs);
+constexpr bool operator!=(Duration lhs, Duration rhs);
+
+// Additive Operators
+inline Time operator+(Time lhs, Duration rhs);
+inline Time operator+(Duration lhs, Time rhs);
+inline Time operator-(Time lhs, Duration rhs);
+inline Duration operator-(Time lhs, Time rhs);
+
+}  // namespace absl
+
+void f() {
+  double x;
+  absl::Duration d1, d2;
+  bool b;
+  absl::Time t1, t2;
+
+  // Check against the RHS
+  b = x > absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) > d1;
+  b = x >= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) >= d1;
+  b = x == absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == d1;
+  b = x <= absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) <= d1;
+  b = x < absl::ToDoubleSeconds(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) < d1;
+  b = x == absl::ToDoubleSeconds(t1 - t2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: absl::Seconds(x) == t1 - t2;
+  b = absl::ToDoubleSeconds(d1) > absl::ToDoubleSeconds(d2);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 > d2;
+
+  // Check against the LHS
+  b = absl::ToDoubleSeconds(d1) < x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 < absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) <= x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the duration domain [abseil-duration-comparison]
+  // CHECK-FIXES: d1 <= absl::Seconds(x);
+  b = absl::ToDoubleSeconds(d1) == x;
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perf

[PATCH] D54796: [clangd] C++ API for emitting file status

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/ClangdServer.h:49
+  /// Called whenever the file status is updated.
+  virtual void onFileUpdated(PathRef File, const TUStatus &Status){};
 };

hokein wrote:
> ilya-biryukov wrote:
> > Have we thought about the way we might expose statuses via the LSP?
> > The `TUStatus` seems a bit too clangd-specific (i.e. `BuildingPreamble`, 
> > `ReuseAST` is not something that makes sense in the protocol). Which might 
> > be fine on the `ClangdServer` layer, but it feels like we should generalize 
> > before sending it over via the LSP
> > The TUStatus seems a bit too clangd-specific (i.e. BuildingPreamble, 
> > ReuseAST is not something that makes sense in the protocol). 
> 
> Yes, this is by design. `TUStatus` presents the internal states of clangd, 
> and would not be exposed via LSP.
> 
> Some ways of exposing status via the LSP
> - define a separate structure e.g. `FileStatus` in LSP,  render `TUStatus` to 
> it, and sending it to clients (probably we might want to add a new extension 
> `textDocument/filestatus`)
> - reuse some existing LSP methods (`window/showMessage`, 
> `window/logMessage`), render `TUStatus` to one of these methods' params.
LG, I believe the `BuildingPreamble` status in particular might be useful on 
the C++ API level for us.
Not sure `showMessage` or `logMessage` is a good idea, the first one will 
definitely be annoying if it'll be popping up all the time. Having a new method 
in the LSP LG.



Comment at: clangd/TUScheduler.cpp:267
   bool ReportDiagnostics = true; /* GUARDED_BY(DiagMu) */
+  /// Status of the TU.
+  TUStatus Status; /* GUARDED_BY(DiagMu) */

The comment duplicates the code, maybe remove it?



Comment at: clangd/TUScheduler.cpp:268
+  /// Status of the TU.
+  TUStatus Status; /* GUARDED_BY(DiagMu) */
 };

Is `Status` actually ever read from multiple threads?
I assume it's only accessed on the worker thread, right? I think we can leave 
out the locks around it and put beside other fields only accessed by the worker 
thread, i.e. `DiagsWereReported`, etc.

PS Sorry go going back and forth, I didn't think about it in the previous 
review iteration.



Comment at: clangd/TUScheduler.cpp:581
+  std::lock_guard Lock(DiagsMu);
+  // Stop emitting file statuses when the ASTWorker is shutting down.
+  if (ReportDiagnostics) {

NIT: Maybe change `Stop emitting` to `Do not emit`?
The current wording seems to suggest we're gonna signal some other code to stop 
doing that...



Comment at: clangd/TUScheduler.cpp:636
+  if (Requests.empty())
+emitTUStatus({TUAction::Idle, /*Name*/ ""});
 }

Maybe do it outside the lock? The less dependencies between the locks we have, 
the better and this one seems unnecessary.



Comment at: clangd/TUScheduler.h:57
+  enum State {
+Unknown,
+Queued,   // The TU is pending in the thread task queue to be 
built.

Maybe remove this value and leave out the default constructor too?
That would ensure we never fill have `State` field with undefined value without 
having this enumerator.
And FWIW, putting **any** value as the default seems fine even if we want to 
keep the default ctor, all the users have to set the State anyway. 



Comment at: clangd/TUScheduler.h:66
+  TUAction() = default;
+  TUAction(State S, llvm::StringRef Name) : S(S), Name(Name) {};
+  State S = Unknown;

Ah, the annoying part about member initializers :-(
(Just grumbling, no need to do anything about it)



Comment at: clangd/TUScheduler.h:60
+BuildingPreamble, // The preamble of the TU is being built.
+BuildingFile, // The TU is being built.
+Idle, // Indicates the worker thread is idle, and ready to run any upcoming

hokein wrote:
> ilya-biryukov wrote:
> > What's the fundamental difference between `BuildingFile` and 
> > `RunningAction`?
> > We will often rebuild ASTs while running various actions that read the 
> > preamble.
> > 
> > Maybe we could squash those two together? One can view diagnostics as an 
> > action on the AST, similar to a direct LSP request like findReferences.
> They are two different states, you can think `RunningAction` means the AST 
> worker starts processing a task (for example `Update`, `GoToDefinition`) and 
> `BuildingFile` means building the AST (which is one possible step when 
> processing the task).  
> 
> In the current implementation, `BuildingPreamble` and `BuildingFile` are only 
> emitted when AST worker processes the `Update` task (as we are mostly 
> interested in TU states of `ASTWorker::update`); for other AST tasks, we just 
> emit `RunningAction` which should be enough.
> 
> Given the following requests in the worker queue:
> `[Update]`
> `[GoToDef]`
> `[Update]`
> `[CodeComp

[PATCH] D55069: Extend the CommentVisitor with parameter types

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

> Huh, that's surprising. It's a perfect forwarding reference

It's not a forwarding reference because the template parameter is from the 
record, not the function. See

https://godbolt.org/z/L4N2aS

> One of the reasons I think this may be important is with the JSON dumper -- 
> it may pass around JSON values rather than references and rely on move 
> semantics to make this work.

I don't know what you're planning so I can't imagine what you want to pass 
through these visit function parameters. It would be surprising to me if you 
used this feature of the visitor for json dumping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55069



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

[PATCH] D55069: Extend the CommentVisitor with parameter types

[Aaron Ballman via Phabricator via cfe-commits]

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

In D55069#1314508 , @steveire wrote:

> > Huh, that's surprising. It's a perfect forwarding reference
>
> It's not a forwarding reference because the template parameter is from the 
> record, not the function. See
>
>   https://godbolt.org/z/L4N2aS


Ah, you're exactly right! I hadn't picked up on that.

>> One of the reasons I think this may be important is with the JSON dumper -- 
>> it may pass around JSON values rather than references and rely on move 
>> semantics to make this work.
> 
> I don't know what you're planning so I can't imagine what you want to pass 
> through these visit function parameters. It would be surprising to me if you 
> used this feature of the visitor for json dumping.

It can be dealt with if/when it arises.

LGTM!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55069



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

[PATCH] D55121: Make several Python scripts portable across Python2 and Python 3

[serge via Phabricator via cfe-commits]

serge-sans-paille created this revision.
serge-sans-paille added reviewers: ddunbar, djasper.
Herald added subscribers: cfe-commits, kadircet, arphaman, whisperity.

  Mostly:
  
  - portability of print through from __future__ import print_function
  - xrange -> range
  - range(...) -> list(range(...))
  - x.items() -> list(x.items())
  - map(f, l) -> list(map(f, l))
  - normalize shebang
  
  No extra dependency added.


Repository:
  rC Clang

https://reviews.llvm.org/D55121

Files:
  bindings/python/clang/cindex.py
  bindings/python/examples/cindex/cindex-dump.py
  bindings/python/tests/cindex/test_translation_unit.py
  docs/conf.py
  docs/tools/dump_format_style.py
  tools/clang-format/clang-format-diff.py
  tools/scan-build-py/libscanbuild/arguments.py
  tools/scan-view/share/Reporter.py
  utils/ABITest/ABITestGen.py
  utils/ABITest/Enumeration.py
  utils/CIndex/completion_logger_server.py
  utils/TestUtils/deep-stack.py
  utils/analyzer/SATestAdd.py
  utils/analyzer/SATestUpdateDiffs.py
  utils/analyzer/SumTimerInfo.py
  utils/check_cfc/obj_diff.py
  utils/check_cfc/setup.py
  utils/check_cfc/test_check_cfc.py
  utils/clangdiag.py
  utils/modfuzz.py
  utils/perf-training/perf-helper.py
  utils/token-delta.py
  www/builtins.py

Index: www/builtins.py
===
--- www/builtins.py
+++ www/builtins.py
@@ -151,7 +151,7 @@
   sys.stderr.write("%s:%d: x86 builtin %s used, too many replacements\n" % (fileinput.filename(), fileinput.filelineno(), builtin))
 
 for line in fileinput.input(inplace=1):
-  for builtin, repl in repl_map.iteritems():
+  for builtin, repl in repl_map.items():
 if builtin in line:
   line = line.replace(builtin, repl)
   report_repl(builtin, repl)
Index: utils/token-delta.py
===
--- utils/token-delta.py
+++ utils/token-delta.py
@@ -1,5 +1,6 @@
 #!/usr/bin/env python
 
+from __future__ import print_function
 import os
 import re
 import subprocess
@@ -37,7 +38,7 @@
 # O(N^2) case unless user requests it.
 if not force:
 if not self.getTestResult(changes):
-raise ValueError,'Initial test passed to delta fails.'
+raise ValueError('Initial test passed to delta fails.')
 
 # Check empty set first to quickly find poor test functions.
 if self.getTestResult(set()):
@@ -94,7 +95,7 @@
 
 ###
 
-class Token:
+class Token(object):
 def __init__(self, type, data, flags, file, line, column):
 self.type   = type
 self.data   = data
@@ -165,7 +166,7 @@
 byFile = self.writeFiles(changes, self.tempFiles)
 
 if self.log:
-print >>sys.stderr, 'TEST - ',
+print('TEST - ', end=' ', file=sys.stderr)
 if self.log > 1:
 for i,(file,_) in enumerate(self.tokenLists):
 indices = byFile[i]
@@ -184,8 +185,8 @@
 sys.stderr.write(str(byFile[i][-1]))
 sys.stderr.write('] ')
 else:
-print >>sys.stderr, ', '.join(['%s:%d tokens' % (file, len(byFile[i]))
-   for i,(file,_) in enumerate(self.tokenLists)]),
+print(', '.join(['%s:%d tokens' % (file, len(byFile[i]))
+   for i,(file,_) in enumerate(self.tokenLists)]), end=' ', file=sys.stderr)
 
 p = subprocess.Popen([self.testProgram] + self.tempFiles)
 res = p.wait() == 0
@@ -194,10 +195,10 @@
 self.writeFiles(changes, self.targetFiles)
 
 if self.log:
-print >>sys.stderr, '=> %s' % res
+print('=> %s' % res, file=sys.stderr)
 else:
 if res:
-print '\nSUCCESS (%d tokens)' % len(changes)
+print('\nSUCCESS (%d tokens)' % len(changes))
 else:
 sys.stderr.write('.')
 
@@ -209,7 +210,7 @@
   for j in range(len(tokens))])
 self.writeFiles(res, self.targetFiles)
 if not self.log:
-print >>sys.stderr
+print(file=sys.stderr)
 return res
 
 def tokenBasedMultiDelta(program, files, log):
@@ -218,15 +219,15 @@
   for file in files]
 
 numTokens = sum([len(tokens) for _,tokens in tokenLists])
-print "Delta on %s with %d tokens." % (', '.join(files), numTokens)
+print("Delta on %s with %d tokens." % (', '.join(files), numTokens))
 
 tbmd = TMBDDelta(program, tokenLists, log)
 
 res = tbmd.run()
 
-print "Finished %s with %d tokens (in %d tests)." % (', '.join(tbmd.targetFiles),
+print("Finished %s with %d tokens (in %d tests)." % (', '.join(tbmd.targetFiles),
  len(res),
- tbmd.numTests)
+   

[PATCH] D55121: Make several Python scripts portable across Python2 and Python 3

[serge via Phabricator via cfe-commits]

serge-sans-paille added a comment.

Conversion using the ``futurize`` script, then manual review. For the sake of 
reviewer sanity, I've left all the difficult part to further separate commits.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55121



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

[PATCH] D55068: NFC: Simplify dumpStmt child handling

[Stephen Kelly via Phabricator via cfe-commits]

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



Comment at: lib/AST/ASTDumper.cpp:1987
 
+ConstStmtVisitor::Visit(S);
+

aaron.ballman wrote:
> steveire wrote:
> > aaron.ballman wrote:
> > > Was there something special about calling the Visit methods directly and 
> > > bailing out? Your code certainly looks reasonable, but I wonder if the 
> > > output is changed because it goes through the `ConstStmtVisitor` instead 
> > > of directly dispatching.
> > I don't think it could have changed. By my understanding of the 
> > `StmtVisitor`, this has the same effect.
> Yeah, I don't see how it would either, but the original code smells 
> suspicious to me. How about adding some tests for DeclStmt and 
> GenericSelectionExpr that demonstrates explicitly this isn't changing 
> behavior?
> 
> In fact, it seems that we have some bugs in this area (with DeclStmt) 
> already, so understanding what's changing may point out fixes. 
> https://godbolt.org/z/tDJ-0H
A visual inspection and reading of the visitor should show that this patch is 
NFC. I'd welcome a test if you can make one. The test should be in the repo 
before this commit anyway.

> In fact, it seems that we have some bugs in this area (with DeclStmt) 
> already, so understanding what's changing may point out fixes. 
> https://godbolt.org/z/tDJ-0H

That is not related to this refactoring.

Also, clang-query has different output:

http://ec2-52-14-16-249.us-east-2.compute.amazonaws.com:10240/z/W_kZFl



Repository:
  rC Clang

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

https://reviews.llvm.org/D55068



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

[PATCH] D55083: Re-arrange content in FunctionDecl dump

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

Yes, please commit your new tests for FunctionDecl dumping before this patch 
can go in.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55083



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

[PATCH] D54903: [Sema] Improve static_assert diagnostics.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/AST/NestedNameSpecifier.cpp:308-310
+if (ResolveTemplateArguments && getAsRecordDecl()) {
+  if (const auto *Record =
+  dyn_cast(getAsRecordDecl())) {

I'd remove the `getAsRecordDecl()` from the first `if` and instead use 
`dyn_cast_or_null` in the second `if`. Probably something like:
```
const auto *Record = 
dyn_cast_or_null(getAsRecordDecl());
if (ResolveTemplateArguments && Record) {
}
```



Comment at: lib/AST/NestedNameSpecifier.cpp:332
"Elaborated type in nested-name-specifier");
-if (const TemplateSpecializationType *SpecType
-  = dyn_cast(T)) {
+if (const TemplateSpecializationType *SpecType =
+dyn_cast(T)) {

This looks to be a spurious formatting change.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54903



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

[PATCH] D55076: [analyzer] RetainCountChecker: recognize that OSObject can be created directly using an operator "new"

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/test/Analysis/osobject-retain-release.cpp:27
+
+  static void * operator new(unsigned long size);
+

NoQ wrote:
> I think we should use `size_t` as much as possible, because this may 
> otherwise have weird consequences on platforms on which `size_t` is not 
> defined as `unsigned long`. Not sure if this checker is ran on such 
> platforms. But the test doesn't have the triple specified, so it runs under 
> the host triple, which may be arbitrary and cause problems on buildbots.
> 
> I.e.,
> 
> ```
> typedef __typeof(sizeof(int)) size_t;
> // use size_t
> ```
http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/440/steps/ninja%20check%202/logs/FAIL%3A%20Clang%3A%3Aosobject-retain-release.cpp


Repository:
  rC Clang

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

https://reviews.llvm.org/D55076



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

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:7598-7602
+  if (ICE->getCastKind() == CK_IntegralCast &&
+  From->isPromotableIntegerType() &&
+  S.Context.getPromotedIntegerType(From) == To)
+return true;
+  return false;

Can be simplified to: `return ICE->getCastKind() == CK_IntegralCast && 
From->isPromotableIntegerType() && S.Context.getPromotedIntegerType(From) == 
To;`



Comment at: test/Sema/format-strings.c:699-700
+  unsigned long b : 2;
+  long c : 32;  // assumes that int is 32 bits
+  unsigned long d : 32; // assumes that int is 32 bits
+} bf;

The test currently does not have a target triple, so this is likely to fail on 
some bots. Rather than tie this entire test to one architecture, I'd split the 
bit-field tests out into a separate file and pick an explicit triple that meets 
this assumption.


Repository:
  rC Clang

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

https://reviews.llvm.org/D51211



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

[PATCH] D54796: [clangd] C++ API for emitting file status

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 176111.
hokein marked 12 inline comments as done.
hokein added a comment.

Address comments and fix a bug in the test.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54796

Files:
  clangd/ClangdServer.cpp
  clangd/ClangdServer.h
  clangd/TUScheduler.cpp
  clangd/TUScheduler.h
  unittests/clangd/TUSchedulerTests.cpp

Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -7,12 +7,13 @@
 //
 //===--===//
 
+#include "Annotations.h"
 #include "Context.h"
 #include "Matchers.h"
 #include "TUScheduler.h"
 #include "TestFS.h"
-#include "llvm/ADT/ScopeExit.h"
 #include "gmock/gmock.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "gtest/gtest.h"
 #include 
 #include 
@@ -23,6 +24,7 @@
 namespace {
 
 using ::testing::_;
+using ::testing::AllOf;
 using ::testing::AnyOf;
 using ::testing::Each;
 using ::testing::ElementsAre;
@@ -30,6 +32,10 @@
 using ::testing::Pointee;
 using ::testing::UnorderedElementsAre;
 
+MATCHER_P2(TUState, State, ActionName, "") {
+  return arg.Action.S == State && arg.Action.Name == ActionName;
+}
+
 class TUSchedulerTests : public ::testing::Test {
 protected:
   ParseInputs getInputs(PathRef File, std::string Contents) {
@@ -658,6 +664,77 @@
   EXPECT_EQ(Counter.load(), 3);
 }
 
+TEST_F(TUSchedulerTests, TUStatus) {
+  class CaptureTUStatus : public DiagnosticsConsumer {
+  public:
+void onDiagnosticsReady(PathRef File,
+std::vector Diagnostics) override {}
+
+void onFileUpdated(PathRef File, const TUStatus &Status) override {
+  std::lock_guard Lock(Mutex);
+  AllStatus.push_back(Status);
+}
+
+std::vector AllStatus;
+
+  private:
+std::mutex Mutex;
+  } CaptureTUStatus;
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, CaptureTUStatus, ClangdServer::optsForTest());
+  Annotations Code("int m^ain () {}");
+
+  // We schedule the following tasks in the queue:
+  //   [Update] [GoToDefinition]
+  Server.addDocument(testPath("foo.cpp"), Code.code(), WantDiagnostics::Yes);
+  Server.findDefinitions(testPath("foo.cpp"), Code.point(),
+ [](Expected> Result) {
+   ASSERT_TRUE((bool)Result);
+ });
+
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+
+  EXPECT_THAT(CaptureTUStatus.AllStatus,
+  ElementsAre(
+  // Statuses of "Update" action.
+  TUState(TUAction::Queued, "Update"),
+  TUState(TUAction::RunningAction, "Update"),
+  TUState(TUAction::BuildingPreamble, "Update"),
+  TUState(TUAction::BuildingFile, "Update"),
+
+  // Statuses of "Definitions" action
+  TUState(TUAction::RunningAction, "Definitions"),
+  TUState(TUAction::Idle, /*No action*/ "")));
+}
+
+TEST_F(TUSchedulerTests, NoTUStatusEmittedForRemovedFile) {
+  class CaptureTUStatus : public DiagnosticsConsumer {
+  public:
+void onDiagnosticsReady(PathRef File,
+std::vector Diagnostics) override {}
+
+void onFileUpdated(PathRef File, const TUStatus &Status) override {
+  // Queued is emitted by the main thread, we don't block it.
+  if (Status.Action.S == TUAction::Queued)
+return;
+  // Block the worker thread until the document is removed.
+  ASSERT_TRUE(Status.Action.S == TUAction::RunningAction);
+  Removed.wait();
+}
+Notification Removed;
+  } CaptureTUStatus;
+  MockFSProvider FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, CaptureTUStatus, ClangdServer::optsForTest());
+
+  Server.addDocument(testPath("foo.cpp"), "int main() {}",
+ WantDiagnostics::Yes);
+  Server.removeDocument(testPath("foo.cpp"));
+  CaptureTUStatus.Removed.notify();
+  ASSERT_TRUE(Server.blockUntilIdleForTest()) << "Waiting for finishing";
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -52,6 +52,37 @@
   unsigned MaxRetainedASTs = 3;
 };
 
+struct TUAction {
+  enum State {
+Unknown,
+Queued,   // The TU is pending in the thread task queue to be built.
+RunningAction,// Starting running actions on the TU.
+BuildingPreamble, // The preamble of the TU is being built.
+BuildingFile, // The TU is being built.
+Idle, // Indicates the worker thread is idle, and ready to run any upcoming
+  // actions.
+  };
+  TUAction() = default;
+  TUAction(State S, llvm::StringRef Name) : S(S), Name(Name){};
+  State S = U

[PATCH] D54796: [clangd] C++ API for emitting file status

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as not done.
hokein added inline comments.



Comment at: clangd/ClangdServer.h:49
+  /// Called whenever the file status is updated.
+  virtual void onFileUpdated(PathRef File, const TUStatus &Status){};
 };

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > Have we thought about the way we might expose statuses via the LSP?
> > > The `TUStatus` seems a bit too clangd-specific (i.e. `BuildingPreamble`, 
> > > `ReuseAST` is not something that makes sense in the protocol). Which 
> > > might be fine on the `ClangdServer` layer, but it feels like we should 
> > > generalize before sending it over via the LSP
> > > The TUStatus seems a bit too clangd-specific (i.e. BuildingPreamble, 
> > > ReuseAST is not something that makes sense in the protocol). 
> > 
> > Yes, this is by design. `TUStatus` presents the internal states of clangd, 
> > and would not be exposed via LSP.
> > 
> > Some ways of exposing status via the LSP
> > - define a separate structure e.g. `FileStatus` in LSP,  render `TUStatus` 
> > to it, and sending it to clients (probably we might want to add a new 
> > extension `textDocument/filestatus`)
> > - reuse some existing LSP methods (`window/showMessage`, 
> > `window/logMessage`), render `TUStatus` to one of these methods' params.
> LG, I believe the `BuildingPreamble` status in particular might be useful on 
> the C++ API level for us.
> Not sure `showMessage` or `logMessage` is a good idea, the first one will 
> definitely be annoying if it'll be popping up all the time. Having a new 
> method in the LSP LG.
`showMessage` seems work well via vim YCM, but it is annoying in vscode (as it 
pops up diags) -- we need to customize this behavior in our own extensions 
(showing messages in status bar). We need to revisit different ways. 



Comment at: clangd/TUScheduler.cpp:268
+  /// Status of the TU.
+  TUStatus Status; /* GUARDED_BY(DiagMu) */
 };

ilya-biryukov wrote:
> Is `Status` actually ever read from multiple threads?
> I assume it's only accessed on the worker thread, right? I think we can leave 
> out the locks around it and put beside other fields only accessed by the 
> worker thread, i.e. `DiagsWereReported`, etc.
> 
> PS Sorry go going back and forth, I didn't think about it in the previous 
> review iteration.
Unfortunately not, most statuses are emitted via worker thread, but we emit 
`Queued` status in the main thread...



Comment at: clangd/TUScheduler.cpp:636
+  if (Requests.empty())
+emitTUStatus({TUAction::Idle, /*Name*/ ""});
 }

ilya-biryukov wrote:
> Maybe do it outside the lock? The less dependencies between the locks we 
> have, the better and this one seems unnecessary.
Is it safe to read the `Requests` out of the lock here?



Comment at: clangd/TUScheduler.h:60
+BuildingPreamble, // The preamble of the TU is being built.
+BuildingFile, // The TU is being built.
+Idle, // Indicates the worker thread is idle, and ready to run any upcoming

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > What's the fundamental difference between `BuildingFile` and 
> > > `RunningAction`?
> > > We will often rebuild ASTs while running various actions that read the 
> > > preamble.
> > > 
> > > Maybe we could squash those two together? One can view diagnostics as an 
> > > action on the AST, similar to a direct LSP request like findReferences.
> > They are two different states, you can think `RunningAction` means the AST 
> > worker starts processing a task (for example `Update`, `GoToDefinition`) 
> > and `BuildingFile` means building the AST (which is one possible step when 
> > processing the task).  
> > 
> > In the current implementation, `BuildingPreamble` and `BuildingFile` are 
> > only emitted when AST worker processes the `Update` task (as we are mostly 
> > interested in TU states of `ASTWorker::update`); for other AST tasks, we 
> > just emit `RunningAction` which should be enough.
> > 
> > Given the following requests in the worker queue:
> > `[Update]`
> > `[GoToDef]`
> > `[Update]`
> > `[CodeComplete]`
> > 
> > statuses we emitted are
> > 
> > `RunningAction(Update) BuildingPreamble BuildingFile`
> > `RunningAction(GoToDef)`
> > `RunningAction(Update)  BuildingPreamble BuildingFile`
> > `RunningAction(GetCurrentPreamble)`
> > `Idle`
> > 
> That's the confusing part: clangd might be building the ASTs inside 
> `RunningAction` too, but we only choose to report `BuildingFile` during 
> updates.
> I would get rid of `BuildingFile` and change it to 
> `RunningAction(Diagnostics)` instead, it feels like that would allow avoiding 
> some confusion in the future.
> 
`BuildingFile` might be more natural (understandable) than `Diagnostics`. Two 
possible ways here:

- we can also report `BuildingFile` for other AST tasks, does it reduce the 
confusing part?

r347994 - Adding tests for -ast-dump; NFC.

[Aaron Ballman via cfe-commits]

Author: aaronballman
Date: Fri Nov 30 06:43:21 2018
New Revision: 347994

URL: http://llvm.org/viewvc/llvm-project?rev=347994&view=rev
Log:
Adding tests for -ast-dump; NFC.

This adds tests for the majority of the functionality around FunctionDecl and 
CXXMethodDecl.

Added:
cfe/trunk/test/Misc/ast-dump-funcs.cpp

Added: cfe/trunk/test/Misc/ast-dump-funcs.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/ast-dump-funcs.cpp?rev=347994&view=auto
==
--- cfe/trunk/test/Misc/ast-dump-funcs.cpp (added)
+++ cfe/trunk/test/Misc/ast-dump-funcs.cpp Fri Nov 30 06:43:21 2018
@@ -0,0 +1,124 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ast-dump %s | FileCheck 
-strict-whitespace %s
+
+struct R {
+  R() = default;
+  // CHECK: CXXConstructorDecl 0x{{[^ ]*}}  col:3 
used constexpr R 'void () noexcept' default trivial
+  ~R() {} // not trivial
+  // CHECK: CXXDestructorDecl 0x{{[^ ]*}}  col:3 
used ~R 'void () noexcept'
+  R(const R&) = delete;
+  // CHECK: CXXConstructorDecl 0x{{[^ ]*}}  col:3 
R 'void (const R &)' delete trivial
+  R(R&&) = default;
+  // CHECK: CXXConstructorDecl 0x{{[^ ]*}}  col:3 
constexpr R 'void (R &&)' default trivial noexcept-unevaluated
+
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:8 implicit 
operator= 'R &(const R &)' inline default_delete trivial noexcept-unevaluated
+};
+
+struct S {
+  int i, j;
+  R r;
+
+  S() : i(0), j(0) {}
+  // CHECK: CXXConstructorDecl 0x{{[^ ]*}}  col:3 
S 'void ()'
+  // CHECK-NEXT: CXXCtorInitializer Field 0x{{[^ ]*}} 'i' 'int'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 0
+  // CHECK-NEXT: CXXCtorInitializer Field 0x{{[^ ]*}} 'j' 'int'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 0
+  // CHECK-NEXT: CXXCtorInitializer Field 0x{{[^ ]*}} 'r' 'R'
+  // CHECK-NEXT: CXXConstructExpr 0x{{[^ ]*}}  'R' 'void () noexcept'
+  // CHECK-NEXT: CompoundStmt 0x{{[^ ]*}} 
+
+  void a();
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:8 a 
'void ()'
+  void b() const;
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:8 b 
'void () const'
+  void c() volatile;
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:8 c 
'void () volatile'
+  void d() &;
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:8 d 
'void () &'
+  void e() &&;
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:8 e 
'void () &&'
+  virtual void f(float, int = 12);
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:16 f 
'void (float, int)' virtual
+  // CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}}  col:23 'float'
+  // CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}}  col:29 'int' cinit
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 12
+
+  virtual void g() = 0;
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:16 g 
'void ()' virtual pure
+
+  // CHECK: CXXConstructorDecl 0x{{[^ ]*}}  col:8 
implicit S 'void (const S &)' inline default_delete noexcept-unevaluated
+  // CHECK: CXXConstructorDecl 0x{{[^ ]*}}  col:8 implicit constexpr S 
'void (S &&)' inline default noexcept-unevaluated
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:8 implicit operator= 'S 
&(const S &)' inline default_delete noexcept-unevaluated
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:8 implicit operator= 'S &(S 
&&)' inline default_delete noexcept-unevaluated
+  // CHECK: CXXDestructorDecl 0x{{[^ ]*}}  col:8 implicit ~S 'void ()' 
inline default noexcept-unevaluated
+};
+
+struct T : S { // T is not referenced, but S is
+  void f(float, int = 100) override;
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:8 f 
'void (float, int)'
+  // CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}}  col:15 'float'
+  // CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}}  col:21 'int' cinit
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 100
+  // CHECK-NEXT: Overrides: [ 0x{{[^ ]*}} S::f 'void (float, int)' ]
+  // CHECK-NEXT: OverrideAttr
+
+  // CHECK: CXXConstructorDecl 0x{{[^ ]*}}  col:8 implicit 
T 'void (const T &)' inline default_delete noexcept-unevaluated
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:8 implicit operator= 'T 
&(const T &)' inline default_delete noexcept-unevaluated
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:8 implicit operator= 'T &(T 
&&)' inline default_delete noexcept-unevaluated
+  // CHECK: CXXDestructorDecl 0x{{[^ ]*}}  col:8 implicit ~T 'void ()' 
inline default noexcept-unevaluated
+};
+
+struct U {
+  void f();
+  // CHECK: CXXMethodDecl 0x{{[^ ]*}}  col:8 f 
'void ()'
+};
+void U::f() {} // parent
+// CHECK: CXXMethodDecl 0x{{[^ ]*}} parent 0x{{[^ ]*}} prev 0x{{[^ ]*}} 
 col:9 f 'void ()'
+// CHECK-NEXT: CompoundStmt 0x{{[^ ]*}} 
+
+void a1();
+// CHECK: FunctionDecl 0x{{[^ ]*}}  col:6 used a1 
'void ()'
+void a2(void);
+// CHECK: FunctionDecl 0x{{[^ ]*}}  col:6 a2 'void 
()'
+void b(int a, int b);
+// CHECK: FunctionDecl 0x{{[^ ]*}}  col:6 b 'void 
(int, int)'
+// CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}}  col:12 a 'int'
+// CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}}  col:19 b 'int'
+void c(int a, int b = 12);
+// CHECK: FunctionDecl 0x{{[^ ]*}}  col:6 c 'void 
(int, int)'
+// CHECK-NEXT: ParmVarDecl 0x{{[

[PATCH] D55083: Re-arrange content in FunctionDecl dump

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D55083#1314546 , @steveire wrote:

> Yes, please commit your new tests for FunctionDecl dumping before this patch 
> can go in.


r347994 has those tests.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55083



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

r347996 - Adding tests for -ast-dump; NFC.

[Aaron Ballman via cfe-commits]

Author: aaronballman
Date: Fri Nov 30 07:11:16 2018
New Revision: 347996

URL: http://llvm.org/viewvc/llvm-project?rev=347996&view=rev
Log:
Adding tests for -ast-dump; NFC.

This adds tests for DeclStmt and demonstrates that we don't create such an AST 
node for global declarations currently.

Added:
cfe/trunk/test/Misc/ast-dump-decl-stmts.cpp

Added: cfe/trunk/test/Misc/ast-dump-decl-stmts.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/ast-dump-decl-stmts.cpp?rev=347996&view=auto
==
--- cfe/trunk/test/Misc/ast-dump-decl-stmts.cpp (added)
+++ cfe/trunk/test/Misc/ast-dump-decl-stmts.cpp Fri Nov 30 07:11:16 2018
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ast-dump %s | FileCheck 
-strict-whitespace %s
+
+void test_func() {
+  int a, b, c;
+  // CHECK: DeclStmt 0x{{[^ ]*}} 
+  // CHECK-NEXT: VarDecl 0x{{[^ ]*}}  col:7 a 'int'
+  // CHECK-NEXT: VarDecl 0x{{[^ ]*}}  col:10 b 'int'
+  // CHECK-NEXT: VarDecl 0x{{[^ ]*}}  col:13 c 'int'
+  void d(), e(int);
+  // CHECK: DeclStmt 0x{{[^ ]*}} 
+  // CHECK-NEXT: FunctionDecl 0x{{[^ ]*}} parent 0x{{[^ ]*}}  
col:8 d 'void ()'
+  // CHECK-NEXT: FunctionDecl 0x{{[^ ]*}} parent 0x{{[^ ]*}}  
col:13 e 'void (int)'
+  // CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}}  col:18 'int'
+  int f;
+  // CHECK: DeclStmt 0x{{[^ ]*}} 
+  // CHECK-NEXT: VarDecl 0x{{[^ ]*}}  col:7 f 'int'
+}
+
+// FIXME: These currently do not show up as a DeclStmt.
+int a, b, c;
+// CHECK: VarDecl 0x{{[^ ]*}}  col:5 a 'int'
+// CHECK-NEXT: VarDecl 0x{{[^ ]*}}  col:8 b 'int'
+// CHECK-NEXT: VarDecl 0x{{[^ ]*}}  col:11 c 'int'
+void d(), e(int);
+// CHECK: FunctionDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}}  col:6 d 'void ()'
+// CHECK-NEXT: FunctionDecl 0x{{[^ ]*}} prev 0x{{[^ ]*}}  
col:11 e 'void (int)'
+// CHECK-NEXT: ParmVarDecl 0x{{[^ ]*}}  col:16 'int'
+int f;
+// CHECK: VarDecl 0x{{[^ ]*}}  col:5 f 'int'
+


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

[PATCH] D55124: [CodeComplete] Cleanup access checking in code completion

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added a reviewer: ioeric.

Also fixes a crash (see the added 'accessibility-crash.cpp' test).


Repository:
  rC Clang

https://reviews.llvm.org/D55124

Files:
  include/clang/Sema/Sema.h
  lib/Sema/CodeCompleteConsumer.cpp
  lib/Sema/SemaAccess.cpp
  lib/Sema/SemaCodeComplete.cpp
  test/CodeCompletion/accessibility-crash.cpp
  test/CodeCompletion/accessibility.cpp

Index: test/CodeCompletion/accessibility.cpp
===
--- /dev/null
+++ test/CodeCompletion/accessibility.cpp
@@ -0,0 +1,73 @@
+class X {
+public:
+ int pub;
+protected:
+ int prot;
+private:
+ int priv;
+};
+
+class Unrelated {
+public:
+  static int pub;
+protected:
+  static int prot;
+private:
+  static int priv;
+};
+
+class Y : public X {
+  int test() {
+this->pub = 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:21:11 %s -o - \
+// RUN: | FileCheck -check-prefix=THIS %s
+// THIS: priv (InBase,Inaccessible)
+// THIS: prot (InBase)
+// THIS: pub (InBase)
+//
+// Also check implicit 'this->', i.e. complete at the start of the line.
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:21:1 %s -o - \
+// RUN: | FileCheck -check-prefix=THIS %s
+
+X().pub + 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:32:9 %s -o - \
+// RUN: | FileCheck -check-prefix=X-OBJ %s
+// X-OBJ: priv (Inaccessible)
+// X-OBJ: prot (Inaccessible)
+// X-OBJ: pub : [#int#]pub
+
+Y().pub + 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:39:9 %s -o - \
+// RUN: | FileCheck -check-prefix=Y-OBJ %s
+// Y-OBJ: priv (InBase,Inaccessible)
+// Y-OBJ: prot (InBase)
+// Y-OBJ: pub (InBase)
+
+this->X::pub = 10;
+X::pub = 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:46:14 %s -o - \
+// RUN: | FileCheck -check-prefix=THIS-BASE %s
+// THIS-BASE: priv (Inaccessible)
+// FIXME: this is handled incorrectly and needs a fix (prot is deemed inaccessible).
+// THIS-BASE: prot (Inaccessible)
+// THIS-BASE: pub : [#int#]pub
+//
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:47:8 %s -o - \
+// RUN: | FileCheck -check-prefix=THIS-BASE %s
+
+
+this->Unrelated::pub = 10; // a check we don't crash in this cases.
+Y().Unrelated::pub = 10; // a check we don't crash in this cases.
+Unrelated::pub = 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:59:22 %s -o - \
+// RUN: | FileCheck -check-prefix=UNRELATED %s
+// UNRELATED: priv (Inaccessible)
+// UNRELATED: prot (Inaccessible)
+// UNRELATED: pub : [#int#]pub
+//
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:60:20 %s -o - \
+// RUN: | FileCheck -check-prefix=UNRELATED %s
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:61:16 %s -o - \
+// RUN: | FileCheck -check-prefix=UNRELATED %s
+  }
+};
Index: test/CodeCompletion/accessibility-crash.cpp
===
--- /dev/null
+++ test/CodeCompletion/accessibility-crash.cpp
@@ -0,0 +1,23 @@
+class X {
+public:
+ int pub;
+protected:
+ int prot;
+private:
+ int priv;
+};
+
+class Y : public X {
+  int test() {
+[]() {
+
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:13:1 %s -o - \
+  // RUN: | FileCheck %s
+  // CHECK: priv (InBase,Inaccessible)
+  // CHECK: prot (InBase)
+  // CHECK: pub (InBase)
+};
+  }
+};
+
+
Index: lib/Sema/SemaCodeComplete.cpp
===
--- lib/Sema/SemaCodeComplete.cpp
+++ lib/Sema/SemaCodeComplete.cpp
@@ -1278,38 +1278,52 @@
 }
 
 namespace {
+
 /// Visible declaration consumer that adds a code-completion result
 /// for each visible declaration.
 class CodeCompletionDeclConsumer : public VisibleDeclConsumer {
   ResultBuilder &Results;
-  DeclContext *CurContext;
+  DeclContext *InitialLookupCtx;
+  // NamingClass and BaseType are used for access-checking. See
+  // Sema::IsSimplyAccessible for details.
+  CXXRecordDecl *NamingClass;
+  QualType BaseType;
   std::vector FixIts;
-  // This is set to the record where the search starts, if this is a record
-  // member completion.
-  RecordDecl *MemberCompletionRecord = nullptr;
 
 public:
   CodeCompletionDeclConsumer(
-  ResultBuilder &Results, DeclContext *CurContext,
-  std::vector FixIts = std::vector(),
-  RecordDecl *MemberCompletionRecord = nullptr)
-  : Results(Results), CurContext(CurContext), FixIts(std::move(FixIts)),
-MemberCompletionRecord(MemberCompletionRecord) {}
+  ResultBuilder &Results, DeclContext *InitialLookupCtx,
+  QualType BaseType = QualType(),
+  std::vector FixIts = std::vector())
+  : Results(Results), InitialLookupCtx(InitialLookupCtx),
+FixIts(std::move(FixIts)) {
+NamingClass = llvm::dyn_cast(InitialL

[PATCH] D52879: Derive builtin return type from its definition

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D52879#1311177 , @riccibruno wrote:

> And moreover I believe this change is subtly incorrect for the following 
> reason:
>  The type that is passed into the constructor of the call expression is the 
> type
>  of the call expression, and not the return type.
>
> The difference between the return type and the type of the call expression is 
> that
>  1.) References are dropped, and
>  2.) For C++: The cv-qualifiers of non class pr-values are dropped,
>  3.) For C: The cv-qualifiers are dropped.
>
> ( as can be seen in `FunctionType::getCallResultType` )


@mantognini Ping ? I agree with you that it would be best to derive the return 
type of builtins from
their definition, but as such I think that this change is incorrect for the 
reason given above.

What I would like to propose is to revert the changes to the code in `Sema`, 
but keep the tests.


Repository:
  rC Clang

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

https://reviews.llvm.org/D52879



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

[PATCH] D55054: [clang] Fill RealPathName for virtual files.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM and a few nits




Comment at: include/clang/Basic/FileManager.h:108
+
+  // Only for use in tests to see if deferred opens are happening, arther than
+  // relying on RealPathName being empty.

s/arther/rather
Let's leave out the RealPathName part, it seemed to be there for historical 
reasons.



Comment at: include/clang/Basic/FileManager.h:182
+  /// RealPathName.
+  bool fillRealPathName(FileEntry *UFE, llvm::StringRef FileName);
+

We never use the returned value. Maybe drop it?



Comment at: unittests/Basic/FileManagerTest.cpp:237
   ASSERT_TRUE(file->isValid());
   // "real path name" reveals whether the file was actually opened.
   EXPECT_EQ("", file->tryGetRealPathName());

kadircet wrote:
> ilya-biryukov wrote:
> > This test seems to rely on the old behavior. Is there a way to test the 
> > file wasn't open other than looking at RealPathName?
> There is also the vfs::file stored inside the FileEntry but unfortunately 
> there are no getters exposing this one. But I believe this behavior is still 
> correct for the getFile case, since `RealPathName` will only be filled when 
> we open the file.
Using `vfs::File` LG, thanks!



Comment at: unittests/Basic/FileManagerTest.cpp:253
   file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
-  EXPECT_EQ("", file->tryGetRealPathName());
 }

kadircet wrote:
> ilya-biryukov wrote:
> > We should find a way to test file was not "opened" (I think it actually 
> > does stat the file now, right?)
> AFAIK, any file accessed through getVirtualFile is immediately opened, then 
> later on accessing a file with the same name through getFile with 
> openfile=false won't have any affect, since this file will be alive and 
> marked as opened in the cache.
> 
> So I believe opening and stating is dual of each other. Therefore checking 
> for one should be equivalent to check for other.
I'm not sure about the semantics either, it seems we need to stat anyway, maybe 
open referes to reading the file contents in this case?
Anyhow, we're not actually changing semantics of the parts that were tested 
here, so there's no reason to go deeper into this.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55054



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

[PATCH] D54999: [clangd] Populate include graph during static indexing action.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

Thank, LGTM. A few NITs.




Comment at: clangd/index/IndexAction.h:31
+std::function RefsCallback,
+std::function IncludeGraphCallback = nullptr);
 

We have only two call sites, maybe leave out the default arg and specify null 
explicitly where needed?



Comment at: unittests/clangd/IndexActionTests.cpp:52
+EXPECT_EQ(Node.URI.data(), IndexFile.Sources->find(URI)->getKeyData());
+  }
+}

Maybe also check the size of `IndexFile.Sources` is the same as `Paths`? To 
make sure we didn't miss any


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54999



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

[PATCH] D55121: Make several Python scripts portable across Python2 and Python 3

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added a comment.

In general LGTM, as someone who's done a 2-3 conversion of similar scale before.

The only suggestion I'd make is to consider changing the `__future__` imports 
to `from __future__ import absolute_import, division, print_function`
I found this gave the best consistency between 2 & 3. Probably you don't need 
it at this point, but once the conversion is done there's less to go wrong in 
future.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55121



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

[PATCH] D55125: Fix a false positive in misc-redundant-expression check

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp created this revision.
dkrupp added reviewers: alexfh, aaron.ballman, gamesh411.
dkrupp added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, Szelethus, rnkovacs.

Do not warn for redundant conditional expressions
when the true and false branches are expanded from
different macros even when they are
defined by one another.

We used to get a false warning in the following case:
define M1  1
#define M2 M1 
int test(int cond){
 return cond ? M1  : M2;//false warning: 'true' 
and 'false' expressions are equivalent
}

The problem was that the Lexer::getImmediateMacroName() call was used for 
comparing macros, which returned "M1 " for the 
expansion of M1  and M2.
Now we are comparing macros from token to token.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D55125

Files:
  clang-tidy/misc/RedundantExpressionCheck.cpp
  test/clang-tidy/misc-redundant-expression.cpp


Index: test/clang-tidy/misc-redundant-expression.cpp
===
--- test/clang-tidy/misc-redundant-expression.cpp
+++ test/clang-tidy/misc-redundant-expression.cpp
@@ -106,6 +106,7 @@
 
 #define COND_OP_MACRO 9
 #define COND_OP_OTHER_MACRO 9
+#define COND_OP_THIRD_MACRO COND_OP_MACRO
 int TestConditional(int x, int y) {
   int k = 0;
   k += (y < 0) ? x : x;
@@ -114,11 +115,15 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: 'true' and 'false' expressions 
are equivalent
   k += (y < 0) ? COND_OP_MACRO : COND_OP_MACRO;
   // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: 'true' and 'false' expressions 
are equivalent
+  k += (y < 0) ? COND_OP_MACRO + COND_OP_OTHER_MACRO : COND_OP_MACRO + 
COND_OP_OTHER_MACRO;
+  // CHECK-MESSAGES: :[[@LINE-1]]:54: warning: 'true' and 'false' expressions 
are equivalent
 
   // Do not match for conditional operators with a macro and a const.
   k += (y < 0) ? COND_OP_MACRO : 9;
   // Do not match for conditional operators with expressions from different 
macros.
   k += (y < 0) ? COND_OP_MACRO : COND_OP_OTHER_MACRO;
+  // Do not match for conditional operators when a macro is defined to another 
macro
+  k += (y < 0) ? COND_OP_MACRO : COND_OP_THIRD_MACRO;
   return k;
 }
 #undef COND_OP_MACRO
Index: clang-tidy/misc/RedundantExpressionCheck.cpp
===
--- clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -598,23 +598,55 @@
   return true;
 }
 
+static bool compareToks(Token &T1, Token &T2, const SourceManager &SM){
+  if (T1.getLength()!=T2.getLength())
+return false;
+  return 
strncmp(SM.getCharacterData(T1.getLocation()),SM.getCharacterData(T2.getLocation()),T1.getLength())==0;
+}
+
+//Returns true if both LhsEpxr and RhsExpr are
+//macro expressions and they are expanded
+//from different macros.
 static bool areExprsFromDifferentMacros(const Expr *LhsExpr,
 const Expr *RhsExpr,
 const ASTContext *AstCtx) {
   if (!LhsExpr || !RhsExpr)
 return false;
-
-  SourceLocation LhsLoc = LhsExpr->getExprLoc();
-  SourceLocation RhsLoc = RhsExpr->getExprLoc();
-
-  if (!LhsLoc.isMacroID() || !RhsLoc.isMacroID())
+  SourceRange Lsr = LhsExpr->getSourceRange();
+  SourceRange Rsr = RhsExpr->getSourceRange();
+  if (!Lsr.getBegin().isMacroID() || !Rsr.getBegin().isMacroID())
 return false;
-
   const SourceManager &SM = AstCtx->getSourceManager();
   const LangOptions &LO = AstCtx->getLangOpts();
 
-  return !(Lexer::getImmediateMacroName(LhsLoc, SM, LO) ==
-  Lexer::getImmediateMacroName(RhsLoc, SM, LO));
+  std::pair LsrLocInfo =
+  SM.getDecomposedLoc(SM.getExpansionLoc(Lsr.getBegin()));
+  std::pair RsrLocInfo =
+  SM.getDecomposedLoc(SM.getExpansionLoc(Rsr.getBegin()));
+  const llvm::MemoryBuffer *MB = SM.getBuffer(LsrLocInfo.first);
+
+  const char *LTokenPos = MB->getBufferStart() + LsrLocInfo.second;
+  const char *RTokenPos = MB->getBufferStart() + RsrLocInfo.second;
+  clang::Lexer LRawLex(SM.getLocForStartOfFile(LsrLocInfo.first), LO,
+   MB->getBufferStart(), LTokenPos, MB->getBufferEnd());
+  clang::Lexer RRawLex(SM.getLocForStartOfFile(RsrLocInfo.first), LO,
+   MB->getBufferStart(), RTokenPos, MB->getBufferEnd());
+
+  clang::Token LTok, RTok;
+  SourceLocation LLoc, RLoc;
+  int LRem, RRem;
+  do {//we compare the expressions token-by-token
+LRawLex.LexFromRawLexer(LTok);
+RRawLex.LexFromRawLexer(RTok);
+LLoc = LTok.getLocation();
+RLoc = RTok.getLocation();
+LRem = SM.getCharacterData(SM.getExpansionLoc(Lsr.getEnd())) -
+   SM.getCharacterData(LLoc);//remaining characters until the end of 
expr
+RRem = SM.getCharacterData(SM.getExpansionLoc(Rsr.getEnd())) -
+   SM.getCharacterD

[PATCH] D52879: Derive builtin return type from its definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

In D52879#1311177 , @riccibruno wrote:

> And moreover I believe this change is subtly incorrect for the following 
> reason:
>  The type that is passed into the constructor of the call expression is the 
> type
>  of the call expression, and not the return type.
>
> The difference between the return type and the type of the call expression is 
> that
>  1.) References are dropped, and
>  2.) For C++: The cv-qualifiers of non class pr-values are dropped,
>  3.) For C: The cv-qualifiers are dropped.
>
> ( as can be seen in `FunctionType::getCallResultType` )


Could you clarify one thing for me: If I understood what you were saying, the 
type passed to CallExpr should not be ref-cv-qualified, is that right?

In that case, couldn't we "simply" remove the ref + CV-qualifiers from 
ReturnTy? (`getNonReferenceType().withoutLocalFastQualifiers()`)

Note: I cannot revert this //and// keep the test because the new ones will fail.


Repository:
  rC Clang

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

https://reviews.llvm.org/D52879



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

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 176130.
ebevhan added a comment.

Rebased and addressed review comments.


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

https://reviews.llvm.org/D51211

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/format-strings-bitfield-promotion.c


Index: test/Sema/format-strings-bitfield-promotion.c
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.c
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+int printf(const char *restrict, ...);
+
+struct bitfields {
+  long a : 2;
+  unsigned long b : 2;
+  long c : 32;  // assumes that int is 32 bits
+  unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+  printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but 
the argument has type 'int'}}
+  printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned 
long' but the argument has type 'int'}}
+  printf("%ld", bf.c); // expected-warning {{format specifies type 'long' but 
the argument has type 'int'}}
+  printf("%lu", bf.d); // expected-warning {{format specifies type 'unsigned 
long' but the argument has type 'unsigned int'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -7697,6 +7697,24 @@
   return std::make_pair(QualType(), StringRef());
 }
 
+/// Return true if \p ICE is an implicit argument promotion of an arithmetic
+/// type. Bit-field 'promotions' from a higher ranked type to a lower ranked
+/// type do not count.
+static bool
+isArithmeticArgumentPromotion(Sema &S, const ImplicitCastExpr *ICE) {
+  QualType From = ICE->getSubExpr()->getType();
+  QualType To = ICE->getType();
+  // It's a floating promotion if the source type is a lower rank.
+  if (ICE->getCastKind() == CK_FloatingCast &&
+  S.Context.getFloatingTypeOrder(From, To) < 0)
+return true;
+  // It's an integer promotion if the destination type is the promoted
+  // source type.
+  return ICE->getCastKind() == CK_IntegralCast &&
+ From->isPromotableIntegerType() &&
+ S.Context.getPromotedIntegerType(From) == To;
+}
+
 bool
 CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
 const char *StartSpecifier,
@@ -7724,11 +7742,11 @@
 
   // Look through argument promotions for our error message's reported type.
   // This includes the integral and floating promotions, but excludes array
-  // and function pointer decay; seeing that an argument intended to be a
-  // string has type 'char [6]' is probably more confusing than 'char *'.
+  // and function pointer decay (seeing that an argument intended to be a
+  // string has type 'char [6]' is probably more confusing than 'char *') and
+  // certain bitfield promotions (bitfields can be 'demoted' to a lesser type).
   if (const ImplicitCastExpr *ICE = dyn_cast(E)) {
-if (ICE->getCastKind() == CK_IntegralCast ||
-ICE->getCastKind() == CK_FloatingCast) {
+if (isArithmeticArgumentPromotion(S, ICE)) {
   E = ICE->getSubExpr();
   ExprTy = E->getType();
 


Index: test/Sema/format-strings-bitfield-promotion.c
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.c
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+int printf(const char *restrict, ...);
+
+struct bitfields {
+  long a : 2;
+  unsigned long b : 2;
+  long c : 32;  // assumes that int is 32 bits
+  unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+  printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but the argument has type 'int'}}
+  printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned long' but the argument has type 'int'}}
+  printf("%ld", bf.c); // expected-warning {{format specifies type 'long' but the argument has type 'int'}}
+  printf("%lu", bf.d); // expected-warning {{format specifies type 'unsigned long' but the argument has type 'unsigned int'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -7697,6 +7697,24 @@
   return std::make_pair(QualType(), StringRef());
 }
 
+/// Return true if \p ICE is an implicit argument promotion of an arithmetic
+/// type. Bit-field 'promotions' from a higher ranked type to a lower ranked
+/// type do not count.
+static bool
+isArithmeticArgumentPromotion(Sema &S, const ImplicitCastExpr *ICE) {
+  QualType From = ICE->getSubExpr()->getType();
+  QualType To = ICE->getType();
+  // It's a floating promotion if the source type is a lower rank.
+  if (ICE->getCastKind() == CK_FloatingCast &

[PATCH] D55127: [OpenCL] Diagnose conflicting address spaces between template definition and its instantiation

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia created this revision.
Anastasia added a reviewer: rjmccall.
Herald added a subscriber: yaxunl.

Added new diagnostics when templates are instantiated with different address 
space from the one provided in definition.

This also prevents deducing generic address space in pointer type of templates 
to allow giving them concrete address space.


https://reviews.llvm.org/D55127

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaType.cpp
  lib/Sema/TreeTransform.h
  test/SemaOpenCLCXX/address-space-templates.cl

Index: test/SemaOpenCLCXX/address-space-templates.cl
===
--- test/SemaOpenCLCXX/address-space-templates.cl
+++ test/SemaOpenCLCXX/address-space-templates.cl
@@ -7,6 +7,25 @@
   void f2(T); // expected-error{{parameter may not be qualified with an address space}}
 };
 
+template 
+T foo1(__global T *i) { // expected-note{{candidate template ignored: substitution failure [with T = __local int]: conflicting address space qualifiers are provided between types '__global T' and '__local int'}}
+  return *i;
+}
+
+template 
+T *foo2(T *i) {
+  return i;
+}
+
+template 
+void foo3() {
+  __private T ii; // expected-error{{conflicting address space qualifiers are provided between types 'T' and '__global int'}}
+}
+
 void bar() {
   S sintgl; // expected-note{{in instantiation of template class 'S' requested here}}
+
+  foo1<__local int>(1); // expected-error{{no matching function for call to 'foo1'}}
+  foo2<__global int>(0);
+  foo3<__global int>(); // expected-note{{in instantiation of function template specialization 'foo3<__global int>' requested here}}
 }
Index: lib/Sema/TreeTransform.h
===
--- lib/Sema/TreeTransform.h
+++ lib/Sema/TreeTransform.h
@@ -684,15 +684,13 @@
   OMPClause *Transform ## Class(Class *S);
 #include "clang/Basic/OpenMPKinds.def"
 
-  /// Build a new qualified type given its unqualified type and type
-  /// qualifiers.
+  /// Build a new qualified type given its unqualified type and type location.
   ///
   /// By default, this routine adds type qualifiers only to types that can
   /// have qualifiers, and silently suppresses those qualifiers that are not
   /// permitted. Subclasses may override this routine to provide different
   /// behavior.
-  QualType RebuildQualifiedType(QualType T, SourceLocation Loc,
-Qualifiers Quals);
+  QualType RebuildQualifiedType(QualType T, QualifiedTypeLoc TL);
 
   /// Build a new pointer type given its pointee type.
   ///
@@ -4228,8 +4226,9 @@
 return nullptr;
 
   if (QTL) {
-Result = getDerived().RebuildQualifiedType(
-Result, QTL.getBeginLoc(), QTL.getType().getLocalQualifiers());
+Result = getDerived().RebuildQualifiedType(Result, QTL);
+if (Result.isNull())
+  return nullptr;
 TLB.TypeWasModifiedSafely(Result);
   }
 
@@ -4246,7 +4245,10 @@
   if (Result.isNull())
 return QualType();
 
-  Result = getDerived().RebuildQualifiedType(Result, T.getBeginLoc(), Quals);
+  Result = getDerived().RebuildQualifiedType(Result, T);
+
+  if (Result.isNull())
+return QualType();
 
   // RebuildQualifiedType might have updated the type, but not in a way
   // that invalidates the TypeLoc. (There's no location information for
@@ -4256,10 +4258,21 @@
   return Result;
 }
 
-template
+template 
 QualType TreeTransform::RebuildQualifiedType(QualType T,
-  SourceLocation Loc,
-  Qualifiers Quals) {
+  QualifiedTypeLoc TL) {
+
+  SourceLocation Loc = TL.getBeginLoc();
+  Qualifiers Quals = TL.getType().getLocalQualifiers();
+
+  if (((T.getAddressSpace() != LangAS::Default &&
+Quals.getAddressSpace() != LangAS::Default)) &&
+  T.getAddressSpace() != Quals.getAddressSpace()) {
+SemaRef.Diag(Loc, diag::err_address_space_mismatch_templ_inst)
+<< TL.getType() << T;
+return QualType();
+  }
+
   // C++ [dcl.fct]p7:
   //   [When] adding cv-qualifications on top of the function type [...] the
   //   cv-qualifiers are ignored.
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -7201,7 +7201,9 @@
!IsPointee) ||
   // Do not deduce addr space of the void type, e.g. in f(void), otherwise
   // it will fail some sema check.
-  (T->isVoidType() && !IsPointee))
+  (T->isVoidType() && !IsPointee) ||
+  // Do not deduce address spaces in templates.
+  T->isDependentType())
 return;
 
   LangAS ImpAddr = LangAS::Default;
@@ -7226,10 +7228,10 @@
 if (IsPointee) {
   ImpAddr = LangAS::opencl_generic;
 } else {
-  if (D.getContext() == DeclaratorContext::TemplateArgContext ||
-  T->isDependentType()) {
-// Do no

[PATCH] D55101: [clang-tidy] Ignore namespaced and C++ member functions in google-objc-function-naming check 🙈

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton added a comment.

IMHO we should just disable this check entirely for C++ files (including 
Objective-C++).

Since Objective-C++ files will have a mix of the Google Objective-C and Google 
C++ styles, I think there will be too many places where we'll hit conflicts 
like this (there are likely more).


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55101



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

[PATCH] D55128: [CMake] Store path to vendor-specific headers in clang-headers target property

[Stefan Gränitz via Phabricator via cfe-commits]

sgraenitz created this revision.
sgraenitz added reviewers: aprantl, JDevlieghere, davide, friss, dexonsmith.
Herald added a subscriber: mgorny.

LLDB.framework wants a copy these headers. With this change LLDB can easily 
glob for the list of files:

  get_target_property(clang_include_dir clang-headers RUNTIME_OUTPUT_DIRECTORY)
  file(GLOB_RECURSE clang_vendor_headers RELATIVE ${clang_include_dir} 
"${clang_include_dir}/*")

By default `RUNTIME_OUTPUT_DIRECTORY` is unset for custom targets like 
`clang-headers`.
Not sure if there is a read&write property that matches the purpose better.

What do you think?


Repository:
  rC Clang

https://reviews.llvm.org/D55128

Files:
  lib/Headers/CMakeLists.txt


Index: lib/Headers/CMakeLists.txt
===
--- lib/Headers/CMakeLists.txt
+++ lib/Headers/CMakeLists.txt
@@ -144,7 +144,7 @@
   list(APPEND out_files ${dst})
 endforeach( f )
 
-add_custom_command(OUTPUT ${output_dir}/arm_neon.h 
+add_custom_command(OUTPUT ${output_dir}/arm_neon.h
   DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h
   COMMAND ${CMAKE_COMMAND} -E copy_if_different 
${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h ${output_dir}/arm_neon.h
   COMMENT "Copying clang's arm_neon.h...")
@@ -156,7 +156,9 @@
 list(APPEND out_files ${output_dir}/arm_fp16.h)
 
 add_custom_target(clang-headers ALL DEPENDS ${out_files})
-set_target_properties(clang-headers PROPERTIES FOLDER "Misc")
+set_target_properties(clang-headers PROPERTIES
+  FOLDER "Misc"
+  RUNTIME_OUTPUT_DIRECTORY "${output_dir}")
 
 install(
   FILES ${files} ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h


Index: lib/Headers/CMakeLists.txt
===
--- lib/Headers/CMakeLists.txt
+++ lib/Headers/CMakeLists.txt
@@ -144,7 +144,7 @@
   list(APPEND out_files ${dst})
 endforeach( f )
 
-add_custom_command(OUTPUT ${output_dir}/arm_neon.h 
+add_custom_command(OUTPUT ${output_dir}/arm_neon.h
   DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h
   COMMAND ${CMAKE_COMMAND} -E copy_if_different ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h ${output_dir}/arm_neon.h
   COMMENT "Copying clang's arm_neon.h...")
@@ -156,7 +156,9 @@
 list(APPEND out_files ${output_dir}/arm_fp16.h)
 
 add_custom_target(clang-headers ALL DEPENDS ${out_files})
-set_target_properties(clang-headers PROPERTIES FOLDER "Misc")
+set_target_properties(clang-headers PROPERTIES
+  FOLDER "Misc"
+  RUNTIME_OUTPUT_DIRECTORY "${output_dir}")
 
 install(
   FILES ${files} ${CMAKE_CURRENT_BINARY_DIR}/arm_neon.h
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55129: [CTU] Eliminate race condition in CTU lit tests

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: xazax.hun, a_sidorin.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs.

We plan to introduce additional CTU related lit test. Since lit may run the
tests in parallel, it is not safe to use the same directory (%T) for these
tests. It is safe to use however test case specific directories (%t).


Repository:
  rC Clang

https://reviews.llvm.org/D55129

Files:
  test/Analysis/ctu-main.cpp


Index: test/Analysis/ctu-main.cpp
===
--- test/Analysis/ctu-main.cpp
+++ test/Analysis/ctu-main.cpp
@@ -1,8 +1,8 @@
-// RUN: mkdir -p %T/ctudir
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o 
%T/ctudir/ctu-other.cpp.ast %S/Inputs/ctu-other.cpp
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o 
%T/ctudir/ctu-chain.cpp.ast %S/Inputs/ctu-chain.cpp
-// RUN: cp %S/Inputs/externalFnMap.txt %T/ctudir/
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -analyze 
-analyzer-checker=core,debug.ExprInspection -analyzer-config 
experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%T/ctudir 
-verify %s
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o 
%t/ctudir/ctu-other.cpp.ast %S/Inputs/ctu-other.cpp
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o 
%t/ctudir/ctu-chain.cpp.ast %S/Inputs/ctu-chain.cpp
+// RUN: cp %S/Inputs/externalFnMap.txt %t/ctudir/
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -analyze 
-analyzer-checker=core,debug.ExprInspection -analyzer-config 
experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%t/ctudir 
-verify %s
 
 #include "ctu-hdr.h"
 


Index: test/Analysis/ctu-main.cpp
===
--- test/Analysis/ctu-main.cpp
+++ test/Analysis/ctu-main.cpp
@@ -1,8 +1,8 @@
-// RUN: mkdir -p %T/ctudir
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o %T/ctudir/ctu-other.cpp.ast %S/Inputs/ctu-other.cpp
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o %T/ctudir/ctu-chain.cpp.ast %S/Inputs/ctu-chain.cpp
-// RUN: cp %S/Inputs/externalFnMap.txt %T/ctudir/
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-config experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%T/ctudir -verify %s
+// RUN: mkdir -p %t/ctudir
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o %t/ctudir/ctu-other.cpp.ast %S/Inputs/ctu-other.cpp
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o %t/ctudir/ctu-chain.cpp.ast %S/Inputs/ctu-chain.cpp
+// RUN: cp %S/Inputs/externalFnMap.txt %t/ctudir/
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-config experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%t/ctudir -verify %s
 
 #include "ctu-hdr.h"
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55128: [CMake] Store path to vendor-specific headers in clang-headers target property

[Jonas Devlieghere via Phabricator via cfe-commits]

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

This makes sense to me. I'm don't know if there's a better property but I think 
this matches the intended use, so I think it is fine.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55128



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

LGTM, could you please run this check over real world code and check that there 
are no obvious false positives?




Comment at: docs/clang-tidy/checks/bugprone-branch-clone.rst:10
+
+  .. code-block:: c++
+

please do not indent the `.. code-block:: c++` but just the code itself, 
(following too)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757



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

[PATCH] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

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

Szelethus added a comment.

@JonasToth this is the `Lexer` based expression equality check I talked about 
in D54757#1311516 .  The point of this 
patch is that the definition is a macro sure could be build dependent, but the 
macro name is not, so it wouldn't warn on the case I showed. What do you think?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55125



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

[PATCH] D54737: [clang-tidy] Add the abseil-duration-comparison check

[Jonas Toth via Phabricator via cfe-commits]

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

Only the test and your opinion on the `Optional` thing, If you want to keep it 
that way its fine.

LGTM afterwards :)




Comment at: clang-tidy/abseil/DurationFactoryFloatCheck.cpp:61
+
+  if (SimpleArg) {
 diag(MatchedCall->getBeginLoc(),

hwright wrote:
> JonasToth wrote:
> > hwright wrote:
> > > JonasToth wrote:
> > > > hwright wrote:
> > > > > JonasToth wrote:
> > > > > > The diagnostic is not printed if for some reason the fixit was not 
> > > > > > creatable. I think that the warning could still be emitted (`auto 
> > > > > > Diag = diag(...); if (Fix) Diag << Fixit::-...`)
> > > > > I'm not sure under which conditions you'd expect this to be an issue. 
> > > > >  Could you give me an example?
> > > > > 
> > > > > My expectation is that if we don't get a value back in `SimpleArg`, 
> > > > > we don't have anything to change, so there wouldn't be a warning to 
> > > > > emit.
> > > > I don't expect this to fail, failure there would mean a bug i guess. 
> > > > Having bugs is somewhat expected :)
> > > > And that would be our way to find the bug, because some user reports 
> > > > that there is no transformation done, that is my motivation behind that.
> > > > 
> > > > The warning itself should be correct, otherwise the matcher does not 
> > > > work, right? This would just be precaution.
> > > I guess what I'm saying is that I'm not sure what kind of warning we'd 
> > > give if we weren't able to offer a fix.  The optionality here means that 
> > > we found a result which was already as simple as we can make it, so 
> > > there's no reason to bother the user.
> > Lets say the result comes out of arithmetic and then there is a comparison 
> > (would be good test btw :)), the warning is still valueable, even if the 
> > transformation fails for some reason. The transformation could fail as 
> > well, because macros interfere or so. Past experience tells me, there are 
> > enough language constructs to break everything in weird combinations :)
> I can buy that, but I'm still having trouble seeing the specifics.  Do you 
> have a concrete example?
No i can't think of one right now. Its more a general argument, that the 
code-layout logically allows that only the transformation fails, even if the 
detection succeeds. No strong opinion, but preference :)



Comment at: test/clang-tidy/abseil-duration-comparison.cpp:148
+  // CHECK-FIXES: absl::Minutes(x) <= d1;
+  b = x < absl::ToInt64Hours(d1);
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: perform comparison in the 
duration domain [abseil-duration-comparison]

please add a test where the `int` part is result of a complex arithmetic 
expression.


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

https://reviews.llvm.org/D54737



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

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

[Scott Linder via Phabricator via cfe-commits]

scott.linder added a comment.

In D53153#1291348 , @rjmccall wrote:

> In D53153#1289380 , @scott.linder 
> wrote:
>
> > I don't believe that is currently the case (the unrestricted linking of OCL 
> > code to OCL code via a dynamic linker), but we do have the notion of a 
> > static link step, followed by dynamic linking at runtime. The static link 
> > step is currently via IR, but we plan to support linking object files. 
> > Maybe I misunderstand the distinction between linkage and visibility, but 
> > it seems reasonable that a user would want to have e.g. a non-kernel 
> > function participate in static linking, but not be preemptible in the final 
> > shared object. The intention with this patch is to allow this with 
> > something like `-fvisibility hidden` without disrupting kernel symbols, 
> > which must appear in the dynsym for the reasons mentioned earlier in the 
> > thread.
>
>
> Okay, this still doesn't need user-controlled symbol visibility.  The basic 
> question here is whether there is any need for anything other than kernel 
> functions to have dynamic linkage.  If not, then you really just need to mark 
> everything as hidden except for kernels.  You *could* do that marking in 
> Sema, but frankly that seems like an implementation detail escaping into the 
> AST, and it's just going to cause you unwanted pain and sub-optimality.  (For 
> example: `-fvisibility=hidden` doesn't actually change the visibility of 
> external declarations for reasons that make a lot of sense for 
> general-purpose environments but probably don't matter to you at all.  If 
> you're doing this to get better code-generation for references to external 
> entities that are going to end up within the image, you actually need to add 
> a visibility attribute to every single top-level declaration to get that 
> effect.  It's much easier to just do it in a pass on the module.)


If such a pass were to mark all non-kernel symbols as hidden, for example, how 
would it support languages other than OpenCL where the runtime constraints 
differ? AMDGPU supports other languages where the assumption that only kernel 
functions must be visible is not valid. Can a module pass override visibility 
without making assumptions about the source language? I may just be unfamiliar 
with Clang, and not know where this sort of pass would live.

I think a pass which only overrides kernel function visibility (i.e. by forcing 
it to be default) would always be correct, but touching other symbol visibility 
begins to make assumptions which are not necessarily valid for all languages.


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

https://reviews.llvm.org/D53153



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

[PATCH] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D55125#1314741 , @Szelethus wrote:

> @JonasToth this is the `Lexer` based expression equality check I talked about 
> in D54757#1311516 .  The point of 
> this patch is that the definition is a macro sure could be build dependent, 
> but the macro name is not, so it wouldn't warn on the case I showed. What do 
> you think?


Yes, this approach is possible.
IMHO it is still a bug/redudant if you do the same thing on both paths and 
warning on it makes the programmer aware of the fact. E.g. the macros might 
have been something different before, but a refactoring made them equal and 
resulted in this situation.




Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:601
 
+static bool compareToks(Token &T1, Token &T2, const SourceManager &SM){
+  if (T1.getLength()!=T2.getLength())

Please run clang-format



Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:630
+  const char *RTokenPos = MB->getBufferStart() + RsrLocInfo.second;
+  clang::Lexer LRawLex(SM.getLocForStartOfFile(LsrLocInfo.first), LO,
+   MB->getBufferStart(), LTokenPos, MB->getBufferEnd());

`clang::` should not be necessary here, is it? `SourceLocation` and other types 
are access without namespace, too.



Comment at: test/clang-tidy/misc-redundant-expression.cpp:109
 #define COND_OP_OTHER_MACRO 9
+#define COND_OP_THIRD_MACRO COND_OP_MACRO
 int TestConditional(int x, int y) {

Could you please add a test where the macro is `undef`ed and redefined to 
something else?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55125



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

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

ebevhan wrote:
> rjmccall wrote:
> > Hmm.  So adding a signed integer to an unsigned fixed-point type always 
> > converts the integer to unsigned?  That's a little weird, but only because 
> > the fixed-point rule seems to be the other way.  Anyway, I assume it's not 
> > a bug in the spec.
> `handleFixedPointConversion` only calculates the result type of the 
> expression, not the calculation type. The final result type of the operation 
> will be the unsigned fixed-point type, but the calculation itself will be 
> done in a signed type with enough precision to represent both the signed 
> integer and the unsigned fixed-point type. 
> 
> Though, if the result ends up being negative, the final result is undefined 
> unless the type is saturating. I don't think there is a test for the 
> saturating case (signed integer + unsigned saturating fixed-point) in the 
> SaturatedAddition tests.
> `handleFixedPointConversion` only calculates the result type of the 
> expression, not the calculation type.

Right, I understand that, but the result type of the expression obviously 
impacts the expressive range of the result, since you can end up with a 
negative value.

Hmm.  With that said, if the general principle is to perform the operation with 
full precision on the original operand values, why are unsigned fixed-point 
operands coerced to their corresponding signed types *before* the operation?  
This is precision-limiting if the unsigned representation doesn't use a padding 
bit.  That seems like a bug in the spec.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53738



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

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D53153#1314777 , @scott.linder 
wrote:

> In D53153#1291348 , @rjmccall wrote:
>
> > In D53153#1289380 , @scott.linder 
> > wrote:
> >
> > > I don't believe that is currently the case (the unrestricted linking of 
> > > OCL code to OCL code via a dynamic linker), but we do have the notion of 
> > > a static link step, followed by dynamic linking at runtime. The static 
> > > link step is currently via IR, but we plan to support linking object 
> > > files. Maybe I misunderstand the distinction between linkage and 
> > > visibility, but it seems reasonable that a user would want to have e.g. a 
> > > non-kernel function participate in static linking, but not be preemptible 
> > > in the final shared object. The intention with this patch is to allow 
> > > this with something like `-fvisibility hidden` without disrupting kernel 
> > > symbols, which must appear in the dynsym for the reasons mentioned 
> > > earlier in the thread.
> >
> >
> > Okay, this still doesn't need user-controlled symbol visibility.  The basic 
> > question here is whether there is any need for anything other than kernel 
> > functions to have dynamic linkage.  If not, then you really just need to 
> > mark everything as hidden except for kernels.  You *could* do that marking 
> > in Sema, but frankly that seems like an implementation detail escaping into 
> > the AST, and it's just going to cause you unwanted pain and sub-optimality. 
> >  (For example: `-fvisibility=hidden` doesn't actually change the visibility 
> > of external declarations for reasons that make a lot of sense for 
> > general-purpose environments but probably don't matter to you at all.  If 
> > you're doing this to get better code-generation for references to external 
> > entities that are going to end up within the image, you actually need to 
> > add a visibility attribute to every single top-level declaration to get 
> > that effect.  It's much easier to just do it in a pass on the module.)
>
>
> If such a pass were to mark all non-kernel symbols as hidden, for example, 
> how would it support languages other than OpenCL where the runtime 
> constraints differ? AMDGPU supports other languages where the assumption that 
> only kernel functions must be visible is not valid. Can a module pass 
> override visibility without making assumptions about the source language? I 
> may just be unfamiliar with Clang, and not know where this sort of pass would 
> live.
>
> I think a pass which only overrides kernel function visibility (i.e. by 
> forcing it to be default) would always be correct, but touching other symbol 
> visibility begins to make assumptions which are not necessarily valid for all 
> languages.


You still have the same linkage model for those other languages, right?  
Ultimately there's something like a kernel function that has to be declared 
specially and which becomes the only public interface of the code?


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

https://reviews.llvm.org/D53153



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

[PATCH] D55121: Make several Python scripts portable across Python2 and Python 3

[serge via Phabricator via cfe-commits]

serge-sans-paille updated this revision to Diff 176141.
serge-sans-paille added a comment.

Use generic ``from __future__ import `` line whenever it makes sense


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

https://reviews.llvm.org/D55121

Files:
  bindings/python/clang/cindex.py
  bindings/python/examples/cindex/cindex-dump.py
  bindings/python/tests/cindex/test_translation_unit.py
  docs/conf.py
  docs/tools/dump_format_style.py
  tools/clang-format/clang-format-diff.py
  tools/clang-format/clang-format-sublime.py
  tools/clang-format/clang-format.py
  tools/clang-format/git-clang-format
  tools/clang-rename/clang-rename.py
  tools/scan-build-py/libscanbuild/arguments.py
  tools/scan-view/share/Reporter.py
  utils/ABITest/ABITestGen.py
  utils/ABITest/Enumeration.py
  utils/CIndex/completion_logger_server.py
  utils/TestUtils/deep-stack.py
  utils/analyzer/SATestAdd.py
  utils/analyzer/SATestUpdateDiffs.py
  utils/analyzer/SumTimerInfo.py
  utils/check_cfc/check_cfc.py
  utils/check_cfc/obj_diff.py
  utils/check_cfc/setup.py
  utils/check_cfc/test_check_cfc.py
  utils/clangdiag.py
  utils/hmaptool/hmaptool
  utils/modfuzz.py
  utils/perf-training/perf-helper.py
  utils/token-delta.py
  www/builtins.py

Index: www/builtins.py
===
--- www/builtins.py
+++ www/builtins.py
@@ -151,7 +151,7 @@
   sys.stderr.write("%s:%d: x86 builtin %s used, too many replacements\n" % (fileinput.filename(), fileinput.filelineno(), builtin))
 
 for line in fileinput.input(inplace=1):
-  for builtin, repl in repl_map.iteritems():
+  for builtin, repl in repl_map.items():
 if builtin in line:
   line = line.replace(builtin, repl)
   report_repl(builtin, repl)
Index: utils/token-delta.py
===
--- utils/token-delta.py
+++ utils/token-delta.py
@@ -1,5 +1,6 @@
 #!/usr/bin/env python
 
+from __future__ import absolute_import, division, print_function
 import os
 import re
 import subprocess
@@ -37,7 +38,7 @@
 # O(N^2) case unless user requests it.
 if not force:
 if not self.getTestResult(changes):
-raise ValueError,'Initial test passed to delta fails.'
+raise ValueError('Initial test passed to delta fails.')
 
 # Check empty set first to quickly find poor test functions.
 if self.getTestResult(set()):
@@ -94,7 +95,7 @@
 
 ###
 
-class Token:
+class Token(object):
 def __init__(self, type, data, flags, file, line, column):
 self.type   = type
 self.data   = data
@@ -165,7 +166,7 @@
 byFile = self.writeFiles(changes, self.tempFiles)
 
 if self.log:
-print >>sys.stderr, 'TEST - ',
+print('TEST - ', end=' ', file=sys.stderr)
 if self.log > 1:
 for i,(file,_) in enumerate(self.tokenLists):
 indices = byFile[i]
@@ -184,8 +185,8 @@
 sys.stderr.write(str(byFile[i][-1]))
 sys.stderr.write('] ')
 else:
-print >>sys.stderr, ', '.join(['%s:%d tokens' % (file, len(byFile[i]))
-   for i,(file,_) in enumerate(self.tokenLists)]),
+print(', '.join(['%s:%d tokens' % (file, len(byFile[i]))
+   for i,(file,_) in enumerate(self.tokenLists)]), end=' ', file=sys.stderr)
 
 p = subprocess.Popen([self.testProgram] + self.tempFiles)
 res = p.wait() == 0
@@ -194,10 +195,10 @@
 self.writeFiles(changes, self.targetFiles)
 
 if self.log:
-print >>sys.stderr, '=> %s' % res
+print('=> %s' % res, file=sys.stderr)
 else:
 if res:
-print '\nSUCCESS (%d tokens)' % len(changes)
+print('\nSUCCESS (%d tokens)' % len(changes))
 else:
 sys.stderr.write('.')
 
@@ -209,7 +210,7 @@
   for j in range(len(tokens))])
 self.writeFiles(res, self.targetFiles)
 if not self.log:
-print >>sys.stderr
+print(file=sys.stderr)
 return res
 
 def tokenBasedMultiDelta(program, files, log):
@@ -218,15 +219,15 @@
   for file in files]
 
 numTokens = sum([len(tokens) for _,tokens in tokenLists])
-print "Delta on %s with %d tokens." % (', '.join(files), numTokens)
+print("Delta on %s with %d tokens." % (', '.join(files), numTokens))
 
 tbmd = TMBDDelta(program, tokenLists, log)
 
 res = tbmd.run()
 
-print "Finished %s with %d tokens (in %d tests)." % (', '.join(tbmd.targetFiles),
+print("Finished %s with %d tokens (in %d tests)." % (', '.join(tbmd.targetFiles),
  len(res),
- 

[PATCH] D52879: Derive builtin return type from its definition

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D52879#1314694 , @mantognini wrote:

> In D52879#1311177 , @riccibruno 
> wrote:
>
> > And moreover I believe this change is subtly incorrect for the following 
> > reason:
> >  The type that is passed into the constructor of the call expression is the 
> > type
> >  of the call expression, and not the return type.
> >
> > The difference between the return type and the type of the call expression 
> > is that
> >  1.) References are dropped, and
> >  2.) For C++: The cv-qualifiers of non class pr-values are dropped,
> >  3.) For C: The cv-qualifiers are dropped.
> >
> > ( as can be seen in `FunctionType::getCallResultType` )
>
>
> Could you clarify one thing for me: If I understood what you were saying, the 
> type passed to CallExpr should not be ref-cv-qualified, is that right?
>
> In that case, couldn't we "simply" remove the ref + CV-qualifiers from 
> ReturnTy? (`getNonReferenceType().withoutLocalFastQualifiers()`)
>
> Note: I cannot revert this //and// keep the test because the new ones will 
> fail.


It's not that simple unfortunately. The type passed to `CallExpr` is the type 
of the call expression,
which is different from the return type. However you can just use 
`FunctionType::getCallResultType`.




Comment at: lib/Sema/SemaExpr.cpp:5550
   ExprResult Result;
+  QualType ReturnTy;
   if (BuiltinID &&

Please rename this to something more appropriate,
like `ResultTy` or similar.



Comment at: lib/Sema/SemaExpr.cpp:5553
   Fn->getType()->isSpecificBuiltinType(BuiltinType::BuiltinFn)) {
-Result = ImpCastExprToType(Fn, Context.getPointerType(FDecl->getType()),
-   CK_BuiltinFnToFnPtr).get();
+// Extract the return type from the (builtin) function pointer type.
+auto FnPtrTy = Context.getPointerType(FDecl->getType());

Please leave a FIXME indicating that someone should
go through each of the builtins and remove the
various `setType` when appropriate.



Comment at: lib/Sema/SemaExpr.cpp:5554
+// Extract the return type from the (builtin) function pointer type.
+auto FnPtrTy = Context.getPointerType(FDecl->getType());
+Result = ImpCastExprToType(Fn, FnPtrTy, CK_BuiltinFnToFnPtr).get();

Don't use `auto` here.



Comment at: lib/Sema/SemaExpr.cpp:5556
+Result = ImpCastExprToType(Fn, FnPtrTy, CK_BuiltinFnToFnPtr).get();
+auto FnTy = FnPtrTy->getPointeeType()->castAs();
+ReturnTy = FnTy->getReturnType();

`const auto *` or just spell the type.


Repository:
  rC Clang

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

https://reviews.llvm.org/D52879



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

[clang-tools-extra] r348005 - [clangd] Populate include graph during static indexing action.

[Kadir Cetinkaya via cfe-commits]

Author: kadircet
Date: Fri Nov 30 08:59:00 2018
New Revision: 348005

URL: http://llvm.org/viewvc/llvm-project?rev=348005&view=rev
Log:
[clangd] Populate include graph during static indexing action.

Summary:
This is the second part for introducing include hierarchy into index
files produced by clangd. You can see the base patch that introduces structures
and discusses the future of the patches in D54817

Reviewers: ilya-biryukov

Subscribers: mgorny, ioeric, MaskRay, jkorous, arphaman, cfe-commits

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

Added:
clang-tools-extra/trunk/unittests/clangd/IndexActionTests.cpp
Modified:
clang-tools-extra/trunk/clangd/Headers.h
clang-tools-extra/trunk/clangd/index/Background.cpp
clang-tools-extra/trunk/clangd/index/IndexAction.cpp
clang-tools-extra/trunk/clangd/index/IndexAction.h
clang-tools-extra/trunk/clangd/indexer/IndexerMain.cpp
clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt

Modified: clang-tools-extra/trunk/clangd/Headers.h
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Headers.h?rev=348005&r1=348004&r2=348005&view=diff
==
--- clang-tools-extra/trunk/clangd/Headers.h (original)
+++ clang-tools-extra/trunk/clangd/Headers.h Fri Nov 30 08:59:00 2018
@@ -46,7 +46,7 @@ struct Inclusion {
   unsigned HashOffset = 0; // Byte offset from start of file to #.
   SrcMgr::CharacteristicKind FileKind = SrcMgr::C_User;
 };
-llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Inclusion&);
+llvm::raw_ostream &operator<<(llvm::raw_ostream &, const Inclusion &);
 
 // Contains information about one file in the build grpah and its direct
 // dependencies. Doesn't own the strings it references (IncludeGraph is
@@ -60,6 +60,8 @@ struct IncludeGraphNode {
 };
 // FileURI and FileInclusions are references to keys of the map containing
 // them.
+// Important: The graph generated by those callbacks might contain cycles, self
+// edges and multi edges.
 using IncludeGraph = llvm::StringMap;
 
 // Information captured about the inclusion graph in a translation unit.

Modified: clang-tools-extra/trunk/clangd/index/Background.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=348005&r1=348004&r2=348005&view=diff
==
--- clang-tools-extra/trunk/clangd/index/Background.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Background.cpp Fri Nov 30 08:59:00 2018
@@ -342,7 +342,8 @@ Error BackgroundIndex::index(tooling::Co
   RefSlab Refs;
   auto Action = createStaticIndexingAction(
   IndexOpts, [&](SymbolSlab S) { Symbols = std::move(S); },
-  [&](RefSlab R) { Refs = std::move(R); });
+  [&](RefSlab R) { Refs = std::move(R); },
+  /*IncludeGraphCallback=*/nullptr);
 
   // We're going to run clang here, and it could potentially crash.
   // We could use CrashRecoveryContext to try to make indexing crashes 
nonfatal,

Modified: clang-tools-extra/trunk/clangd/index/IndexAction.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/IndexAction.cpp?rev=348005&r1=348004&r2=348005&view=diff
==
--- clang-tools-extra/trunk/clangd/index/IndexAction.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/IndexAction.cpp Fri Nov 30 08:59:00 
2018
@@ -9,6 +9,98 @@ namespace clang {
 namespace clangd {
 namespace {
 
+llvm::Optional toURI(const FileEntry *File) {
+  if (!File)
+return llvm::None;
+  auto AbsolutePath = File->tryGetRealPathName();
+  if (AbsolutePath.empty())
+return llvm::None;
+  return URI::create(AbsolutePath).toString();
+}
+
+// Collects the nodes and edges of include graph during indexing action.
+// Important: The graph generated by those callbacks might contain cycles and
+// self edges.
+struct IncludeGraphCollector : public PPCallbacks {
+public:
+  IncludeGraphCollector(const SourceManager &SM, IncludeGraph &IG)
+  : SM(SM), IG(IG) {}
+
+  // Populates everything except direct includes for a node, which represents
+  // edges in the include graph and populated in inclusion directive.
+  // We cannot populate the fields in InclusionDirective because it does not
+  // have access to the contents of the included file.
+  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
+   SrcMgr::CharacteristicKind FileType,
+   FileID PrevFID) override {
+// We only need to process each file once. So we don't care about anything
+// but entries.
+if (Reason != FileChangeReason::EnterFile)
+  return;
+
+const auto FileID = SM.getFileID(Loc);
+const auto File = SM.getFileEntryForID(FileID);
+auto URI = toURI(File);
+if (!URI)
+  return;
+auto I = IG.try_emplace(*URI).first;
+
+auto &Node = I->getValue();

[PATCH] D54999: [clangd] Populate include graph during static indexing action.

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL348005: [clangd] Populate include graph during static 
indexing action. (authored by kadircet, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54999

Files:
  clang-tools-extra/trunk/clangd/Headers.h
  clang-tools-extra/trunk/clangd/index/Background.cpp
  clang-tools-extra/trunk/clangd/index/IndexAction.cpp
  clang-tools-extra/trunk/clangd/index/IndexAction.h
  clang-tools-extra/trunk/clangd/indexer/IndexerMain.cpp
  clang-tools-extra/trunk/unittests/clangd/CMakeLists.txt
  clang-tools-extra/trunk/unittests/clangd/IndexActionTests.cpp

Index: clang-tools-extra/trunk/clangd/index/IndexAction.cpp
===
--- clang-tools-extra/trunk/clangd/index/IndexAction.cpp
+++ clang-tools-extra/trunk/clangd/index/IndexAction.cpp
@@ -9,6 +9,98 @@
 namespace clangd {
 namespace {
 
+llvm::Optional toURI(const FileEntry *File) {
+  if (!File)
+return llvm::None;
+  auto AbsolutePath = File->tryGetRealPathName();
+  if (AbsolutePath.empty())
+return llvm::None;
+  return URI::create(AbsolutePath).toString();
+}
+
+// Collects the nodes and edges of include graph during indexing action.
+// Important: The graph generated by those callbacks might contain cycles and
+// self edges.
+struct IncludeGraphCollector : public PPCallbacks {
+public:
+  IncludeGraphCollector(const SourceManager &SM, IncludeGraph &IG)
+  : SM(SM), IG(IG) {}
+
+  // Populates everything except direct includes for a node, which represents
+  // edges in the include graph and populated in inclusion directive.
+  // We cannot populate the fields in InclusionDirective because it does not
+  // have access to the contents of the included file.
+  void FileChanged(SourceLocation Loc, FileChangeReason Reason,
+   SrcMgr::CharacteristicKind FileType,
+   FileID PrevFID) override {
+// We only need to process each file once. So we don't care about anything
+// but entries.
+if (Reason != FileChangeReason::EnterFile)
+  return;
+
+const auto FileID = SM.getFileID(Loc);
+const auto File = SM.getFileEntryForID(FileID);
+auto URI = toURI(File);
+if (!URI)
+  return;
+auto I = IG.try_emplace(*URI).first;
+
+auto &Node = I->getValue();
+// Node has already been populated.
+if (Node.URI.data() == I->getKeyData()) {
+#ifndef NDEBUG
+  auto Digest = digestFile(SM, FileID);
+  assert(Digest && Node.Digest == *Digest &&
+ "Same file, different digest?");
+#endif
+  return;
+}
+if (auto Digest = digestFile(SM, FileID))
+  Node.Digest = std::move(*Digest);
+Node.IsTU = FileID == SM.getMainFileID();
+Node.URI = I->getKey();
+  }
+
+  // Add edges from including files to includes.
+  void InclusionDirective(SourceLocation HashLoc, const Token &IncludeTok,
+  StringRef FileName, bool IsAngled,
+  CharSourceRange FilenameRange, const FileEntry *File,
+  StringRef SearchPath, StringRef RelativePath,
+  const Module *Imported,
+  SrcMgr::CharacteristicKind FileType) override {
+auto IncludeURI = toURI(File);
+if (!IncludeURI)
+  return;
+
+auto IncludingURI = toURI(SM.getFileEntryForID(SM.getFileID(HashLoc)));
+if (!IncludingURI)
+  return;
+
+auto NodeForInclude = IG.try_emplace(*IncludeURI).first->getKey();
+auto NodeForIncluding = IG.try_emplace(*IncludingURI);
+
+NodeForIncluding.first->getValue().DirectIncludes.push_back(NodeForInclude);
+  }
+
+  // Sanity check to ensure we have already populated a skipped file.
+  void FileSkipped(const FileEntry &SkippedFile, const Token &FilenameTok,
+   SrcMgr::CharacteristicKind FileType) override {
+#ifndef NDEBUG
+auto URI = toURI(&SkippedFile);
+if (!URI)
+  return;
+auto I = IG.try_emplace(*URI);
+assert(!I.second && "File inserted for the first time on skip.");
+assert(I.first->getKeyData() == I.first->getValue().URI.data() &&
+   "Node have not been populated yet");
+#endif
+  }
+
+private:
+  const SourceManager &SM;
+  IncludeGraph &IG;
+};
+
 // Wraps the index action and reports index data after each translation unit.
 class IndexAction : public WrapperFrontendAction {
 public:
@@ -16,15 +108,20 @@
   std::unique_ptr Includes,
   const index::IndexingOptions &Opts,
   std::function SymbolsCallback,
-  std::function RefsCallback)
+  std::function RefsCallback,
+  std::function IncludeGraphCallback)
   : WrapperFrontendAction(index::createIndexingAction(C, Opts, nullptr)),
 SymbolsCallback(SymbolsCallback), RefsCallback(RefsCallback),
-

[PATCH] D54999: [clangd] Populate include graph during static indexing action.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 176144.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54999

Files:
  clangd/Headers.h
  clangd/index/Background.cpp
  clangd/index/IndexAction.cpp
  clangd/index/IndexAction.h
  clangd/indexer/IndexerMain.cpp
  unittests/clangd/CMakeLists.txt
  unittests/clangd/IndexActionTests.cpp

Index: unittests/clangd/IndexActionTests.cpp
===
--- /dev/null
+++ unittests/clangd/IndexActionTests.cpp
@@ -0,0 +1,230 @@
+//===-- IndexActionTests.cpp  ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "TestFS.h"
+#include "index/IndexAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using ::testing::AllOf;
+using ::testing::ElementsAre;
+using ::testing::Not;
+using ::testing::Pair;
+using ::testing::UnorderedElementsAre;
+using ::testing::UnorderedPointwise;
+
+std::string toUri(llvm::StringRef Path) { return URI::create(Path).toString(); }
+
+MATCHER(IsTU, "") { return arg.IsTU; }
+
+MATCHER_P(HasDigest, Digest, "") { return arg.Digest == Digest; }
+
+MATCHER(HasSameURI, "") {
+  llvm::StringRef URI = testing::get<0>(arg);
+  const std::string &Path = testing::get<1>(arg);
+  return toUri(Path) == URI;
+}
+
+testing::Matcher
+IncludesAre(const std::vector &Includes) {
+  return ::testing::Field(&IncludeGraphNode::DirectIncludes,
+  UnorderedPointwise(HasSameURI(), Includes));
+}
+
+void checkNodesAreInitialized(const IndexFileIn &IndexFile,
+  const std::vector &Paths) {
+  EXPECT_THAT(Paths.size(), IndexFile.Sources->size());
+  for (llvm::StringRef Path : Paths) {
+auto URI = toUri(Path);
+const auto &Node = IndexFile.Sources->lookup(URI);
+// Uninitialized nodes will have an empty URI.
+EXPECT_EQ(Node.URI.data(), IndexFile.Sources->find(URI)->getKeyData());
+  }
+}
+
+std::map toMap(const IncludeGraph &IG) {
+  std::map Nodes;
+  for (auto &I : IG)
+Nodes.emplace(I.getKey(), I.getValue());
+  return Nodes;
+}
+
+class IndexActionTest : public ::testing::Test {
+public:
+  IndexActionTest() : InMemoryFileSystem(new llvm::vfs::InMemoryFileSystem) {}
+
+  IndexFileIn
+  runIndexingAction(llvm::StringRef MainFilePath,
+const std::vector &ExtraArgs = {}) {
+IndexFileIn IndexFile;
+IntrusiveRefCntPtr Files(
+new FileManager(FileSystemOptions(), InMemoryFileSystem));
+
+auto Action = createStaticIndexingAction(
+SymbolCollector::Options(),
+[&](SymbolSlab S) { IndexFile.Symbols = std::move(S); },
+[&](RefSlab R) { IndexFile.Refs = std::move(R); },
+[&](IncludeGraph IG) { IndexFile.Sources = std::move(IG); });
+
+std::vector Args = {"index_action", "-fsyntax-only",
+ "-xc++","-std=c++11",
+ "-iquote",  testRoot()};
+Args.insert(Args.end(), ExtraArgs.begin(), ExtraArgs.end());
+Args.push_back(MainFilePath);
+
+tooling::ToolInvocation Invocation(
+Args, Action.release(), Files.get(),
+std::make_shared());
+
+Invocation.run();
+
+checkNodesAreInitialized(IndexFile, FilePaths);
+return IndexFile;
+  }
+
+  void addFile(llvm::StringRef Path, llvm::StringRef Content) {
+InMemoryFileSystem->addFile(Path, 0,
+llvm::MemoryBuffer::getMemBuffer(Content));
+FilePaths.push_back(Path);
+  }
+
+protected:
+  std::vector FilePaths;
+  IntrusiveRefCntPtr InMemoryFileSystem;
+};
+
+TEST_F(IndexActionTest, CollectIncludeGraph) {
+  std::string MainFilePath = testPath("main.cpp");
+  std::string MainCode = "#include \"level1.h\"";
+  std::string Level1HeaderPath = testPath("level1.h");
+  std::string Level1HeaderCode = "#include \"level2.h\"";
+  std::string Level2HeaderPath = testPath("level2.h");
+  std::string Level2HeaderCode = "";
+
+  addFile(MainFilePath, MainCode);
+  addFile(Level1HeaderPath, Level1HeaderCode);
+  addFile(Level2HeaderPath, Level2HeaderCode);
+
+  IndexFileIn IndexFile = runIndexingAction(MainFilePath);
+  auto Nodes = toMap(*IndexFile.Sources);
+
+  EXPECT_THAT(Nodes,
+  UnorderedElementsAre(
+  Pair(toUri(MainFilePath),
+   AllOf(IsTU(), IncludesAre({Level1HeaderPath}),
+ HasDigest(digest(MainCode,
+  Pair(toUri(Level1HeaderPath),
+   AllOf(Not(IsTU(

[PATCH] D55054: [clang] Fill RealPathName for virtual files.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 176146.
kadircet marked 5 inline comments as done.
kadircet added a comment.

- Address comments


Repository:
  rC Clang

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

https://reviews.llvm.org/D55054

Files:
  include/clang/Basic/FileManager.h
  lib/Basic/FileManager.cpp
  unittests/Basic/FileManagerTest.cpp

Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -235,22 +235,18 @@
   ASSERT_TRUE(file != nullptr);
   ASSERT_TRUE(file->isValid());
   // "real path name" reveals whether the file was actually opened.
-  EXPECT_EQ("", file->tryGetRealPathName());
+  EXPECT_FALSE(file->isOpenForTests());
 
   file = manager.getFile("/tmp/test", /*OpenFile=*/true);
   ASSERT_TRUE(file != nullptr);
   ASSERT_TRUE(file->isValid());
-#ifdef _WIN32
-  EXPECT_EQ("/tmp\\test", file->tryGetRealPathName());
-#else
-  EXPECT_EQ("/tmp/test", file->tryGetRealPathName());
-#endif
+  EXPECT_TRUE(file->isOpenForTests());
 
   // However we should never try to open a file previously opened as virtual.
   ASSERT_TRUE(manager.getVirtualFile("/tmp/testv", 5, 0));
   ASSERT_TRUE(manager.getFile("/tmp/testv", /*OpenFile=*/false));
   file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
-  EXPECT_EQ("", file->tryGetRealPathName());
+  EXPECT_FALSE(file->isOpenForTests());
 }
 
 // The following tests apply to Unix-like system only.
@@ -353,4 +349,19 @@
   EXPECT_EQ(Path, ExpectedResult);
 }
 
+// getVirtualFile should always fill the real path.
+TEST_F(FileManagerTest, getVirtualFileFillsRealPathName) {
+  // Inject fake files into the file system.
+  auto statCache = llvm::make_unique();
+  statCache->InjectDirectory("/tmp", 42);
+  statCache->InjectFile("/tmp/test", 43);
+  manager.addStatCache(std::move(statCache));
+
+  // Check for real path.
+  const FileEntry *file = manager.getVirtualFile("/tmp/test", 123, 1);
+  ASSERT_TRUE(file != nullptr);
+  ASSERT_TRUE(file->isValid());
+  EXPECT_EQ(file->tryGetRealPathName(), "/tmp/test");
+}
+
 } // anonymous namespace
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -293,16 +293,8 @@
   // If we opened the file for the first time, record the resulting info.
   // Do this even if the cache entry was valid, maybe we didn't previously open.
   if (F && !UFE.File) {
-if (auto PathName = F->getName()) {
-  llvm::SmallString<128> AbsPath(*PathName);
-  // This is not the same as `VFS::getRealPath()`, which resolves symlinks
-  // but can be very expensive on real file systems.
-  // FIXME: the semantic of RealPathName is unclear, and the name might be
-  // misleading. We need to clean up the interface here.
-  makeAbsolutePath(AbsPath);
-  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
-  UFE.RealPathName = AbsPath.str();
-}
+if (auto PathName = F->getName())
+  fillRealPathName(&UFE, *PathName);
 UFE.File = std::move(F);
 assert(!UFE.DeferredOpen && "we just opened it!");
   }
@@ -395,6 +387,7 @@
 UFE->UniqueID = Data.UniqueID;
 UFE->IsNamedPipe = Data.IsNamedPipe;
 UFE->InPCH = Data.InPCH;
+fillRealPathName(UFE, Data.Name);
   }
 
   if (!UFE) {
@@ -438,6 +431,17 @@
   return Changed;
 }
 
+void FileManager::fillRealPathName(FileEntry *UFE, llvm::StringRef FileName) {
+  llvm::SmallString<128> AbsPath(FileName);
+  // This is not the same as `VFS::getRealPath()`, which resolves symlinks
+  // but can be very expensive on real file systems.
+  // FIXME: the semantic of RealPathName is unclear, and the name might be
+  // misleading. We need to clean up the interface here.
+  makeAbsolutePath(AbsPath);
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE->RealPathName = AbsPath.str();
+}
+
 llvm::ErrorOr>
 FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile,
   bool ShouldCloseOpenFile) {
Index: include/clang/Basic/FileManager.h
===
--- include/clang/Basic/FileManager.h
+++ include/clang/Basic/FileManager.h
@@ -104,6 +104,10 @@
   void closeFile() const {
 File.reset(); // rely on destructor to close File
   }
+
+  // Only for use in tests to see if deferred opens are happening, rather than
+  // relying on RealPathName being empty.
+  bool isOpenForTests() const { return File != nullptr; }
 };
 
 struct FileData;
@@ -173,6 +177,9 @@
   /// or a directory) as virtual directories.
   void addAncestorsAsVirtualDirs(StringRef Path);
 
+  /// Fills the RealPathName in file entry.
+  void fillRealPathName(FileEntry *UFE, llvm::StringRef FileName);
+
 public:
   FileManager(const FileSystemOptions &FileSystemOpts,
   IntrusiveRefCntPtr FS = nullptr);

[PATCH] D55054: [clang] Fill RealPathName for virtual files.

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC348006: [clang] Fill RealPathName for virtual files. 
(authored by kadircet, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55054?vs=176146&id=176147#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D55054

Files:
  include/clang/Basic/FileManager.h
  lib/Basic/FileManager.cpp
  unittests/Basic/FileManagerTest.cpp

Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -293,16 +293,8 @@
   // If we opened the file for the first time, record the resulting info.
   // Do this even if the cache entry was valid, maybe we didn't previously open.
   if (F && !UFE.File) {
-if (auto PathName = F->getName()) {
-  llvm::SmallString<128> AbsPath(*PathName);
-  // This is not the same as `VFS::getRealPath()`, which resolves symlinks
-  // but can be very expensive on real file systems.
-  // FIXME: the semantic of RealPathName is unclear, and the name might be
-  // misleading. We need to clean up the interface here.
-  makeAbsolutePath(AbsPath);
-  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
-  UFE.RealPathName = AbsPath.str();
-}
+if (auto PathName = F->getName())
+  fillRealPathName(&UFE, *PathName);
 UFE.File = std::move(F);
 assert(!UFE.DeferredOpen && "we just opened it!");
   }
@@ -395,6 +387,7 @@
 UFE->UniqueID = Data.UniqueID;
 UFE->IsNamedPipe = Data.IsNamedPipe;
 UFE->InPCH = Data.InPCH;
+fillRealPathName(UFE, Data.Name);
   }
 
   if (!UFE) {
@@ -438,6 +431,17 @@
   return Changed;
 }
 
+void FileManager::fillRealPathName(FileEntry *UFE, llvm::StringRef FileName) {
+  llvm::SmallString<128> AbsPath(FileName);
+  // This is not the same as `VFS::getRealPath()`, which resolves symlinks
+  // but can be very expensive on real file systems.
+  // FIXME: the semantic of RealPathName is unclear, and the name might be
+  // misleading. We need to clean up the interface here.
+  makeAbsolutePath(AbsPath);
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE->RealPathName = AbsPath.str();
+}
+
 llvm::ErrorOr>
 FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile,
   bool ShouldCloseOpenFile) {
Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -235,22 +235,18 @@
   ASSERT_TRUE(file != nullptr);
   ASSERT_TRUE(file->isValid());
   // "real path name" reveals whether the file was actually opened.
-  EXPECT_EQ("", file->tryGetRealPathName());
+  EXPECT_FALSE(file->isOpenForTests());
 
   file = manager.getFile("/tmp/test", /*OpenFile=*/true);
   ASSERT_TRUE(file != nullptr);
   ASSERT_TRUE(file->isValid());
-#ifdef _WIN32
-  EXPECT_EQ("/tmp\\test", file->tryGetRealPathName());
-#else
-  EXPECT_EQ("/tmp/test", file->tryGetRealPathName());
-#endif
+  EXPECT_TRUE(file->isOpenForTests());
 
   // However we should never try to open a file previously opened as virtual.
   ASSERT_TRUE(manager.getVirtualFile("/tmp/testv", 5, 0));
   ASSERT_TRUE(manager.getFile("/tmp/testv", /*OpenFile=*/false));
   file = manager.getFile("/tmp/testv", /*OpenFile=*/true);
-  EXPECT_EQ("", file->tryGetRealPathName());
+  EXPECT_FALSE(file->isOpenForTests());
 }
 
 // The following tests apply to Unix-like system only.
@@ -353,4 +349,19 @@
   EXPECT_EQ(Path, ExpectedResult);
 }
 
+// getVirtualFile should always fill the real path.
+TEST_F(FileManagerTest, getVirtualFileFillsRealPathName) {
+  // Inject fake files into the file system.
+  auto statCache = llvm::make_unique();
+  statCache->InjectDirectory("/tmp", 42);
+  statCache->InjectFile("/tmp/test", 43);
+  manager.addStatCache(std::move(statCache));
+
+  // Check for real path.
+  const FileEntry *file = manager.getVirtualFile("/tmp/test", 123, 1);
+  ASSERT_TRUE(file != nullptr);
+  ASSERT_TRUE(file->isValid());
+  EXPECT_EQ(file->tryGetRealPathName(), "/tmp/test");
+}
+
 } // anonymous namespace
Index: include/clang/Basic/FileManager.h
===
--- include/clang/Basic/FileManager.h
+++ include/clang/Basic/FileManager.h
@@ -104,6 +104,10 @@
   void closeFile() const {
 File.reset(); // rely on destructor to close File
   }
+
+  // Only for use in tests to see if deferred opens are happening, rather than
+  // relying on RealPathName being empty.
+  bool isOpenForTests() const { return File != nullptr; }
 };
 
 struct FileData;
@@ -173,6 +177,9 @@
   /// or a directory) as virtual directories.
   void addAncestorsAsVirtualDirs(StringRef Path);
 
+  /// Fills the RealPathName in file entry.
+  void fillRealPathName(FileEntry *UFE, llvm::Stri

r348006 - [clang] Fill RealPathName for virtual files.

[Kadir Cetinkaya via cfe-commits]

Author: kadircet
Date: Fri Nov 30 09:10:11 2018
New Revision: 348006

URL: http://llvm.org/viewvc/llvm-project?rev=348006&view=rev
Log:
[clang] Fill RealPathName for virtual files.

Summary:
Absolute path information for virtual files were missing even if we
have already stat'd the files. This patch puts that information for virtual
files that can succesffully be stat'd.

Reviewers: ilya-biryukov

Subscribers: cfe-commits

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

Modified:
cfe/trunk/include/clang/Basic/FileManager.h
cfe/trunk/lib/Basic/FileManager.cpp
cfe/trunk/unittests/Basic/FileManagerTest.cpp

Modified: cfe/trunk/include/clang/Basic/FileManager.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/FileManager.h?rev=348006&r1=348005&r2=348006&view=diff
==
--- cfe/trunk/include/clang/Basic/FileManager.h (original)
+++ cfe/trunk/include/clang/Basic/FileManager.h Fri Nov 30 09:10:11 2018
@@ -104,6 +104,10 @@ public:
   void closeFile() const {
 File.reset(); // rely on destructor to close File
   }
+
+  // Only for use in tests to see if deferred opens are happening, rather than
+  // relying on RealPathName being empty.
+  bool isOpenForTests() const { return File != nullptr; }
 };
 
 struct FileData;
@@ -173,6 +177,9 @@ class FileManager : public RefCountedBas
   /// or a directory) as virtual directories.
   void addAncestorsAsVirtualDirs(StringRef Path);
 
+  /// Fills the RealPathName in file entry.
+  void fillRealPathName(FileEntry *UFE, llvm::StringRef FileName);
+
 public:
   FileManager(const FileSystemOptions &FileSystemOpts,
   IntrusiveRefCntPtr FS = nullptr);

Modified: cfe/trunk/lib/Basic/FileManager.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/FileManager.cpp?rev=348006&r1=348005&r2=348006&view=diff
==
--- cfe/trunk/lib/Basic/FileManager.cpp (original)
+++ cfe/trunk/lib/Basic/FileManager.cpp Fri Nov 30 09:10:11 2018
@@ -293,16 +293,8 @@ const FileEntry *FileManager::getFile(St
   // If we opened the file for the first time, record the resulting info.
   // Do this even if the cache entry was valid, maybe we didn't previously 
open.
   if (F && !UFE.File) {
-if (auto PathName = F->getName()) {
-  llvm::SmallString<128> AbsPath(*PathName);
-  // This is not the same as `VFS::getRealPath()`, which resolves symlinks
-  // but can be very expensive on real file systems.
-  // FIXME: the semantic of RealPathName is unclear, and the name might be
-  // misleading. We need to clean up the interface here.
-  makeAbsolutePath(AbsPath);
-  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
-  UFE.RealPathName = AbsPath.str();
-}
+if (auto PathName = F->getName())
+  fillRealPathName(&UFE, *PathName);
 UFE.File = std::move(F);
 assert(!UFE.DeferredOpen && "we just opened it!");
   }
@@ -395,6 +387,7 @@ FileManager::getVirtualFile(StringRef Fi
 UFE->UniqueID = Data.UniqueID;
 UFE->IsNamedPipe = Data.IsNamedPipe;
 UFE->InPCH = Data.InPCH;
+fillRealPathName(UFE, Data.Name);
   }
 
   if (!UFE) {
@@ -438,6 +431,17 @@ bool FileManager::makeAbsolutePath(Small
   return Changed;
 }
 
+void FileManager::fillRealPathName(FileEntry *UFE, llvm::StringRef FileName) {
+  llvm::SmallString<128> AbsPath(FileName);
+  // This is not the same as `VFS::getRealPath()`, which resolves symlinks
+  // but can be very expensive on real file systems.
+  // FIXME: the semantic of RealPathName is unclear, and the name might be
+  // misleading. We need to clean up the interface here.
+  makeAbsolutePath(AbsPath);
+  llvm::sys::path::remove_dots(AbsPath, /*remove_dot_dot=*/true);
+  UFE->RealPathName = AbsPath.str();
+}
+
 llvm::ErrorOr>
 FileManager::getBufferForFile(const FileEntry *Entry, bool isVolatile,
   bool ShouldCloseOpenFile) {

Modified: cfe/trunk/unittests/Basic/FileManagerTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Basic/FileManagerTest.cpp?rev=348006&r1=348005&r2=348006&view=diff
==
--- cfe/trunk/unittests/Basic/FileManagerTest.cpp (original)
+++ cfe/trunk/unittests/Basic/FileManagerTest.cpp Fri Nov 30 09:10:11 2018
@@ -235,22 +235,18 @@ TEST_F(FileManagerTest, getFileDefersOpe
   ASSERT_TRUE(file != nullptr);
   ASSERT_TRUE(file->isValid());
   // "real path name" reveals whether the file was actually opened.
-  EXPECT_EQ("", file->tryGetRealPathName());
+  EXPECT_FALSE(file->isOpenForTests());
 
   file = manager.getFile("/tmp/test", /*OpenFile=*/true);
   ASSERT_TRUE(file != nullptr);
   ASSERT_TRUE(file->isValid());
-#ifdef _WIN32
-  EXPECT_EQ("/tmp\\test", file->tryGetRealPathName());
-#else
-  EXPECT_EQ("/tmp/test", file->tryGetRealPathName());
-#endif
+  EXPECT_TR

[PATCH] D54630: Move detection of libc++ include dirs to Driver on MacOS

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

@arphaman, did you have a chance to run the tests? There's not rush, just 
wanted to know whether we have any data at all at how


Repository:
  rC Clang

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

https://reviews.llvm.org/D54630



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

[PATCH] D55062: [clangd] Partition include graph on auto-index.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clangd/index/Background.cpp:70
+// We keep only the node "Path" and its edges.
+IncludeGraph getSubGraph(const URI &Uri, const IncludeGraph &FullGraph) {
+  IncludeGraph IG;

Naming: technically the variable name should be `URI`, but to avoid clashing 
with the type name we could use `U` 




Comment at: clangd/index/Background.cpp:259
 auto RS = llvm::make_unique(std::move(Refs).build());
+std::unique_ptr IG =
+Index.Sources ? llvm::make_unique(getSubGraph(

Maybe use auto? The type is spelled explicitly in the rhs



Comment at: clangd/index/Background.cpp:260
+std::unique_ptr IG =
+Index.Sources ? llvm::make_unique(getSubGraph(
+URI::create(Path), Index.Sources.getValue()))

What are the cases when it happens in practice? Do we ever run the indexer 
without producing the include graph?



Comment at: clangd/index/Background.cpp:388
 
-  log("Indexed {0} ({1} symbols, {2} refs)", Inputs.CompileCommand.Filename,
-  Symbols.size(), Refs.numRefs());
-  SPAN_ATTACH(Tracer, "symbols", int(Symbols.size()));
-  SPAN_ATTACH(Tracer, "refs", int(Refs.numRefs()));
-  IndexFileIn Index;
-  Index.Symbols = std::move(Symbols);
-  Index.Refs = std::move(Refs);
+  log("Indexed {0} ({1} symbols, {2} refs {3} sources)",
+  Inputs.CompileCommand.Filename, Index.Symbols->size(),

add a comma before `{3} sources`?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D55062



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

r348008 - Adding tests for -ast-dump; NFC.

[Aaron Ballman via cfe-commits]

Author: aaronballman
Date: Fri Nov 30 09:19:06 2018
New Revision: 348008

URL: http://llvm.org/viewvc/llvm-project?rev=348008&view=rev
Log:
Adding tests for -ast-dump; NFC.

This adds tests for GenericSelectionExpr; note that it points out a minor 
whitespace bug for selection expression cases.

Modified:
cfe/trunk/test/Misc/ast-dump-stmt.c

Modified: cfe/trunk/test/Misc/ast-dump-stmt.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/ast-dump-stmt.c?rev=348008&r1=348007&r2=348008&view=diff
==
--- cfe/trunk/test/Misc/ast-dump-stmt.c (original)
+++ cfe/trunk/test/Misc/ast-dump-stmt.c Fri Nov 30 09:19:06 2018
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -ast-dump -ast-dump-filter Test %s | FileCheck 
-strict-whitespace %s
+// RUN: %clang_cc1 -std=c11 -ast-dump -ast-dump-filter Test %s | FileCheck 
-strict-whitespace %s
 
 int TestLocation = 0;
 // CHECK:  VarDecl{{.*}}TestLocation
@@ -65,3 +65,64 @@ void TestUnaryOperatorExpr(void) {
   // CHECK-NEXT: ImplicitCastExpr
   // CHECK-NEXT:   DeclRefExpr{{.*}}'T2' 'int'
 }
+
+void TestGenericSelectionExpressions(int i) {
+  _Generic(i, int : 12);
+  // CHECK: GenericSelectionExpr 0x{{[^ ]*}}  'int'
+  // CHECK-NEXT: ImplicitCastExpr 0x{{[^ ]*}}
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'i' 'int'
+  // CHECK-NEXT: BuiltinType 0x{{[^ ]*}} 'int'
+  // FIXME: note that the following test line has a spurious whitespace.
+  // CHECK-NEXT: case  'int' selected
+  // CHECK-NEXT: BuiltinType 0x{{[^ ]*}} 'int'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 12
+  _Generic(i, int : 12, default : 0);
+  // CHECK: GenericSelectionExpr 0x{{[^ ]*}}  'int'
+  // CHECK-NEXT: ImplicitCastExpr 0x{{[^ ]*}}
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'i' 'int'
+  // CHECK-NEXT: BuiltinType 0x{{[^ ]*}} 'int'
+  // FIXME: note that the following test line has a spurious whitespace.
+  // CHECK-NEXT: case  'int' selected
+  // CHECK-NEXT: BuiltinType 0x{{[^ ]*}} 'int'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 12
+  // CHECK-NEXT: default
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 0
+  _Generic(i, default : 0, int : 12);
+  // CHECK: GenericSelectionExpr 0x{{[^ ]*}}  'int'
+  // CHECK-NEXT: ImplicitCastExpr 0x{{[^ ]*}}
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'i' 'int'
+  // CHECK-NEXT: BuiltinType 0x{{[^ ]*}} 'int'
+  // CHECK-NEXT: default
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 0
+  // FIXME: note that the following test line has a spurious whitespace.
+  // CHECK-NEXT: case  'int' selected
+  // CHECK-NEXT: BuiltinType 0x{{[^ ]*}} 'int'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 12
+  _Generic(i, int : 12, float : 10, default : 100);
+  // CHECK: GenericSelectionExpr 0x{{[^ ]*}}  'int'
+  // CHECK-NEXT: ImplicitCastExpr 0x{{[^ ]*}}
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'i' 'int'
+  // CHECK-NEXT: BuiltinType 0x{{[^ ]*}} 'int'
+  // FIXME: note that the following test line has a spurious whitespace.
+  // CHECK-NEXT: case  'int' selected
+  // CHECK-NEXT: BuiltinType 0x{{[^ ]*}} 'int'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 12
+  // FIXME: note that the following test line has a spurious whitespace.
+  // CHECK-NEXT: case  'float'
+  // CHECK-NEXT: BuiltinType 0x{{[^ ]*}} 'float'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 10
+  // CHECK-NEXT: default
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 100
+
+  int j = _Generic(i, int : 12);
+  // CHECK: DeclStmt 0x{{[^ ]*}} 
+  // CHECK-NEXT: VarDecl 0x{{[^ ]*}}  col:7 j 'int' cinit
+  // CHECK-NEXT: GenericSelectionExpr 0x{{[^ ]*}}  'int'
+  // CHECK-NEXT: ImplicitCastExpr 0x{{[^ ]*}}
+  // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue ParmVar 0x{{[^ 
]*}} 'i' 'int'
+  // CHECK-NEXT: BuiltinType 0x{{[^ ]*}} 'int'
+  // FIXME: note that the following test line has a spurious whitespace.
+  // CHECK-NEXT: case  'int' selected
+  // CHECK-NEXT: BuiltinType 0x{{[^ ]*}} 'int'
+  // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 12
+}


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

[PATCH] D55131: [CTU] Add more lit tests and better error handling

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: xazax.hun, balazske, a_sidorin.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs.

Adding some more CTU list tests. E.g. to check if a construct is unsupported.
We also slightly modify the handling of the return value of the `Import`
function from ASTImporter.


Repository:
  rC Clang

https://reviews.llvm.org/D55131

Files:
  lib/CrossTU/CrossTranslationUnit.cpp
  test/Analysis/Inputs/ctu-other.c
  test/Analysis/Inputs/externalFnMap2.txt
  test/Analysis/ctu-main.c

Index: test/Analysis/ctu-main.c
===
--- /dev/null
+++ test/Analysis/ctu-main.c
@@ -0,0 +1,60 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: mkdir -p %t/ctudir2
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-pch -o %t/ctudir2/ctu-other.c.ast %S/Inputs/ctu-other.c
+// RUN: cp %S/Inputs/externalFnMap2.txt %t/ctudir2/externalFnMap.txt
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -std=c89 -analyze -analyzer-checker=core,debug.ExprInspection  -analyzer-config experimental-enable-naive-ctu-analysis=true -analyzer-config ctu-dir=%t/ctudir2 -verify %s
+
+void clang_analyzer_eval(int);
+
+typedef struct {
+  int a;
+  int b;
+} foobar;
+
+static int s1 = 21;
+
+int f(int);
+int enumcheck(void);
+int static_check(void);
+
+enum A { x,
+ y,
+ z };
+
+extern foobar fb;
+
+int getkey();
+
+int main() {
+  clang_analyzer_eval(f(5) == 1); // expected-warning{{TRUE}}
+  clang_analyzer_eval(x == 0);// expected-warning{{TRUE}}
+  clang_analyzer_eval(enumcheck() == 42); // expected-warning{{TRUE}}
+
+  return getkey();
+}
+
+// Test reporting error in a macro.
+struct S;
+int g(struct S *);
+void test_macro(void) {
+  g(0); // expected-warning@Inputs/ctu-other.c:31 {{Access to field 'a' results in a dereference of a null pointer (loaded from variable 'ctx')}}
+}
+
+// The external function prototype is incomplete.
+// warning:implicit functions are prohibited by c99
+void test_implicit(){
+int res=ident_implicit(6);// external implicit functions are not inlined
+clang_analyzer_eval(res == 6); // expected-warning{{TRUE}}
+}
+
+
+// Tests the import of functions that have a struct parameter
+// defined in its prototype.
+struct data_t{int a;int b;};
+int struct_in_proto(struct data_t *d);
+void test_struct_def_in_argument(){
+  struct data_t d;
+  d.a=1;
+  d.b=0;
+  clang_analyzer_eval(struct_in_proto(&d)==0);// expected-warning{{UNKNOWN}}
+}
Index: test/Analysis/Inputs/externalFnMap2.txt
===
--- /dev/null
+++ test/Analysis/Inputs/externalFnMap2.txt
@@ -0,0 +1,6 @@
+c:@F@getkey ctu-other.c.ast
+c:@F@g ctu-other.c.ast
+c:@F@f ctu-other.c.ast
+c:@F@enumcheck ctu-other.c.ast
+c:@F@ident_implicit ctu-other.c.ast
+c:@F@struct_in_proto ctu-other.c.ast
Index: test/Analysis/Inputs/ctu-other.c
===
--- /dev/null
+++ test/Analysis/Inputs/ctu-other.c
@@ -0,0 +1,51 @@
+enum B {x = 42,l,s};
+
+typedef struct {
+  int a;
+  int b;
+} foobar;
+
+int enumcheck(void) {
+  return x;
+}
+
+foobar fb;
+
+int f(int i) {
+  if (fb.a) {
+fb.b = i;
+  }
+  return 1;
+}
+
+//TEST reporting an
+//error in macro
+//definition
+#define MYMACRO(ctx) \
+ctx->a;
+struct S{
+  int a;
+};
+
+int g(struct S *ctx){
+  MYMACRO(ctx);
+  return 0;
+}
+
+// TEST asm import not failing
+int getkey() {
+  int res;
+  asm ( "mov $42, %0"
+  : "=r" (res));
+  return res;
+}
+
+//Implicit function
+int ident_implicit(int in){
+return in;
+}
+
+//ASTImporter doesn't support this
+int struct_in_proto(struct data_t{int a;int b;} *d){
+  return 0;
+}
Index: lib/CrossTU/CrossTranslationUnit.cpp
===
--- lib/CrossTU/CrossTranslationUnit.cpp
+++ lib/CrossTU/CrossTranslationUnit.cpp
@@ -247,7 +247,10 @@
 CrossTranslationUnitContext::importDefinition(const FunctionDecl *FD) {
   ASTImporter &Importer = getOrCreateASTImporter(FD->getASTContext());
   auto *ToDecl =
-  cast(Importer.Import(const_cast(FD)));
+  cast_or_null(Importer.Import(const_cast(FD)));
+  if (!ToDecl) {
+return llvm::make_error(index_error_code::failed_import);
+  }
   assert(ToDecl->hasBody());
   assert(FD->hasBody() && "Functions already imported should have body.");
   return ToDecl;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52879: Derive builtin return type from its definition

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

Thank you for the detailed review. I'll work on a patch and add you as reviewer 
once done (prob. on Monday though).


Repository:
  rC Clang

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

https://reviews.llvm.org/D52879



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

[PATCH] D55124: [CodeComplete] Cleanup access checking in code completion

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov updated this revision to Diff 176150.
ilya-biryukov added a comment.

- Add missed field init
- Also fix access checks for member access with qualifiers


Repository:
  rC Clang

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

https://reviews.llvm.org/D55124

Files:
  include/clang/Sema/Sema.h
  lib/Parse/ParseExprCXX.cpp
  lib/Sema/CodeCompleteConsumer.cpp
  lib/Sema/SemaAccess.cpp
  lib/Sema/SemaCodeComplete.cpp
  test/CodeCompletion/accessibility-crash.cpp
  test/CodeCompletion/accessibility.cpp

Index: test/CodeCompletion/accessibility.cpp
===
--- /dev/null
+++ test/CodeCompletion/accessibility.cpp
@@ -0,0 +1,73 @@
+class X {
+public:
+ int pub;
+protected:
+ int prot;
+private:
+ int priv;
+};
+
+class Unrelated {
+public:
+  static int pub;
+protected:
+  static int prot;
+private:
+  static int priv;
+};
+
+class Y : public X {
+  int test() {
+this->pub = 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:21:11 %s -o - \
+// RUN: | FileCheck -check-prefix=THIS %s
+// THIS: priv (InBase,Inaccessible)
+// THIS: prot (InBase)
+// THIS: pub (InBase)
+//
+// Also check implicit 'this->', i.e. complete at the start of the line.
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:21:1 %s -o - \
+// RUN: | FileCheck -check-prefix=THIS %s
+
+X().pub + 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:32:9 %s -o - \
+// RUN: | FileCheck -check-prefix=X-OBJ %s
+// X-OBJ: priv (Inaccessible)
+// X-OBJ: prot (Inaccessible)
+// X-OBJ: pub : [#int#]pub
+
+Y().pub + 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:39:9 %s -o - \
+// RUN: | FileCheck -check-prefix=Y-OBJ %s
+// Y-OBJ: priv (InBase,Inaccessible)
+// Y-OBJ: prot (InBase)
+// Y-OBJ: pub (InBase)
+
+this->X::pub = 10;
+X::pub = 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:46:14 %s -o - \
+// RUN: | FileCheck -check-prefix=THIS-BASE %s
+//
+// THIS-BASE: priv (Inaccessible)
+// THIS-BASE: prot : [#int#]prot
+// THIS-BASE: pub : [#int#]pub
+//
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:47:8 %s -o - \
+// RUN: | FileCheck -check-prefix=THIS-BASE %s
+
+
+this->Unrelated::pub = 10; // a check we don't crash in this cases.
+Y().Unrelated::pub = 10; // a check we don't crash in this cases.
+Unrelated::pub = 10;
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:59:22 %s -o - \
+// RUN: | FileCheck -check-prefix=UNRELATED %s
+// UNRELATED: priv (Inaccessible)
+// UNRELATED: prot (Inaccessible)
+// UNRELATED: pub : [#int#]pub
+//
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:60:20 %s -o - \
+// RUN: | FileCheck -check-prefix=UNRELATED %s
+// RUN: not %clang_cc1 -fsyntax-only -code-completion-at=%s:61:16 %s -o - \
+// RUN: | FileCheck -check-prefix=UNRELATED %s
+  }
+};
Index: test/CodeCompletion/accessibility-crash.cpp
===
--- /dev/null
+++ test/CodeCompletion/accessibility-crash.cpp
@@ -0,0 +1,23 @@
+class X {
+public:
+ int pub;
+protected:
+ int prot;
+private:
+ int priv;
+};
+
+class Y : public X {
+  int test() {
+[]() {
+
+  // RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:13:1 %s -o - \
+  // RUN: | FileCheck %s
+  // CHECK: priv (InBase,Inaccessible)
+  // CHECK: prot (InBase)
+  // CHECK: pub (InBase)
+};
+  }
+};
+
+
Index: lib/Sema/SemaCodeComplete.cpp
===
--- lib/Sema/SemaCodeComplete.cpp
+++ lib/Sema/SemaCodeComplete.cpp
@@ -1278,38 +1278,53 @@
 }
 
 namespace {
+
 /// Visible declaration consumer that adds a code-completion result
 /// for each visible declaration.
 class CodeCompletionDeclConsumer : public VisibleDeclConsumer {
   ResultBuilder &Results;
-  DeclContext *CurContext;
+  DeclContext *InitialLookupCtx;
+  // NamingClass and BaseType are used for access-checking. See
+  // Sema::IsSimplyAccessible for details.
+  CXXRecordDecl *NamingClass;
+  QualType BaseType;
   std::vector FixIts;
-  // This is set to the record where the search starts, if this is a record
-  // member completion.
-  RecordDecl *MemberCompletionRecord = nullptr;
 
 public:
   CodeCompletionDeclConsumer(
-  ResultBuilder &Results, DeclContext *CurContext,
-  std::vector FixIts = std::vector(),
-  RecordDecl *MemberCompletionRecord = nullptr)
-  : Results(Results), CurContext(CurContext), FixIts(std::move(FixIts)),
-MemberCompletionRecord(MemberCompletionRecord) {}
+  ResultBuilder &Results, DeclContext *InitialLookupCtx,
+  QualType BaseType = QualType(),
+  std::vector FixIts = std::vector())
+  : Results(Results), InitialLookupCtx(InitialLookupCtx),
+FixIts(std::move(FixIts)) {
+Nam