[PATCH] D54781: [clangd] Add 'Switch header/source' command in clangd-vscode
malaperle updated this revision to Diff 175078.
malaperle marked 5 inline comments as done.
malaperle added a comment.
Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54781
Files:
clangd/clients/clangd-vscode/package.json
clangd/clients/clangd-vscode/src/extension.ts
Index: clangd/clients/clangd-vscode/src/extension.ts
===
--- clangd/clients/clangd-vscode/src/extension.ts
+++ clangd/clients/clangd-vscode/src/extension.ts
@@ -12,6 +12,12 @@
return config.get(option, defaultValue);
}
+namespace SwitchSourceHeaderRequest {
+export const type =
+new vscodelc.RequestType('textDocument/switchSourceHeader');
+}
+
/**
* this method is called when your extension is activate
* your extension is activated the very first time the command is executed
@@ -51,8 +57,25 @@
}
};
-const clangdClient = new vscodelc.LanguageClient('Clang Language Server',
serverOptions, clientOptions);
-console.log('Clang Language Server is now active!');
-
-const disposable = clangdClient.start();
+ const clangdClient = new vscodelc.LanguageClient('Clang Language Server',
serverOptions, clientOptions);
+ console.log('Clang Language Server is now active!');
+ context.subscriptions.push(clangdClient.start());
+ context.subscriptions.push(vscode.commands.registerCommand(
+ 'clangd-vscode.switchheadersource', async () => {
+const uri =
+vscode.Uri.file(vscode.window.activeTextEditor.document.fileName);
+if (!uri) {
+ return;
+}
+const docIdentifier =
+vscodelc.TextDocumentIdentifier.create(uri.toString());
+const sourceUri = await clangdClient.sendRequest(
+SwitchSourceHeaderRequest.type, docIdentifier);
+if (!sourceUri) {
+ return;
+}
+const doc = await vscode.workspace.openTextDocument(
+vscode.Uri.parse(sourceUri));
+vscode.window.showTextDocument(doc);
+ }));
}
Index: clangd/clients/clangd-vscode/package.json
===
--- clangd/clients/clangd-vscode/package.json
+++ clangd/clients/clangd-vscode/package.json
@@ -74,6 +74,20 @@
"description": "Names a file that clangd should log a
performance trace to, in chrome trace-viewer JSON format."
}
}
-}
+},
+"commands": [
+{
+"command": "clangd-vscode.switchheadersource",
+"title": "Switch between Source/Header"
+}
+],
+"keybindings": [
+{
+"command": "clangd-vscode.switchheadersource",
+"key": "Alt+o",
+"mac": "Alt+cmd+o",
+"when": "editorTextFocus"
+}
+]
}
}
Index: clangd/clients/clangd-vscode/src/extension.ts
===
--- clangd/clients/clangd-vscode/src/extension.ts
+++ clangd/clients/clangd-vscode/src/extension.ts
@@ -12,6 +12,12 @@
return config.get(option, defaultValue);
}
+namespace SwitchSourceHeaderRequest {
+export const type =
+new vscodelc.RequestType('textDocument/switchSourceHeader');
+}
+
/**
* this method is called when your extension is activate
* your extension is activated the very first time the command is executed
@@ -51,8 +57,25 @@
}
};
-const clangdClient = new vscodelc.LanguageClient('Clang Language Server', serverOptions, clientOptions);
-console.log('Clang Language Server is now active!');
-
-const disposable = clangdClient.start();
+ const clangdClient = new vscodelc.LanguageClient('Clang Language Server', serverOptions, clientOptions);
+ console.log('Clang Language Server is now active!');
+ context.subscriptions.push(clangdClient.start());
+ context.subscriptions.push(vscode.commands.registerCommand(
+ 'clangd-vscode.switchheadersource', async () => {
+const uri =
+vscode.Uri.file(vscode.window.activeTextEditor.document.fileName);
+if (!uri) {
+ return;
+}
+const docIdentifier =
+vscodelc.TextDocumentIdentifier.create(uri.toString());
+const sourceUri = await clangdClient.sendRequest(
+SwitchSourceHeaderRequest.type, docIdentifier);
+if (!sourceUri) {
+ return;
+}
+const doc = await vscode.workspace.openTextDocument(
+vscode.Uri.parse(sourceUri));
+vscode.window.showTextDocument(doc);
+ }));
}
Index: clangd/clients/clangd-vscode/package.json
===
--- clangd/clients/clangd-vscode/package.json
+++ clangd/clients/clangd-vscode/package.json
@@ -74,6 +74,20 @@
"description": "Names a file that clangd should log a performance trace
[PATCH] D54781: [clangd] Add 'Switch header/source' command in clangd-vscode
malaperle added a comment. In https://reviews.llvm.org/D54781#1306102, @ioeric wrote: > Could you run clang-format on the changed lines? I didn't know clang-format could be used for Typescript. I ran it and it's a bit inconsistent with the rest of the file but I don't want to format the whole file. Comment at: clangd/clients/clangd-vscode/package.json:81 +"command": "clangd-vscode.switchheadersource", +"title": "C/C++: Switch between Source/Header" +} ioeric wrote: > and objc? I just removed the languages. It's a bit much to have all of them. Comment at: clangd/clients/clangd-vscode/src/extension.ts:55 protocol2Code: (value: string) => vscode.Uri.file(realpathSync(vscode.Uri.parse(value).fsPath)) } ioeric wrote: > We might want to share the logic for converting protocol URI to vscode URI. What do you have in mind? I'm not sure there is much to share since I'm not sure we should be using readpathSync for the toggle header/source. If the source is opened as a symlink and the header also exists as a symlink then I think it's OK to open the symlink. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54379: Add Hurd toolchain support to Clang
kristina added a comment. Ping. Would like someone else to co-review this, everything looks fine aside from tests (I think a portion of this could use a short unit test with regards to creating the actual target instance). Also your FileCheck test is only for invoking `clang -cc1` from the looks, can you add coverage for static linker invocations (as driver is also generally used to invoke the linker, unless that's explicitly not the case on Hurd)? Repository: rC Clang https://reviews.llvm.org/D54379 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64
kristina requested changes to this revision. kristina added a comment. In https://reviews.llvm.org/D53696#1305990, @kallisti5 wrote: > Indeed. I agree the _GLIBCXX_USE_FLOAT128 is misplaced there. That comes > from config.h in libstdc++. That's working around an issue in Haiku's build > (libstdc++ is built by gcc, but then we try and use it with clang later) > I can remove that. > > Is the __FLOAT128__ desireable? Sorry I was referring to `__FLOAT128__` being standard, the other define shouldn't be there IMO. But then again I'm not an expert on Haiku, `__FLOAT128__` seems fine to me, the other change looks more questionable, if anyone is familiar with Haiku internals, please chime in. Repository: rC Clang https://reviews.llvm.org/D53696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r347480 - A __builtin_constant_p() returns 0 with a function type.
Author: void
Date: Thu Nov 22 14:58:06 2018
New Revision: 347480
URL: http://llvm.org/viewvc/llvm-project?rev=347480=rev
Log:
A __builtin_constant_p() returns 0 with a function type.
Modified:
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/test/CodeGen/builtin-constant-p.c
Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=347480=347479=347480=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Thu Nov 22 14:58:06 2018
@@ -1935,7 +1935,7 @@ RValue CodeGenFunction::EmitBuiltinExpr(
const Expr *Arg = E->getArg(0);
QualType ArgType = Arg->getType();
-if (!hasScalarEvaluationKind(ArgType))
+if (!hasScalarEvaluationKind(ArgType) || ArgType->isFunctionType())
// We can only reason about scalar types.
return RValue::get(ConstantInt::get(ResultType, 0));
Modified: cfe/trunk/test/CodeGen/builtin-constant-p.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/builtin-constant-p.c?rev=347480=347479=347480=diff
==
--- cfe/trunk/test/CodeGen/builtin-constant-p.c (original)
+++ cfe/trunk/test/CodeGen/builtin-constant-p.c Thu Nov 22 14:58:06 2018
@@ -128,3 +128,29 @@ int test13() {
// CHECK: ret i32 1
return __builtin_constant_p( != 0);
}
+
+typedef unsigned long uintptr_t;
+#define assign(p, v) ({ \
+ uintptr_t _r_a_p__v = (uintptr_t)(v); \
+ if (__builtin_constant_p(v) && _r_a_p__v == (uintptr_t)0) { \
+union { \
+ uintptr_t __val;\
+ char __c[1];\
+} __u = { \
+ .__val = (uintptr_t)_r_a_p__v \
+};\
+*(volatile unsigned int*) = *(unsigned int*)(__u.__c); \
+__u.__val;\
+ } \
+ _r_a_p__v; \
+})
+
+typedef void fn_p(void);
+extern fn_p *dest_p;
+
+static void src_fn(void) {
+}
+
+void test14() {
+ assign(dest_p, src_fn);
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
jkorous updated this revision to Diff 175069.
jkorous marked 2 inline comments as done.
jkorous added a comment.
Changes based on review.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54799
Files:
ClangdLSPServer.cpp
ClangdLSPServer.h
ClangdServer.cpp
ClangdServer.h
Protocol.cpp
Protocol.h
XRefs.cpp
XRefs.h
clangd/cursor-info.test
index/Index.cpp
index/Index.h
Index: clangd/cursor-info.test
===
--- /dev/null
+++ clangd/cursor-info.test
@@ -0,0 +1,46 @@
+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///simple.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///simple.cpp"},"position":{"line":0,"character":27}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "CA2EBE44A1D76D2A1547D47BC6D51EBF",
+# CHECK-NEXT:"name": "foo",
+# CHECK-NEXT:"usr": "c:@F@foo#"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///nested-decl.cpp","languageId":"cpp","version":1,"text":"namespace nnn { struct aaa {}; void foo() { aaa a; }; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///nested-decl.cpp"},"position":{"line":0,"character":46}}}
+# CHECK:"containerName": "nnn::",
+# CHECK-NEXT:"id": "20237FF18EB405D842456DC5D578426D",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:@N@nnn@S@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///reference.cpp","languageId":"cpp","version":1,"text":"int value; void foo(int) {} void bar() { foo(value); }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///reference.cpp"},"position":{"line":0,"character":48}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "844613FB2393C9D40A2AFF25D5D316A1",
+# CHECK-NEXT:"name": "value",
+# CHECK-NEXT:"usr": "c:@value"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///local.cpp","languageId":"cpp","version":1,"text":"void foo() { int aaa; int bbb = aaa; }"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///local.cpp"},"position":{"line":0,"character":33}}}
+# CHECK:"containerName": "foo",
+# CHECK-NEXT:"id": "C05589F2664B06F392C2C438568E55E0",
+# CHECK-NEXT:"name": "aaa",
+# CHECK-NEXT:"usr": "c:local.cpp@13@F@foo#@aaa"
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///macro.cpp","languageId":"cpp","version":1,"text":"#define MACRO 5\nint i = MACRO;"}}}
+---
+{"jsonrpc":"2.0","id":1,"method":"textDocument/symbolInfo","params":{"textDocument":{"uri":"test:///macro.cpp"},"position":{"line":1,"character":11}}}
+# CHECK:"containerName": "",
+# CHECK-NEXT:"id": "29EB506CBDF1BA6D1B6EC203FF03B384",
+# CHECK-NEXT:"name": "MACRO",
+# CHECK-NEXT:"usr": "c:macro.cpp@24@macro@MACRO"
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: index/Index.h
===
--- index/Index.h
+++ index/Index.h
@@ -10,11 +10,11 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_INDEX_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_INDEX_H
+#include "Protocol.h"
#include "clang/Index/IndexSymbol.h"
#include "clang/Lex/Lexer.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/DenseSet.h"
-#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
@@ -94,54 +94,6 @@
}
llvm::raw_ostream <<(llvm::raw_ostream &, const SymbolLocation &);
-// The class identifies a particular C++ symbol (class, function, method, etc).
-//
-// As USRs (Unified Symbol Resolution) could be large, especially for functions
-// with long type arguments, SymbolID is using truncated SHA1(USR) values to
-// guarantee the uniqueness of symbols while using a relatively small amount of
-// memory (vs storing USRs directly).
-//
-// SymbolID can be used as key in the symbol indexes to lookup the symbol.
-class SymbolID {
-public:
- SymbolID() = default;
- explicit SymbolID(llvm::StringRef USR);
-
- bool operator==(const SymbolID ) const {
-return HashValue == Sym.HashValue;
- }
- bool operator<(const SymbolID ) const {
-return HashValue < Sym.HashValue;
- }
-
- // The stored hash is truncated to RawSize bytes.
- // This trades off memory against the number of symbols we can handle.
- // FIXME: can we
[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker
Szelethus added a comment. Herald added a subscriber: baloghadamsoftware. I'll commit tomorrow, when I have time to keep an eye out for buildbots. Thanks! https://reviews.llvm.org/D33672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54799: [clangd][WIP] textDocument/CursorInfo method
jkorous marked 7 inline comments as done.
jkorous added a comment.
Hi Sam.
In https://reviews.llvm.org/D54799#1305470, @sammccall wrote:
>
>> I'd like to ask for early feedback - what's still missing is relevant client
>> capability.
>
> I'd suggest leaving it out unless others feel strongly.
More than happy to keep it simple! I somehow assumed there'll be demand for
this capability.
>> - conditional return in `getCursorInfo` - Should we return for example data
>> with empty `USR`?
>
> Please return a symbol unless it has no SymbolID (we don't treat those as
> symbols in clangd).
Ok.
>> - `containerName` of local variables - It's currently empty even if semantic
>> parent has a name (say it's a function). (Please search for local.cpp in the
>> test.) Is that what we want?
>
> good question, no idea.
I am adding an attempt to get a named declaration context for such cases. It's
easy to remove if we decide against that.
>> - For now I used `getSymbolAtPosition()` as it's simpler and fits the
>> context better. However I assume I could use this optimization from
>> `tooling::getNamedDeclAt()` (in a separate patch):
>> https://github.com/llvm-mirror/clang/blob/master/lib/Tooling/Refactoring/Rename/#L82
>
> Yes, though at least for C++ this optimization won't usually buy anything, as
> the top-level decl will just be a namespace decl I think.
> We could plumb this deeper if we want to win more here.
Ok, I'll leave it for the hypothetical future.
> - One thing I am wondering about is whether we could use (and whether it's a
> significant improvement) some early return in `indexTopLevelDecls` (using
> `DeclarationAndMacrosFinder`) also for hover and definition use-cases. Is it
> correct to assume that at a given `Location` there can be maximum of one
> declaration and one definition? There can be multiple and we should return
> them all. (Not sure what we do for hover, but defs handles this correctly).
Ah, my bad - I was considering only explicit cases. Thanks for the explanation.
In the context of textDocument/cursorInfo - does it make sense to use the first
explicit declaration we find or am I missing something still? (This is related
to discussion about `StopOnFirstDeclFound`.)
I processed and implemented most of your comments. Two major tasks are:
finishing the discussion about multiple declarations and writing unittests
(preferably after we agree on interfaces).
Comment at: Protocol.h:676
+/// Represents information about identifier.
+struct IdentifierData {
sammccall wrote:
> sammccall wrote:
> > Unless I'm misunderstanding, this is about a symbol, not an identifier.
> > (e.g. overloads of C++ functions are distinct symbols). Maybe
> > `SymbolDetails` would be a useful name.
> >
> > There's a question of scope here: `getCursorInfo` seems to return a single
> > instance of these, but there can be 0, 1, or many symbols at a given
> > location. These could be `vector` or so, or if this struct
> > is intended to encapsulate them all, it could be named `SymbolsAtLocation`
> > or so.
> /// This is returned from textDocument/cursorInfo, which is a clangd
> extension.
I probably don't understand "many symbols at a given location" - more at the
end of my out-of-line comment.
Comment at: Protocol.h:678
+struct IdentifierData {
+ /// The name of this symbol.
+ std::string name;
sammccall wrote:
> This comment doesn't really say anything. It can be omitted, or we could add
> additional information.
> (Some of the existing comments are equally content-free, because we've copied
> the comments from LSP verbatim, but that's not the case here)
I was actually wondering what is the value of those comments but decided to
keep it consistent. Deleting.
Comment at: Protocol.h:682
+ /// The name of the symbol containing this symbol.
+ std::string containerName;
+
sammccall wrote:
> sammccall wrote:
> > If we're not going to merge, I'd suggest making this "scope" and allowing
> > it to end with ::.
> > This makes it easier to compose (qname = scope + name), and makes it
> > clearer what "contains" means. (LSP got this wrong IMO, at least for C++).
> what do you want this to do for objc methods and fields?
> It'd be worth having a test if it's important, I'm guilty of not
> knowing/checking ObjC behavior.
I don't really have any opinion about this but @benlangmuir might have.
I'd just make sure whatever we decide makes sense for local symbols (if we keep
returning them).
I'll add tests for ObjC. Thanks for pointing that out!
Comment at: Protocol.h:684
+
+ /// The USR of this symbol.
+ llvm::SmallString<128> USR;
sammccall wrote:
> USR could use some definition/references.
True. Would you go further than expanding the acronym and copying description
from doxygen annotation of `SymbolID`?
I haven't actually found any
[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone
JonasToth added a comment.
Thank you for this great patch!
Did you consider using `clang/Tooling/ASTDiff/*` functionality, as there is the
possibility of just comparing the AST 'below' the branches?
Please add tests that contain macros as well, and what do you think should
happen with them? I think it would be very valuable to figure out that
different macro expansion result in the same code.
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:31
+/// an if/else if/else chain is one statement (which may be a CompoundStmt).
+using SwitchBranch = llvm::SmallVector;
+} // anonymous namespace
maybe plural for the typename would fit better, as its a vector of multiple
elements?
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:46
+RHS[i]->stripLabelLikeStatements(), Context)) {
+ // NOTE: We strip goto labels and annotations in addition to stripping
+ // the `case X:` or `default:` labels, but it is very unlikely that this
You can ellide the single-stmt braces here, maybe its better to move the
comment above the for then?
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:71
+// Check whether this `if` is part of an `else if`:
+if (const auto *IP =
+dyn_cast(Result.Nodes.getNodeAs("ifParent"))) {
This if should always be true, no? The matcher will always bind `ifParent`
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:83
+assert(Then && "An IfStmt must have a `then` branch!");
+const Stmt *Else = IS->getElse();
+if (!Else) {
This detection can and should land in the matcher, to filter as much as
possible already while matching.
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:88
+ return;
+} else if (!isa(Else)) {
+ // Just a simple if with no `else if` branch.
Please don't use `else` after return. You can ellide the braces in the `if` as
well.
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:113
+ Cur = dyn_cast(Else);
+ if (!Cur) {
+// ...this is just a plain `else` branch; we store it.
you can ellide the braces.
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:116
+Branches.push_back(Else);
+// Note: We will exit the while loop here.
+ }
This note is misplaced? The exit is at the `break`.
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:120
+
+const size_t N = Branches.size();
+llvm::BitVector KnownAsClone(N);
please dont use `const` for values, only pointers and references
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:124
+for (size_t i = 0; i + 1 < N; i++) {
+ if (KnownAsClone[i]) {
+// We have already seen Branches[i] as a clone of an earlier branch.
braces, i will stop pointing at it. Please do it on the other parts as well
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:153
+ }
+}
+ } else if (const auto *SS = Result.Nodes.getNodeAs("switch")) {
i think you can use early return here, if yes, no `else` afterwards.
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:164
+
+// We will first collect the branhces of the switch statements. For the
+// sake of simplicity we say that branches are delimited by the SwitchCase
typo, branhces
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:187
+while (FamilyBegin < End) {
+ auto FamilyEnd = FamilyBegin + 1;
+ while (FamilyEnd < End &&
i think the name `FamilyEnd` is not very descriptive, maybe `SubsequentBranch`
or so?
Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:196-197
+diag(FamilyBegin->front()->getBeginLoc(),
+ "switch has %0 consecutive identical branches")
+<< static_cast(FamilyEnd - FamilyBegin);
+diag(Lexer::getLocForEndOfToken((FamilyEnd - 1)->back()->getEndLoc(),
0,
please use `std::distance` instead
Comment at: docs/clang-tidy/checks/list.rst:13
abseil-str-cat-append
+ abseil-string-find-startswith
android-cloexec-accept
spurious change
Comment at: test/clang-tidy/bugprone-branch-clone.cpp:4
+void test_basic1(int in, int ) {
+ if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else
branches [bugprone-branch-clone]
could you please add tests for ternary operators?
Repository:
rCTE Clang Tools Extra
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] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py
JonasToth updated this revision to Diff 175063.
JonasToth added a comment.
Push some fixes i did while working with this script, for reference of others
potentially looking at this patch.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54141
Files:
clang-tidy/tool/run-clang-tidy.py
clang-tidy/tool/run_clang_tidy.py
clang-tidy/tool/test_input/out_csa_cmake.log
clang-tidy/tool/test_input/out_performance_cmake.log
clang-tidy/tool/test_log_parser.py
docs/ReleaseNotes.rst
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -217,6 +217,9 @@
` check does not warn
about calls inside macros anymore by default.
+- `run-clang-tidy.py` support deduplication of `clang-tidy` diagnostics
+ to reduce the amount of output with the optional `-deduplicate` flag.
+
Improvements to include-fixer
-
Index: clang-tidy/tool/test_log_parser.py
===
--- /dev/null
+++ clang-tidy/tool/test_log_parser.py
@@ -0,0 +1,236 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+
+import unittest
+from run_clang_tidy import Diagnostic, Deduplication
+from run_clang_tidy import ParseClangTidyDiagnostics, _is_valid_diag_match
+
+
+class TestDiagnostics(unittest.TestCase):
+"""Test fingerprinting diagnostic messages"""
+
+def test_construction(self):
+d = Diagnostic("/home/user/project/my_file.h", 24, 4,
+ "warning: Do not do this thing [warning-category]")
+self.assertIsNotNone(d)
+self.assertEqual(str(d),
+ "/home/user/project/my_file.h:24:4: warning: Do not do this thing [warning-category]")
+
+d.add_additional_line(" MyCodePiece();")
+d.add_additional_line(" ^")
+
+self.assertEqual(str(d),
+ "/home/user/project/my_file.h:24:4: warning: Do not do this thing [warning-category]"
+ "\n MyCodePiece();"
+ "\n ^")
+
+
+class TestDeduplication(unittest.TestCase):
+"""Test the `DiagEssence` based deduplication of diagnostic messages."""
+
+def test_construction(self):
+self.assertIsNotNone(Deduplication())
+
+def test_dedup(self):
+dedup = Deduplication()
+d = Diagnostic("/home/user/project/my_file.h", 24, 4,
+ "warning: Do not do this thing [warning-category]")
+self.assertTrue(dedup.insert_and_query(d))
+self.assertFalse(dedup.insert_and_query(d))
+
+d2 = Diagnostic("/home/user/project/my_file.h", 24, 4,
+"warning: Do not do this thing [warning-category]")
+d2.add_additional_line(" MyCodePiece();")
+d2.add_additional_line(" ^")
+self.assertTrue(dedup.insert_and_query(d2))
+self.assertFalse(dedup.insert_and_query(d2))
+
+d3 = Diagnostic("/home/user/project/my_file.h", 24, 4,
+"warning: Do not do this thing [warning-category]")
+self.assertFalse(dedup.insert_and_query(d3))
+
+class TestLinewiseParsing(unittest.TestCase):
+def test_construction(self):
+self.assertIsNotNone(ParseClangTidyDiagnostics())
+
+def test_valid_diags_regex(self):
+pp = ParseClangTidyDiagnostics()
+
+warning = "/home/user/project/my_file.h:123:1: warning: don't do it [no]"
+m = pp._diag_re.match(warning)
+self.assertTrue(m)
+self.assertTrue(_is_valid_diag_match(m.groups()))
+
+error = "/home/user/project/my_file.h:1:110: error: wrong! [not-ok]"
+m = pp._diag_re.match(error)
+self.assertTrue(m)
+self.assertTrue(_is_valid_diag_match(m.groups()))
+
+hybrid = "/home/user/project/boo.cpp:30:42: error: wrong! [not-ok,bad]"
+m = pp._diag_re.match(hybrid)
+self.assertTrue(m)
+self.assertTrue(_is_valid_diag_match(m.groups()))
+
+note = "/home/user/project/my_file.h:1:110: note: alksdj"
+m = pp._diag_re.match(note)
+self.assertFalse(m)
+
+garbage = "not a path:not_a_number:110: gibberish"
+m = pp._diag_re.match(garbage)
+self.assertFalse(m)
+
+def test_single_diagnostics(self):
+pp = ParseClangTidyDiagnostics()
+example_warning = [
+"/project/git/Source/kwsys/Terminal.c:53:21: warning: use of a signed integer operand with a binary bitwise operator [hicpp-signed-bitwise]",
+]
+pp._parse_lines(example_warning)
+self.assertEqual(
+str(pp.get_diags()[0]),
+"/project/git/Source/kwsys/Terminal.c:53:21: warning: use of a signed integer operand with a binary bitwise operator [hicpp-signed-bitwise]"
+)
+
+def test_no_diag(self):
+pp = ParseClangTidyDiagnostics()
+garbage_lines = \
+"""
+hicpp-no-array-decay
+
[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions
Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342 + DefaultBool IsOptimistic; + MemFunctionInfoTy MemFunctionInfo; public: Szelethus wrote: > Szelethus wrote: > > MTC wrote: > > > I can't say that abstracting `MemFunctionInfo` is a perfect solution, for > > > example `IsOptimistic` and `ShouldIncludeOwnershipAnnotatedFunctions ` > > > are close to each other in the object layout, but they do the same thing, > > > which makes me feel weird. And there are so many `MemFunctionInfo.` :) > > Well the thinking here was that `MallocChecker` is seriously overcrowded -- > > it's a one-tool-to-solve-everything class, and moving these > > `IdentifierInfos` in their separate class was pretty much the first step I > > took: I think it encapsulates a particular task well and eases a lot on > > `MallocChecker`. Sure `MemFunctionInfo.` is reduntant, but each time you > > see it, it indicates that we're calling a function or using a data member > > that has no effect on the actual analysis, that we're inquiring about more > > information about a given function. and nothing more. For a checker that > > emits warning at seemingly random places wherever, I think this is valuable. > > > > By the way, it also forces almost all similar conditions in a separate > > line, which is an unexpected, but pleasant help to the untrained eye: > > ``` > > if (Fun == MemFunctionInfo.II_malloc || > > Fun == MemFunctionInfo.II_whatever) > > ``` > > Nevertheless, this is the only change I'm not 100% confident about, if you > > and/or others disagree, I'll happily revert it. > (leaving a separate inline for a separate topic) > The was this checker abuses checker options isn't nice at all, I agree. I > could just rename `MallocChecker::IsOptimistic` to > `MallocChecker::ShouldIncludeOwnershipAnnotatedFunctions`, and have it passed > around as a parameter in `MemFunctionInfoTy`. Would you prefer that, should > you be okay with `MemFunctionInfoTy` in general? The way* this checker abuses Repository: rC Clang https://reviews.llvm.org/D54823 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions
Szelethus added a comment.
And thank you for helping me along the way! :D
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342
+ DefaultBool IsOptimistic;
+ MemFunctionInfoTy MemFunctionInfo;
public:
MTC wrote:
> I can't say that abstracting `MemFunctionInfo` is a perfect solution, for
> example `IsOptimistic` and `ShouldIncludeOwnershipAnnotatedFunctions ` are
> close to each other in the object layout, but they do the same thing, which
> makes me feel weird. And there are so many `MemFunctionInfo.` :)
Well the thinking here was that `MallocChecker` is seriously overcrowded --
it's a one-tool-to-solve-everything class, and moving these `IdentifierInfos`
in their separate class was pretty much the first step I took: I think it
encapsulates a particular task well and eases a lot on `MallocChecker`. Sure
`MemFunctionInfo.` is reduntant, but each time you see it, it indicates that
we're calling a function or using a data member that has no effect on the
actual analysis, that we're inquiring about more information about a given
function. and nothing more. For a checker that emits warning at seemingly
random places wherever, I think this is valuable.
By the way, it also forces almost all similar conditions in a separate line,
which is an unexpected, but pleasant help to the untrained eye:
```
if (Fun == MemFunctionInfo.II_malloc ||
Fun == MemFunctionInfo.II_whatever)
```
Nevertheless, this is the only change I'm not 100% confident about, if you
and/or others disagree, I'll happily revert it.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342
+ DefaultBool IsOptimistic;
+ MemFunctionInfoTy MemFunctionInfo;
public:
Szelethus wrote:
> MTC wrote:
> > I can't say that abstracting `MemFunctionInfo` is a perfect solution, for
> > example `IsOptimistic` and `ShouldIncludeOwnershipAnnotatedFunctions ` are
> > close to each other in the object layout, but they do the same thing, which
> > makes me feel weird. And there are so many `MemFunctionInfo.` :)
> Well the thinking here was that `MallocChecker` is seriously overcrowded --
> it's a one-tool-to-solve-everything class, and moving these `IdentifierInfos`
> in their separate class was pretty much the first step I took: I think it
> encapsulates a particular task well and eases a lot on `MallocChecker`. Sure
> `MemFunctionInfo.` is reduntant, but each time you see it, it indicates that
> we're calling a function or using a data member that has no effect on the
> actual analysis, that we're inquiring about more information about a given
> function. and nothing more. For a checker that emits warning at seemingly
> random places wherever, I think this is valuable.
>
> By the way, it also forces almost all similar conditions in a separate line,
> which is an unexpected, but pleasant help to the untrained eye:
> ```
> if (Fun == MemFunctionInfo.II_malloc ||
> Fun == MemFunctionInfo.II_whatever)
> ```
> Nevertheless, this is the only change I'm not 100% confident about, if you
> and/or others disagree, I'll happily revert it.
(leaving a separate inline for a separate topic)
The was this checker abuses checker options isn't nice at all, I agree. I could
just rename `MallocChecker::IsOptimistic` to
`MallocChecker::ShouldIncludeOwnershipAnnotatedFunctions`, and have it passed
around as a parameter in `MemFunctionInfoTy`. Would you prefer that, should you
be okay with `MemFunctionInfoTy` in general?
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
if (FD->getKind() == Decl::Function) {
-initIdentifierInfo(C.getASTContext());
+MemFunctionInfo.initIdentifierInfo(C.getASTContext());
IdentifierInfo *FunI = FD->getIdentifier();
MTC wrote:
> If I not wrong, `MemFunctionInfo` is mainly a class member of `MallocChecker`
> that hold a bunch of memory function identifiers and provide corresponding
> helper APIs. And we should pick a proper time to initialize it.
>
> I want to known why you call `initIdentifierInfo()` in `checkPostStmt(const
> CallExpr *E,)`, this callback is called after `checkPreCall(const CallEvent
> , )` in which we need `MemFunctionInfo`.
Well, I'd like to know too! I think lazy initializing this struct creates more
problems that it solves, but I wanted to draw a line somewhere in terms of how
invasive I'd like to make this patch.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1247
+CheckerContext , const Expr *E, const unsigned IndexOfSizeArg,
+ProgramStateRef State, Optional RetVal) {
if (!State)
MTC wrote:
> `const` qualifier missing?
This method was made `static`.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1323
+/// pointer to the type of the constructed record or one of its bases.
+static bool hasNonTrivialConstructorCall(const CXXNewExpr *NE) {
[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.
Szelethus added a comment. Let's also have the link to your most recent mail about this issue here: http://lists.llvm.org/pipermail/cfe-dev/2018-November/060038.html I re-read the mailing list conversation from 2016, @szepet's `MisusedMoveChecker` patch, so I have a better graps on whats happening. I think this really is non-trivial stuff. When I initially read your workbook, `SymbolReaper` (along with what inlining really is) was among the biggest unsolved mysteries for me. The explanation of it you wrote back in 2016[1] is //awesome//, I feel enlightened after reading it, it's very well constructed, but I find it unfortunate that it took me almost a year of working in the analyzer to find it. After looking at `SymbolManager.cpp`'s history, I can see that the logic didn't change since, I'd very strongly prefer to have this goldnugget copy-pasted even to a simple txt file, and have it commited with this patch. [1] http://lists.llvm.org/pipermail/cfe-dev/2016-March/048142.html In https://reviews.llvm.org/D18860#1306359, @Szelethus wrote: > In https://reviews.llvm.org/D18860#1297742, @NoQ wrote: > > > Nope, unit tests aren't quite useful here. I can't unit-test the behavior > > of API that i'm about to //remove//... right? > > > Hmmm, can't really argue with that, can I? :D Well, we probably should implement unit tests for these anyways, `SymbolReaper` is not only performance-critical, but is also among the few data structures that's an essential part of the analysis, and it's change in functionality could be very easily shown in dedicated test files. But that's an issue for another time, and until then, `debug.ExprInspection` still does the job well enough. Let's not delay this patch longer just because of that, and enjoy the post zombie apocalypse analyzer. https://reviews.llvm.org/D18860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54760: [clangd] Cleanup: make the diags callback global in TUScheduler
This revision was automatically updated to reflect the committed changes.
Closed by commit rL347474: [clangd] Cleanup: make the diags callback global in
TUScheduler (authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D54760?vs=175056=175061#toc
Repository:
rL LLVM
https://reviews.llvm.org/D54760
Files:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
@@ -11,6 +11,7 @@
#include "Matchers.h"
#include "TUScheduler.h"
#include "TestFS.h"
+#include "llvm/ADT/ScopeExit.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include
@@ -29,20 +30,71 @@
using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
-void ignoreUpdate(Optional>) {}
-
class TUSchedulerTests : public ::testing::Test {
protected:
ParseInputs getInputs(PathRef File, std::string Contents) {
return ParseInputs{*CDB.getCompileCommand(File),
buildTestFS(Files, Timestamps), std::move(Contents)};
}
+ void updateWithCallback(TUScheduler , PathRef File, StringRef Contents,
+ WantDiagnostics WD,
+ llvm::unique_function CB) {
+WithContextValue Ctx(llvm::make_scope_exit(std::move(CB)));
+S.update(File, getInputs(File, Contents), WD);
+ }
+
+ static Key)>>
+ DiagsCallbackKey;
+
+ /// A diagnostics callback that should be passed to TUScheduler when it's used
+ /// in updateWithDiags.
+ static std::unique_ptr captureDiags() {
+class CaptureDiags : public ParsingCallbacks {
+ void onDiagnostics(PathRef File, std::vector Diags) override {
+auto D = Context::current().get(DiagsCallbackKey);
+if (!D)
+ return;
+const_cast)> &> (
+*D)(File, Diags);
+ }
+};
+return llvm::make_unique();
+ }
+
+ /// Schedule an update and call \p CB with the diagnostics it produces, if
+ /// any. The TUScheduler should be created with captureDiags as a
+ /// DiagsCallback for this to work.
+ void updateWithDiags(TUScheduler , PathRef File, ParseInputs Inputs,
+ WantDiagnostics WD,
+ llvm::unique_function)> CB) {
+Path OrigFile = File.str();
+WithContextValue Ctx(
+DiagsCallbackKey,
+Bind(
+[OrigFile](decltype(CB) CB, PathRef File, std::vector Diags) {
+ assert(File == OrigFile);
+ CB(std::move(Diags));
+},
+std::move(CB)));
+S.update(File, std::move(Inputs), WD);
+ }
+
+ void updateWithDiags(TUScheduler , PathRef File, llvm::StringRef Contents,
+ WantDiagnostics WD,
+ llvm::unique_function)> CB) {
+return updateWithDiags(S, File, getInputs(File, Contents), WD,
+ std::move(CB));
+ }
+
StringMap Files;
StringMap Timestamps;
MockCompilationDatabase CDB;
};
+Key)>>
+TUSchedulerTests::DiagsCallbackKey;
+
TEST_F(TUSchedulerTests, MissingFiles) {
TUScheduler S(getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
@@ -55,7 +107,7 @@
auto Missing = testPath("missing.cpp");
Files[Missing] = "";
- S.update(Added, getInputs(Added, ""), WantDiagnostics::No, ignoreUpdate);
+ S.update(Added, getInputs(Added, ""), WantDiagnostics::No);
// Assert each operation for missing file is an error (even if it's available
// in VFS).
@@ -96,25 +148,25 @@
Notification Ready;
TUScheduler S(
getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
+/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
auto Path = testPath("foo.cpp");
-S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
- [&](std::vector) { Ready.wait(); });
-
-S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
- [&](std::vector Diags) { ++CallbackCount; });
-S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
- ADD_FAILURE() << "auto should have been cancelled by auto";
- });
-S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
- [&](std::vector Diags) {
- ADD_FAILURE() << "no diags should not be called back";
- });
[clang-tools-extra] r347474 - [clangd] Cleanup: make the diags callback global in TUScheduler
Author: ibiryukov
Date: Thu Nov 22 09:27:08 2018
New Revision: 347474
URL: http://llvm.org/viewvc/llvm-project?rev=347474=rev
Log:
[clangd] Cleanup: make the diags callback global in TUScheduler
Reviewers: sammccall
Reviewed By: sammccall
Subscribers: javed.absar, ioeric, MaskRay, jkorous, arphaman, kadircet,
cfe-commits
Differential Revision: https://reviews.llvm.org/D54760
Modified:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=347474=347473=347474=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Thu Nov 22 09:27:08 2018
@@ -65,26 +65,32 @@ public:
Optional> Result;
};
-} // namespace
-
-// Returns callbacks that can be used to update the FileIndex with new ASTs.
-static std::unique_ptr
-makeUpdateCallbacks(FileIndex *FIndex) {
- struct CB : public ParsingCallbacks {
-CB(FileIndex *FIndex) : FIndex(FIndex) {}
-FileIndex *FIndex;
-void onPreambleAST(PathRef Path, ASTContext ,
- std::shared_ptr PP) override {
+// Update the FileIndex with new ASTs and plumb the diagnostics responses.
+struct UpdateIndexCallbacks : public ParsingCallbacks {
+ UpdateIndexCallbacks(FileIndex *FIndex, DiagnosticsConsumer )
+ : FIndex(FIndex), DiagConsumer(DiagConsumer) {}
+
+ void onPreambleAST(PathRef Path, ASTContext ,
+ std::shared_ptr PP) override {
+if (FIndex)
FIndex->updatePreamble(Path, Ctx, std::move(PP));
-}
+ }
-void onMainAST(PathRef Path, ParsedAST ) override {
+ void onMainAST(PathRef Path, ParsedAST ) override {
+if (FIndex)
FIndex->updateMain(Path, AST);
-}
- };
- return llvm::make_unique(FIndex);
-}
+ }
+
+ void onDiagnostics(PathRef File, std::vector Diags) override {
+DiagConsumer.onDiagnosticsReady(File, std::move(Diags));
+ }
+
+private:
+ FileIndex *FIndex;
+ DiagnosticsConsumer
+};
+} // namespace
ClangdServer::Options ClangdServer::optsForTest() {
ClangdServer::Options Opts;
@@ -98,7 +104,7 @@ ClangdServer::ClangdServer(const GlobalC
const FileSystemProvider ,
DiagnosticsConsumer ,
const Options )
-: CDB(CDB), DiagConsumer(DiagConsumer), FSProvider(FSProvider),
+: CDB(CDB), FSProvider(FSProvider),
ResourceDir(Opts.ResourceDir ? *Opts.ResourceDir
: getStandardResourceDir()),
DynamicIdx(Opts.BuildDynamicSymbolIndex
@@ -112,8 +118,8 @@ ClangdServer::ClangdServer(const GlobalC
// FIXME(ioeric): this can be slow and we may be able to index on less
// critical paths.
WorkScheduler(Opts.AsyncThreadsCount, Opts.StorePreamblesInMemory,
-DynamicIdx ? makeUpdateCallbacks(DynamicIdx.get())
- : nullptr,
+llvm::make_unique(DynamicIdx.get(),
+DiagConsumer),
Opts.UpdateDebounce, Opts.RetentionPolicy) {
if (DynamicIdx && Opts.StaticIndex) {
MergedIdx =
@@ -129,14 +135,10 @@ ClangdServer::ClangdServer(const GlobalC
void ClangdServer::addDocument(PathRef File, StringRef Contents,
WantDiagnostics WantDiags) {
- ParseInputs Inputs = {getCompileCommand(File), FSProvider.getFileSystem(),
-Contents.str()};
- Path FileStr = File.str();
- WorkScheduler.update(File, std::move(Inputs), WantDiags,
- [this, FileStr](std::vector Diags) {
- DiagConsumer.onDiagnosticsReady(FileStr,
- std::move(Diags));
- });
+ WorkScheduler.update(File,
+ ParseInputs{getCompileCommand(File),
+ FSProvider.getFileSystem(), Contents.str()},
+ WantDiags);
}
void ClangdServer::removeDocument(PathRef File) {
Modified: clang-tools-extra/trunk/clangd/ClangdServer.h
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=347474=347473=347474=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.h (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.h Thu Nov 22 09:27:08 2018
@@ -226,7 +226,6 @@ private:
tooling::CompileCommand getCompileCommand(PathRef File);
const GlobalCompilationDatabase
-
[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull
martong added a subscriber: gamesh411. martong added a comment. @shafik , @gamesh411 I think when I used arcanist to update the patch it removed you as a reviewer and a subscriber. I am really sorry about this. Fortunately the Herald script added you guys back. And once you are here, this is a gentle ping. :) Repository: rC Clang https://reviews.llvm.org/D53755 ___ 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
martong added a comment.
Constructs like `struct A { struct Foo *p; };` are very common in C projects.
Since `Foo` as a forward decl cannot be found by normal lookup we need this
patch in order to properly CTU analyze even a simple C project like `tmux`.
Repository:
rC Clang
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
martong added a comment. Herald added a reviewer: shafik. Herald added a subscriber: gamesh411. Gentle Ping. Repository: rC Clang 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] D54013: [analyzer] NFC: MallocChecker: Avoid redundant transitions.
Szelethus added inline comments. Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2383-2384 + if (RS == OldRS) +return; + Hmmm, I guess we return here because if `RegionState` is unchanged, so should be `ReallocPairs` and `FreeReturnValue`, correct? https://reviews.llvm.org/D54013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53693: [ASTImporter] Typedef import brings in the complete type
martong added inline comments.
Herald added a reviewer: shafik.
Herald added a subscriber: gamesh411.
Comment at: lib/AST/ASTImporter.cpp:2310
+D->getUnderlyingType(), FoundTypedef->getUnderlyingType())) {
+ QualType FromUT = D->getUnderlyingType();
+ QualType FoundUT = FoundTypedef->getUnderlyingType();
a_sidorin wrote:
> We can move these two vars upper and use them in the condition.
Good point, changed it.
Comment at: lib/AST/ASTImporter.cpp:2314
+ // already have a complete underlying type then return with that.
+ if (!FromUT->isIncompleteType() && !FoundUT->isIncompleteType())
return Importer.MapImported(D, FoundTypedef);
a_sidorin wrote:
> This condition omits the case where both types are complete. Should we use
> `FromUT->isIncompleteType == FoundUT-isIncompleteType()` instead?
I think you wanted to say "both types are INcomplete".
The condition indeed omits the case where both types are incomplete and that is
a potential bug, thanks for finding it!
I think the best way to handle this case is to do the something similar what is
done in case of functions, i.e break the for loop once we know that the found
and the to be imported Decl are structurally equivalent.
So, I added a `break` to do that and to be similar what we do in case of
functions.
This is what we have with functions:
```
if (IsStructuralMatch(D, FoundFunction)) {
const FunctionDecl *Definition = nullptr;
if (D->doesThisDeclarationHaveABody() &&
FoundFunction->hasBody(Definition)) {
return Importer.MapImported(
D, const_cast(Definition));
}
FoundByLookup = FoundFunction;
break;
}
```
I'd like to have something similar with typedefs. The goal is to have only one
TypedefNameDecl with a complete type. A TypedefNameDecl with a complete type is
analogous to a FunctionDecl with a definition, we want to have only one in the
"To" context.
A TypedefNameDecl is also redeclarable, so on a long term we should connect a
new Decl to the found previous one.
Repository:
rC Clang
https://reviews.llvm.org/D53693
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates
martong marked 3 inline comments as done.
martong added inline comments.
Comment at: lib/AST/DeclBase.cpp:1469
assert(Pos != Map->end() && "no lookup entry for decl");
-if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND)
+// Remove the decl only if it is contained.
+if ((Pos->second.getAsVector() && Pos->second.containsInVector(ND)) ||
Szelethus wrote:
> balazske wrote:
> > martong wrote:
> > > Szelethus wrote:
> > > > Contained in?
> > > Indeed, `containedInVector` sounds better, so I renamed.
> > For me, `containsInVector` is the better name, or `hasInVector` ("contains"
> > is already used at other places but not "contained" and it sounds like it
> > is not contained any more).
> Sorry about the confusion, my inline was only about the comment above it,
> that it isn't obvious enough that //what// decl is contained in. But after
> taking a second look, it's very clear that only `Map` is mutated in this
> context, so I don't insist on any modification :)
Okay, so I just reverted the name change.
Repository:
rC Clang
https://reviews.llvm.org/D53655
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates
martong updated this revision to Diff 175058.
martong added a comment.
- Revert "Minor style changes and rename a function"
Repository:
rC Clang
https://reviews.llvm.org/D53655
Files:
include/clang/AST/DeclContextInternals.h
include/clang/ASTMatchers/ASTMatchers.h
lib/AST/ASTImporter.cpp
lib/AST/DeclBase.cpp
lib/ASTMatchers/ASTMatchersInternal.cpp
unittests/AST/ASTImporterTest.cpp
unittests/AST/StructuralEquivalenceTest.cpp
Index: unittests/AST/StructuralEquivalenceTest.cpp
===
--- unittests/AST/StructuralEquivalenceTest.cpp
+++ unittests/AST/StructuralEquivalenceTest.cpp
@@ -597,6 +597,77 @@
EXPECT_FALSE(testStructuralMatch(R0, R1));
}
+TEST_F(StructuralEquivalenceRecordTest, AnonymousRecordsShouldBeInequivalent) {
+ auto t = makeTuDecls(
+ R"(
+ struct X {
+struct {
+ int a;
+};
+struct {
+ int b;
+};
+ };
+ )",
+ "", Lang_C);
+ auto *TU = get<0>(t);
+ auto *A = FirstDeclMatcher().match(
+ TU, indirectFieldDecl(hasName("a")));
+ auto *FA = cast(A->chain().front());
+ RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+ auto *B = FirstDeclMatcher().match(
+ TU, indirectFieldDecl(hasName("b")));
+ auto *FB = cast(B->chain().front());
+ RecordDecl *RB = cast(FB->getType().getTypePtr())->getDecl();
+
+ ASSERT_NE(RA, RB);
+ EXPECT_TRUE(testStructuralMatch(RA, RA));
+ EXPECT_TRUE(testStructuralMatch(RB, RB));
+ EXPECT_FALSE(testStructuralMatch(RA, RB));
+}
+
+TEST_F(StructuralEquivalenceRecordTest,
+ RecordsAreInequivalentIfOrderOfAnonRecordsIsDifferent) {
+ auto t = makeTuDecls(
+ R"(
+ struct X {
+struct { int a; };
+struct { int b; };
+ };
+ )",
+ R"(
+ struct X { // The order is reversed.
+struct { int b; };
+struct { int a; };
+ };
+ )",
+ Lang_C);
+
+ auto *TU = get<0>(t);
+ auto *A = FirstDeclMatcher().match(
+ TU, indirectFieldDecl(hasName("a")));
+ auto *FA = cast(A->chain().front());
+ RecordDecl *RA = cast(FA->getType().getTypePtr())->getDecl();
+
+ auto *TU1 = get<1>(t);
+ auto *A1 = FirstDeclMatcher().match(
+ TU1, indirectFieldDecl(hasName("a")));
+ auto *FA1 = cast(A1->chain().front());
+ RecordDecl *RA1 = cast(FA1->getType().getTypePtr())->getDecl();
+
+ RecordDecl *X =
+ FirstDeclMatcher().match(TU, recordDecl(hasName("X")));
+ RecordDecl *X1 =
+ FirstDeclMatcher().match(TU1, recordDecl(hasName("X")));
+ ASSERT_NE(X, X1);
+ EXPECT_FALSE(testStructuralMatch(X, X1));
+
+ ASSERT_NE(RA, RA1);
+ EXPECT_TRUE(testStructuralMatch(RA, RA));
+ EXPECT_TRUE(testStructuralMatch(RA1, RA1));
+ EXPECT_FALSE(testStructuralMatch(RA1, RA));
+}
+
TEST_F(StructuralEquivalenceRecordTest,
UnnamedRecordsShouldBeInequivalentEvenIfTheSecondIsBeingDefined) {
auto Code =
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -11,9 +11,10 @@
//
//===--===//
+#include "clang/AST/ASTImporter.h"
#include "MatchVerifier.h"
#include "clang/AST/ASTContext.h"
-#include "clang/AST/ASTImporter.h"
+#include "clang/AST/DeclContextInternals.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Tooling/Tooling.h"
@@ -1808,6 +1809,65 @@
EXPECT_NE(To0->getCanonicalDecl(), To1->getCanonicalDecl());
}
+TEST_P(ASTImporterTestBase, AnonymousRecords) {
+ auto *Code =
+ R"(
+ struct X {
+struct { int a; };
+struct { int b; };
+ };
+ )";
+ Decl *FromTU0 = getTuDecl(Code, Lang_C, "input0.c");
+
+ Decl *FromTU1 = getTuDecl(Code, Lang_C, "input1.c");
+
+ auto *X0 =
+ FirstDeclMatcher().match(FromTU0, recordDecl(hasName("X")));
+ auto *X1 =
+ FirstDeclMatcher().match(FromTU1, recordDecl(hasName("X")));
+ Import(X0, Lang_C);
+ Import(X1, Lang_C);
+
+ auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+ // We expect no (ODR) warning during the import.
+ EXPECT_EQ(0u, ToTU->getASTContext().getDiagnostics().getNumWarnings());
+ EXPECT_EQ(1u,
+DeclCounter().match(ToTU, recordDecl(hasName("X";
+}
+
+TEST_P(ASTImporterTestBase, AnonymousRecordsReversed) {
+ Decl *FromTU0 = getTuDecl(
+ R"(
+ struct X {
+struct { int a; };
+struct { int b; };
+ };
+ )",
+ Lang_C, "input0.c");
+
+ Decl *FromTU1 = getTuDecl(
+ R"(
+ struct X { // reversed order
+struct { int b; };
+struct { int a; };
+ };
+ )",
+ Lang_C, "input1.c");
+
+ auto *X0 =
+ FirstDeclMatcher().match(FromTU0, recordDecl(hasName("X")));
+ auto *X1 =
+ FirstDeclMatcher().match(FromTU1, recordDecl(hasName("X")));
+ Import(X0,
[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions
MTC added a comment.
Thank you for doing the cleaning that no one else is interested in! Comments is
inline below.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342
+ DefaultBool IsOptimistic;
+ MemFunctionInfoTy MemFunctionInfo;
public:
I can't say that abstracting `MemFunctionInfo` is a perfect solution, for
example `IsOptimistic` and `ShouldIncludeOwnershipAnnotatedFunctions ` are
close to each other in the object layout, but they do the same thing, which
makes me feel weird. And there are so many `MemFunctionInfo.` :)
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:422
+ ///
+ /// \param [in] CE The expression that allocates memory.
+ /// \param [in] IndexOfSizeArg Index of the argument that specifies the size
`CE` typo?
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:428
+ /// if unspecified, the value of expression \p E is used.
+ static ProgramStateRef ProcessZeroAllocation(CheckerContext , const Expr
*E,
+ const unsigned IndexOfSizeArg,
Personally, `ProcessZeroAllocation` is like half the story, maybe
`ProcessZeroAllocCheck` or `ProcessZeroAllocationCheck` is better.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:448
+ /// \param [in] State The \c ProgramState right before allocation.
+ /// \returns The programstate right after allocation.
ProgramStateRef MallocMemReturnsAttr(CheckerContext ,
Maybe `program state` or just `ProgramState` is better?
Same as 462, 476, 507, 575, 593.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:592
+ /// \param [in] CE The expression that reallocated memory
+ /// \param [in] State The \c ProgramState right before reallocation.
+ /// \returns The programstate right after allocation.
`before realloction` typo?
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
if (FD->getKind() == Decl::Function) {
-initIdentifierInfo(C.getASTContext());
+MemFunctionInfo.initIdentifierInfo(C.getASTContext());
IdentifierInfo *FunI = FD->getIdentifier();
If I not wrong, `MemFunctionInfo` is mainly a class member of `MallocChecker`
that hold a bunch of memory function identifiers and provide corresponding
helper APIs. And we should pick a proper time to initialize it.
I want to known why you call `initIdentifierInfo()` in `checkPostStmt(const
CallExpr *E,)`, this callback is called after `checkPreCall(const CallEvent
, )` in which we need `MemFunctionInfo`.
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1247
+CheckerContext , const Expr *E, const unsigned IndexOfSizeArg,
+ProgramStateRef State, Optional RetVal) {
if (!State)
`const` qualifier missing?
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1323
+/// pointer to the type of the constructed record or one of its bases.
+static bool hasNonTrivialConstructorCall(const CXXNewExpr *NE) {
I'm not sure `hasNonTrivialConstructorCall()` is a good name although you
explained it in details in the comments below. And the comments for this
function is difficult for me to understand, which is maybe my problem.
And I also think this function doesn't do as much as described in the comments,
it is just `true if the invoked constructor in \p NE has a parameter - pointer
or reference to a record`, right?
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2407
const CallExpr *CE,
- bool FreesOnFail,
+ bool ShouldFreeOnFail,
ProgramStateRef State,
The parameter name `ShouldFreeOnFail` is not consistent with the declaration,
see line 577.
Repository:
rC Clang
https://reviews.llvm.org/D54823
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54796: [clangd] **Prototype**: C++ API for emitting file status
sammccall added inline comments.
Comment at: clangd/ClangdLSPServer.cpp:787
+ notify("window/showMessage",
+ json::Object {
+ {"uri", URI},
we should have a protocol model object (in Protocol.h) for the file status
updates.
I think it should *likely* it should be distinct from the FileStatus in
TUScheduler - optimizing for easy update/comparison and programmatic checks vs
optimizing for matching the desired protocol.
Naming is hard, I'd be tempted to take `FileStatus` for the protocol object and
and make the TUScheduler one something more obscure like `TUScheduler::TUState`.
Unclear to me which one ClangdServer should expose, my *guess* is we should
start by exposing the protocol object, and switch if it doesn't capture enough
of the semantics that embedders need.
Comment at: clangd/TUScheduler.cpp:338
auto Task = [=](decltype(OnUpdated) OnUpdated) mutable {
+OnUpdated(FileStatus{FileStatus::State::Building, "Building"}, {});
// Will be used to check if we can avoid rebuilding the AST.
So I'm slightly nervous about just emitting objects directly. The problem is if
you have independent properties (e.g. "is using a fallback command" vs "are we
computing diagnostics"), then it's a burden on the code here to pass them all
every time and a burden on the callback code if you don't.
I think one solution (which *may* be what Ilya's suggesting) is to put a
mutable FileStatus object in the ASTWorker, and mutate it in place and then
invoke the callback, passing the `const FileStatus&`.
Repository:
rCTE Clang Tools Extra
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] r347472 - [clangd] Cleanup error consumption code. NFC
Author: ibiryukov
Date: Thu Nov 22 08:20:12 2018
New Revision: 347472
URL: http://llvm.org/viewvc/llvm-project?rev=347472=rev
Log:
[clangd] Cleanup error consumption code. NFC
- Remove reimplementations of llvm::consumeError.
- Simplify test code by using EXPECT_ERROR where it fits.
Modified:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/unittests/clangd/Matchers.h
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
clang-tools-extra/trunk/unittests/clangd/URITests.cpp
Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=347472=347471=347472=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Thu Nov 22 08:20:12 2018
@@ -39,10 +39,6 @@ namespace clang {
namespace clangd {
namespace {
-void ignoreError(Error Err) {
- handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});
-}
-
std::string getStandardResourceDir() {
static int Dummy; // Just an address in this process.
return CompilerInvocation::GetResourcesPath("clangd", (void *));
@@ -312,7 +308,7 @@ void ClangdServer::dumpAST(PathRef File,
unique_function Callback) {
auto Action = [](decltype(Callback) Callback, Expected InpAST)
{
if (!InpAST) {
- ignoreError(InpAST.takeError());
+ llvm::consumeError(InpAST.takeError());
return Callback("");
}
std::string Result;
Modified: clang-tools-extra/trunk/unittests/clangd/Matchers.h
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/Matchers.h?rev=347472=347471=347472=diff
==
--- clang-tools-extra/trunk/unittests/clangd/Matchers.h (original)
+++ clang-tools-extra/trunk/unittests/clangd/Matchers.h Thu Nov 22 08:20:12 2018
@@ -125,9 +125,7 @@ PolySubsequenceMatcher HasSubse
<< ::testing::PrintToString(*ComputedValue);
\
break;
\
}
\
-handleAllErrors(ComputedValue.takeError(),
\
-[](const llvm::ErrorInfoBase &) {});
\
-
\
+llvm::consumeError(ComputedValue.takeError());
\
} while (false)
} // namespace clangd
Modified: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp?rev=347472=347471=347472=diff
==
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Thu Nov 22
08:20:12 2018
@@ -8,6 +8,7 @@
//===--===//
#include "Context.h"
+#include "Matchers.h"
#include "TUScheduler.h"
#include "TestFS.h"
#include "gmock/gmock.h"
@@ -29,9 +30,6 @@ using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
void ignoreUpdate(Optional>) {}
-void ignoreError(Error Err) {
- handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});
-}
class TUSchedulerTests : public ::testing::Test {
protected:
@@ -61,15 +59,11 @@ TEST_F(TUSchedulerTests, MissingFiles) {
// Assert each operation for missing file is an error (even if it's available
// in VFS).
- S.runWithAST("", Missing, [&](Expected AST) {
-ASSERT_FALSE(bool(AST));
-ignoreError(AST.takeError());
- });
- S.runWithPreamble("", Missing, TUScheduler::Stale,
-[&](Expected Preamble) {
- ASSERT_FALSE(bool(Preamble));
- ignoreError(Preamble.takeError());
-});
+ S.runWithAST("", Missing,
+ [&](Expected AST) { EXPECT_ERROR(AST); });
+ S.runWithPreamble(
+ "", Missing, TUScheduler::Stale,
+ [&](Expected Preamble) { EXPECT_ERROR(Preamble); });
// remove() shouldn't crash on missing files.
S.remove(Missing);
@@ -83,14 +77,12 @@ TEST_F(TUSchedulerTests, MissingFiles) {
S.remove(Added);
// Assert that all operations fail after removing the file.
- S.runWithAST("", Added, [&](Expected AST) {
-ASSERT_FALSE(bool(AST));
-ignoreError(AST.takeError());
- });
+ S.runWithAST("", Added,
+ [&](Expected AST) { EXPECT_ERROR(AST); });
S.runWithPreamble("", Added, TUScheduler::Stale,
[&](Expected Preamble) {
ASSERT_FALSE(bool(Preamble));
-
[PATCH] D54760: [clangd] Cleanup: make the diags callback global in TUScheduler
sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Everything looks so much better... Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54760 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r347470 - [clang-tidy] Ignore template instantiations in modernize-use-using
Author: alexfh
Date: Thu Nov 22 08:10:18 2018
New Revision: 347470
URL: http://llvm.org/viewvc/llvm-project?rev=347470=rev
Log:
[clang-tidy] Ignore template instantiations in modernize-use-using
The test I'm adding passes without the change due to the deduplication logic in
ClangTidyDiagnosticConsumer::take(). However this bug manifests in our internal
integration with clang-tidy.
I've verified the fix by locally changing LessClangTidyError to consider
replacements.
Modified:
clang-tools-extra/trunk/clang-tidy/modernize/UseUsingCheck.cpp
clang-tools-extra/trunk/test/clang-tidy/modernize-use-using.cpp
Modified: clang-tools-extra/trunk/clang-tidy/modernize/UseUsingCheck.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/UseUsingCheck.cpp?rev=347470=347469=347470=diff
==
--- clang-tools-extra/trunk/clang-tidy/modernize/UseUsingCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseUsingCheck.cpp Thu Nov 22
08:10:18 2018
@@ -24,7 +24,8 @@ UseUsingCheck::UseUsingCheck(StringRef N
void UseUsingCheck::registerMatchers(MatchFinder *Finder) {
if (!getLangOpts().CPlusPlus11)
return;
- Finder->addMatcher(typedefDecl().bind("typedef"), this);
+ Finder->addMatcher(typedefDecl(unless(isInstantiated())).bind("typedef"),
+ this);
}
// Checks if 'typedef' keyword can be removed - we do it only if
Modified: clang-tools-extra/trunk/test/clang-tidy/modernize-use-using.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/modernize-use-using.cpp?rev=347470=347469=347470=diff
==
--- clang-tools-extra/trunk/test/clang-tidy/modernize-use-using.cpp (original)
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-use-using.cpp Thu Nov 22
08:10:18 2018
@@ -162,3 +162,24 @@ typedef unsigned Map[lol];
typedef void (*fun_type)();
// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
// CHECK-FIXES: using fun_type = void (*)();
+
+namespace template_instantiations {
+template
+class C {
+ protected:
+ typedef C super;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use 'using' instead of 'typedef'
+ // CHECK-FIXES: using super = C;
+ virtual void f();
+
+public:
+ virtual ~C();
+};
+
+class D : public C {
+ void f() override { super::f(); }
+};
+class E : public C {
+ void f() override { super::f(); }
+};
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54760: [clangd] Cleanup: make the diags callback global in TUScheduler
ilya-biryukov updated this revision to Diff 175056.
ilya-biryukov added a comment.
- Rebase
- Remove makeUpdateCallbacks, expose the callback class instead
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54760
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
@@ -10,6 +10,7 @@
#include "Context.h"
#include "TUScheduler.h"
#include "TestFS.h"
+#include "llvm/ADT/ScopeExit.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include
@@ -28,7 +29,6 @@
using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
-void ignoreUpdate(Optional>) {}
void ignoreError(Error Err) {
handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});
}
@@ -40,11 +40,64 @@
buildTestFS(Files, Timestamps), std::move(Contents)};
}
+ void updateWithCallback(TUScheduler , PathRef File, StringRef Contents,
+ WantDiagnostics WD,
+ llvm::unique_function CB) {
+WithContextValue Ctx(llvm::make_scope_exit(std::move(CB)));
+S.update(File, getInputs(File, Contents), WD);
+ }
+
+ static Key)>>
+ DiagsCallbackKey;
+
+ /// A diagnostics callback that should be passed to TUScheduler when it's used
+ /// in updateWithDiags.
+ static std::unique_ptr captureDiags() {
+class CaptureDiags : public ParsingCallbacks {
+ void onDiagnostics(PathRef File, std::vector Diags) override {
+auto D = Context::current().get(DiagsCallbackKey);
+if (!D)
+ return;
+const_cast)> &> (
+*D)(File, Diags);
+ }
+};
+return llvm::make_unique();
+ }
+
+ /// Schedule an update and call \p CB with the diagnostics it produces, if
+ /// any. The TUScheduler should be created with captureDiags as a
+ /// DiagsCallback for this to work.
+ void updateWithDiags(TUScheduler , PathRef File, ParseInputs Inputs,
+ WantDiagnostics WD,
+ llvm::unique_function)> CB) {
+Path OrigFile = File.str();
+WithContextValue Ctx(
+DiagsCallbackKey,
+Bind(
+[OrigFile](decltype(CB) CB, PathRef File, std::vector Diags) {
+ assert(File == OrigFile);
+ CB(std::move(Diags));
+},
+std::move(CB)));
+S.update(File, std::move(Inputs), WD);
+ }
+
+ void updateWithDiags(TUScheduler , PathRef File, llvm::StringRef Contents,
+ WantDiagnostics WD,
+ llvm::unique_function)> CB) {
+return updateWithDiags(S, File, getInputs(File, Contents), WD,
+ std::move(CB));
+ }
+
StringMap Files;
StringMap Timestamps;
MockCompilationDatabase CDB;
};
+Key)>>
+TUSchedulerTests::DiagsCallbackKey;
+
TEST_F(TUSchedulerTests, MissingFiles) {
TUScheduler S(getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
@@ -57,7 +110,7 @@
auto Missing = testPath("missing.cpp");
Files[Missing] = "";
- S.update(Added, getInputs(Added, ""), WantDiagnostics::No, ignoreUpdate);
+ S.update(Added, getInputs(Added, ""), WantDiagnostics::No);
// Assert each operation for missing file is an error (even if it's available
// in VFS).
@@ -104,25 +157,25 @@
Notification Ready;
TUScheduler S(
getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
+/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
auto Path = testPath("foo.cpp");
-S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
- [&](std::vector) { Ready.wait(); });
-
-S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
- [&](std::vector Diags) { ++CallbackCount; });
-S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
- ADD_FAILURE() << "auto should have been cancelled by auto";
- });
-S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
- [&](std::vector Diags) {
- ADD_FAILURE() << "no diags should not be called back";
- });
-S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+updateWithDiags(S, Path, "", WantDiagnostics::Yes,
+[&](std::vector) { Ready.wait(); });
+updateWithDiags(S, Path, "request diags", WantDiagnostics::Yes,
+[&](std::vector) { ++CallbackCount; });
+updateWithDiags(S,
[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.
Szelethus added a comment. In https://reviews.llvm.org/D18860#1297742, @NoQ wrote: > Nope, unit tests aren't quite useful here. I can't unit-test the behavior of > API that i'm about to //remove//... right? Hmmm, can't really argue with that, can I? :D https://reviews.llvm.org/D18860 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54563: [analyzer] MoveChecker Pt.4: Add a few more state reset methods.
Szelethus added a comment. In https://reviews.llvm.org/D54563#1301893, @NoQ wrote: > In https://reviews.llvm.org/D54563#1299918, @Szelethus wrote: > > > How about `std::list::splice`? > > > Hmm, you mean that it leaves its argument empty? Interesting, i guess we may > need a separate facility for that. I looked it up, and this is what I found: (source: http://eel.is/c++draft/list.ops) > `list` provides three `splice` operations that destructively move elements > from one list to another. The behavior of splice operations is undefined if > `get_allocator() != x.get_allocator()`. > > void splice(const_iterator position, list& x); > void splice(const_iterator position, list&& x); > > **Effects:** Inserts the contents of `x` before position and `x` becomes > empty. Pointers and references to the moved elements of `x` now refer to > those same elements but as members of *this. Iterators referring to the moved > >elements will continue to refer to their elements, but they now behave as > iterators into `*this`, not into `x`. This suggests to me that `list1.splice(list1.begin(), std::move(list2))` leaves `list` in a valid, well-defined, but empty state. > I wonder how many other state-reset methods are we forgetting. `merge` does something similar too for `map`, `multimap`, `set`, `multiset`, as well as their unordered counterparts. > As a TODO, we might want to warn anyway when the value of the argument is not > 0. Maybe leave a `TODO` in the code as well? ^-^ Repository: rC Clang https://reviews.llvm.org/D54563 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54829: [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE347468: [clangd] Cleanup: make diagnostics callbacks from
TUScheduler non-racy (authored by ibiryukov, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D54829?vs=175053=175055#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54829
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -124,6 +124,8 @@
S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
[&](std::vector Diags) { ++CallbackCount; });
Ready.notify();
+
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
EXPECT_EQ(2, CallbackCount);
}
@@ -147,6 +149,8 @@
std::this_thread::sleep_for(std::chrono::seconds(2));
S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
[&](std::vector Diags) { ++CallbackCount; });
+
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
EXPECT_EQ(2, CallbackCount);
}
@@ -267,6 +271,8 @@
Update("U3");
Read("R3")();
Proceed.notify();
+
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
EXPECT_THAT(DiagsSeen, ElementsAre("U2", "U3"))
<< "U1 and all dependent reads were cancelled. "
@@ -373,6 +379,7 @@
}
}
}
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
} // TUScheduler destructor waits for all operations to finish.
std::lock_guard Lock(Mut);
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -748,7 +748,8 @@
BlockingRequests[RequestIndex]();
}
}
- } // Wait for ClangdServer to shutdown before proceeding.
+ASSERT_TRUE(Server.blockUntilIdleForTest());
+ }
// Check some invariants about the state of the program.
std::vector Stats = DiagConsumer.takeFileStats();
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -108,7 +108,8 @@
llvm::unique_function)> OnUpdated);
/// Remove \p File from the list of tracked files and schedule removal of its
- /// resources.
+ /// resources. Pending diagnostics for closed files may not be delivered, even
+ /// if requested with WantDiags::Auto or WantDiags::Yes.
void remove(PathRef File);
/// Schedule an async task with no dependencies.
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -125,7 +125,8 @@
WantDiagnostics WD = WantDiagnostics::Auto);
/// Remove \p File from list of tracked files, schedule a request to free
- /// resources associated with it.
+ /// resources associated with it. Pending diagnostics for closed files may not
+ /// be delivered, even if requested with WantDiags::Auto or WantDiags::Yes.
void removeDocument(PathRef File);
/// Run code completion for \p File at \p Pos.
@@ -222,20 +223,12 @@
formatCode(llvm::StringRef Code, PathRef File,
ArrayRef Ranges);
- typedef uint64_t DocVersion;
-
- void consumeDiagnostics(PathRef File, DocVersion Version,
- std::vector Diags);
-
tooling::CompileCommand getCompileCommand(PathRef File);
const GlobalCompilationDatabase
DiagnosticsConsumer
const FileSystemProvider
- /// Used to synchronize diagnostic responses for added and removed files.
- llvm::StringMap InternalVersion;
-
Path ResourceDir;
// The index used to look up symbols. This could be:
// - null (all index functionality is optional)
@@ -255,12 +248,6 @@
llvm::Optional WorkspaceRoot;
std::shared_ptr PCHs;
- /// Used to serialize diagnostic callbacks.
- /// FIXME(ibiryukov): get rid of an extra map and put all version counters
- /// into CppFile.
- std::mutex DiagnosticsMutex;
- /// Maps from a filename to the latest version of reported diagnostics.
- llvm::StringMap ReportedDiagnosticVersions;
// WorkScheduler has to be the last member, because its destructor has to be
// called before all other members to stop the worker thread that references
// ClangdServer.
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -252,6 +252,13 @@
bool Done;/* GUARDED_BY(Mutex) */
std::deque Requests; /* GUARDED_BY(Mutex) */
mutable std::condition_variable RequestsCV;
+ /// Guards a critical section
[clang-tools-extra] r347468 - [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy
Author: ibiryukov
Date: Thu Nov 22 07:39:54 2018
New Revision: 347468
URL: http://llvm.org/viewvc/llvm-project?rev=347468=rev
Log:
[clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy
Summary:
Previously, removeDoc followed by an addDoc to TUScheduler resulted in
racy diagnostic responses, i.e. the old dianostics could be delivered
to the client after the new ones by TUScheduler.
To workaround this, we tracked a version number in ClangdServer and
discarded stale diagnostics. After this commit, the TUScheduler will
stop delivering diagnostics for removed files and the workaround in
ClangdServer is not required anymore.
Reviewers: sammccall
Reviewed By: sammccall
Subscribers: javed.absar, ioeric, MaskRay, jkorous, arphaman, jfb, kadircet,
cfe-commits
Differential Revision: https://reviews.llvm.org/D54829
Modified:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=347468=347467=347468=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Thu Nov 22 07:39:54 2018
@@ -133,19 +133,17 @@ ClangdServer::ClangdServer(const GlobalC
void ClangdServer::addDocument(PathRef File, StringRef Contents,
WantDiagnostics WantDiags) {
- DocVersion Version = ++InternalVersion[File];
ParseInputs Inputs = {getCompileCommand(File), FSProvider.getFileSystem(),
Contents.str()};
-
Path FileStr = File.str();
WorkScheduler.update(File, std::move(Inputs), WantDiags,
- [this, FileStr, Version](std::vector Diags) {
- consumeDiagnostics(FileStr, Version,
std::move(Diags));
+ [this, FileStr](std::vector Diags) {
+ DiagConsumer.onDiagnosticsReady(FileStr,
+ std::move(Diags));
});
}
void ClangdServer::removeDocument(PathRef File) {
- ++InternalVersion[File];
WorkScheduler.remove(File);
}
@@ -444,24 +442,6 @@ void ClangdServer::findHover(PathRef Fil
WorkScheduler.runWithAST("Hover", File, Bind(Action, std::move(CB)));
}
-void ClangdServer::consumeDiagnostics(PathRef File, DocVersion Version,
- std::vector Diags) {
- // We need to serialize access to resulting diagnostics to avoid calling
- // `onDiagnosticsReady` in the wrong order.
- std::lock_guard DiagsLock(DiagnosticsMutex);
- DocVersion = ReportedDiagnosticVersions[File];
-
- // FIXME(ibiryukov): get rid of '<' comparison here. In the current
- // implementation diagnostics will not be reported after version counters'
- // overflow. This should not happen in practice, since DocVersion is a
- // 64-bit unsigned integer.
- if (Version < LastReportedDiagsVersion)
-return;
- LastReportedDiagsVersion = Version;
-
- DiagConsumer.onDiagnosticsReady(File, std::move(Diags));
-}
-
tooling::CompileCommand ClangdServer::getCompileCommand(PathRef File) {
trace::Span Span("GetCompileCommand");
Optional C = CDB.getCompileCommand(File);
Modified: clang-tools-extra/trunk/clangd/ClangdServer.h
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=347468=347467=347468=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.h (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.h Thu Nov 22 07:39:54 2018
@@ -125,7 +125,8 @@ public:
WantDiagnostics WD = WantDiagnostics::Auto);
/// Remove \p File from list of tracked files, schedule a request to free
- /// resources associated with it.
+ /// resources associated with it. Pending diagnostics for closed files may
not
+ /// be delivered, even if requested with WantDiags::Auto or WantDiags::Yes.
void removeDocument(PathRef File);
/// Run code completion for \p File at \p Pos.
@@ -222,20 +223,12 @@ private:
formatCode(llvm::StringRef Code, PathRef File,
ArrayRef Ranges);
- typedef uint64_t DocVersion;
-
- void consumeDiagnostics(PathRef File, DocVersion Version,
- std::vector Diags);
-
tooling::CompileCommand getCompileCommand(PathRef File);
const GlobalCompilationDatabase
DiagnosticsConsumer
const FileSystemProvider
- /// Used to synchronize diagnostic responses for added and removed files.
- llvm::StringMap InternalVersion;
-
Path
[PATCH] D52273: [clangd] Initial implementation of expected types
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clangd/ExpectedTypes.cpp:8
+
+namespace clang {
+namespace clangd {
nit: using namespace llvm (until/unless we switch other files)
Comment at: unittests/clangd/ExpectedTypeTest.cpp:33
+protected:
+ void build(llvm::StringRef Code) {
+assert(!AST && "AST built twice");
drop llvm:: here and below?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54834: [analyzer][MallocChecker] Improve warning messages on double-delete errors
Szelethus created this revision.
Szelethus added reviewers: george.karpenkov, NoQ, xazax.hun, rnkovacs.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy,
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, whisperity.
Repository:
rC Clang
https://reviews.llvm.org/D54834
Files:
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/Inputs/expected-plists/NewDelete-path-notes.cpp.plist
test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp
test/Analysis/NewDelete-checker-test.cpp
test/Analysis/NewDelete-path-notes.cpp
test/Analysis/dtor.cpp
Index: test/Analysis/dtor.cpp
===
--- test/Analysis/dtor.cpp
+++ test/Analysis/dtor.cpp
@@ -528,7 +528,7 @@
return *this;
}
~NonTrivial() {
-delete[] p; // expected-warning {{free released memory}}
+delete[] p; // expected-warning {{delete released memory}}
}
};
Index: test/Analysis/NewDelete-path-notes.cpp
===
--- test/Analysis/NewDelete-path-notes.cpp
+++ test/Analysis/NewDelete-path-notes.cpp
@@ -11,8 +11,8 @@
delete p;
// expected-note@-1 {{Memory is released}}
- delete p; // expected-warning {{Attempt to free released memory}}
- // expected-note@-1 {{Attempt to free released memory}}
+ delete p; // expected-warning {{Attempt to delete released memory}}
+ // expected-note@-1 {{Attempt to delete released memory}}
}
struct Odd {
@@ -24,7 +24,7 @@
void test(Odd *odd) {
odd->kill(); // expected-note{{Calling 'Odd::kill'}}
// expected-note@-1 {{Returning; memory was released}}
- delete odd; // expected-warning {{Attempt to free released memory}}
- // expected-note@-1 {{Attempt to free released memory}}
+ delete odd; // expected-warning {{Attempt to delete released memory}}
+ // expected-note@-1 {{Attempt to delete released memory}}
}
Index: test/Analysis/NewDelete-checker-test.cpp
===
--- test/Analysis/NewDelete-checker-test.cpp
+++ test/Analysis/NewDelete-checker-test.cpp
@@ -182,7 +182,7 @@
void testDoubleDelete() {
int *p = new int;
delete p;
- delete p; // expected-warning{{Attempt to free released memory}}
+ delete p; // expected-warning{{Attempt to delete released memory}}
}
void testExprDeleteArg() {
Index: test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp
===
--- test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp
+++ test/Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp
@@ -46,7 +46,7 @@
void testNewDoubleFree() {
int *p = new int;
delete p;
- delete p; // expected-warning{{Attempt to free released memory}}
+ delete p; // expected-warning{{Attempt to delete released memory}}
}
void testNewLeak() {
Index: test/Analysis/Inputs/expected-plists/NewDelete-path-notes.cpp.plist
===
--- test/Analysis/Inputs/expected-plists/NewDelete-path-notes.cpp.plist
+++ test/Analysis/Inputs/expected-plists/NewDelete-path-notes.cpp.plist
@@ -194,17 +194,17 @@
depth0
extended_message
- Attempt to free released memory
+ Attempt to delete released memory
message
- Attempt to free released memory
+ Attempt to delete released memory
- descriptionAttempt to free released memory
+ descriptionAttempt to delete released memory
categoryMemory error
- typeDouble free
+ typeDouble delete
check_namecplusplus.NewDelete
- issue_hash_content_of_line_in_contextbd8e324d09c70b9e2be6f824a4942e5a
+ issue_hash_content_of_line_in_context593b185245106bed5175ccf2753039b2
issue_context_kindfunction
issue_contexttest
issue_hash_function_offset8
@@ -423,17 +423,17 @@
depth0
extended_message
- Attempt to free released memory
+ Attempt to delete released memory
message
- Attempt to free released memory
+ Attempt to delete released memory
- descriptionAttempt to free released memory
+ descriptionAttempt to delete released memory
categoryMemory error
- typeDouble free
+ typeDouble delete
check_namecplusplus.NewDelete
- issue_hash_content_of_line_in_context8bf1a5b9fdae9d86780aa6c4cdce2605
+ issue_hash_content_of_line_in_context6484e9b006ede7362edef2187ba6eb37
issue_context_kindfunction
issue_contexttest
issue_hash_function_offset3
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1435,7 +1435,8 @@
void MallocChecker::checkPreStmt(const CXXDeleteExpr *DE,
CheckerContext ) const {
-
+ // This will regard deleting freed() regions as a
[PATCH] D54829: [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy
ilya-biryukov updated this revision to Diff 175053.
ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added a comment.
- Address the comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54829
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -124,6 +124,8 @@
S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
[&](std::vector Diags) { ++CallbackCount; });
Ready.notify();
+
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
EXPECT_EQ(2, CallbackCount);
}
@@ -147,6 +149,8 @@
std::this_thread::sleep_for(std::chrono::seconds(2));
S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
[&](std::vector Diags) { ++CallbackCount; });
+
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
EXPECT_EQ(2, CallbackCount);
}
@@ -304,6 +308,7 @@
}
}
}
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
} // TUScheduler destructor waits for all operations to finish.
std::lock_guard Lock(Mut);
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -748,7 +748,8 @@
BlockingRequests[RequestIndex]();
}
}
- } // Wait for ClangdServer to shutdown before proceeding.
+ASSERT_TRUE(Server.blockUntilIdleForTest());
+ }
// Check some invariants about the state of the program.
std::vector Stats = DiagConsumer.takeFileStats();
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -105,7 +105,8 @@
llvm::unique_function)> OnUpdated);
/// Remove \p File from the list of tracked files and schedule removal of its
- /// resources.
+ /// resources. Pending diagnostics for closed files may not be delivered, even
+ /// if requested with WantDiags::Auto or WantDiags::Yes.
void remove(PathRef File);
/// Schedule an async task with no dependencies.
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -251,6 +251,13 @@
bool Done;/* GUARDED_BY(Mutex) */
std::deque Requests; /* GUARDED_BY(Mutex) */
mutable std::condition_variable RequestsCV;
+ /// Guards a critical section for running the diagnostics callbacks.
+ std::mutex DiagsMu;
+ // Used to prevent remove document + leading to out-of-order diagnostics:
+ // The lifetime of the old/new ASTWorkers will overlap, but their handles
+ // don't. When the old handle is destroyed, the old worker will stop reporting
+ // diagnostics.
+ bool ReportDiagnostics = true; /* GUARDED_BY(DiagMu) */
};
/// A smart-pointer-like class that points to an active ASTWorker.
@@ -405,6 +412,14 @@
if (WantDiags == WantDiagnostics::No)
return;
+{
+ std::lock_guard Lock(DiagsMu);
+ // No need to rebuild the AST if we won't send the diagnotics. However,
+ // note that we don't prevent preamble rebuilds.
+ if (!ReportDiagnostics)
+return;
+}
+
// Get the AST for diagnostics.
Optional> AST = IdleASTs.take(this);
if (!AST) {
@@ -417,7 +432,11 @@
// spam us with updates.
// Note *AST can still be null if buildAST fails.
if (*AST) {
- OnUpdated((*AST)->getDiagnostics());
+ {
+std::lock_guard Lock(DiagsMu);
+if (ReportDiagnostics)
+ OnUpdated((*AST)->getDiagnostics());
+ }
trace::Span Span("Running main AST callback");
Callbacks.onMainAST(FileName, **AST);
DiagsWereReported = true;
@@ -509,6 +528,10 @@
bool ASTWorker::isASTCached() const { return IdleASTs.getUsedBytes(this) != 0; }
void ASTWorker::stop() {
+ {
+std::lock_guard Lock(DiagsMu);
+ReportDiagnostics = false;
+ }
{
std::lock_guard Lock(Mutex);
assert(!Done && "stop() called twice");
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -129,7 +129,8 @@
WantDiagnostics WD = WantDiagnostics::Auto);
/// Remove \p File from list of tracked files, schedule a request to free
- /// resources associated with it.
+ /// resources associated with it. Pending diagnostics for closed files may not
+ /// be delivered, even if requested with WantDiags::Auto or WantDiags::Yes.
void removeDocument(PathRef File);
/// Run code completion for \p File at \p Pos.
@@
[PATCH] D54829: [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy
ilya-biryukov added inline comments. Comment at: clangd/ClangdServer.h:132 /// Remove \p File from list of tracked files, schedule a request to free - /// resources associated with it. + /// resources associated with it. After this function returns, we guarantee + /// that no diagnostics for \p File will be delivered even if there are sammccall wrote: > I'm not sure we actually want to *guarantee* this in the API: this > implementation serializes removeDocument and the diagnostics callback, but > only because it was the most convenient. > > I'd suggest just "pending diagnostics for closed files may not be delivered". > (and similarly for TUScheduler). > > Up to you though. Yeah, it might be a bit stronger than what we guaranteed before (which was "you won't get stale diagnostics for a file that you reopen") Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54829 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54466: [Analyzer] Iterator Checkers - Use the region of the topmost base class for iterators stored in a region
Szelethus added a comment. In https://reviews.llvm.org/D54466#1305305, @baloghadamsoftware wrote: > In https://reviews.llvm.org/D54466#1297887, @NoQ wrote: > > > > Hmmm, shouldn't we add this to `MemRegion`'s interface instead? > > > This: > > > I wouldn't insist, but this does indeed sound useful. I suggest > > `MemRegion::getMostDerivedObjectRegion()` or something like that. > > vs > > > Also, ugh, that nomenclature: the base region of `CXXBaseObjectRegion` in > > fact represents the //derived// object. > > So if `CXXBaseObjectRegion` is the derived object, then > `MemRegion::getMostDerivedObjectRegion()` gets the least derived object > region now? Hmm, are there any particular reasons against renaming it to `CXXDerivedRegion`? https://reviews.llvm.org/D54466 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates
Szelethus added inline comments.
Comment at: lib/AST/DeclBase.cpp:1469
assert(Pos != Map->end() && "no lookup entry for decl");
-if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND)
+// Remove the decl only if it is contained.
+if ((Pos->second.getAsVector() && Pos->second.containsInVector(ND)) ||
balazske wrote:
> martong wrote:
> > Szelethus wrote:
> > > Contained in?
> > Indeed, `containedInVector` sounds better, so I renamed.
> For me, `containsInVector` is the better name, or `hasInVector` ("contains"
> is already used at other places but not "contained" and it sounds like it is
> not contained any more).
Sorry about the confusion, my inline was only about the comment above it, that
it isn't obvious enough that //what// decl is contained in. But after taking a
second look, it's very clear that only `Map` is mutated in this context, so I
don't insist on any modification :)
Repository:
rC Clang
https://reviews.llvm.org/D53655
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54800: [clangd] Cleanup: stop passing around list of supported URI schemes.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE347467: [clangd] Cleanup: stop passing around list of
supported URI schemes. (authored by ioeric, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D54800?vs=175048=175052#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54800
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/URI.cpp
clangd/URI.h
clangd/index/Background.cpp
clangd/index/Background.h
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Serialization.cpp
clangd/index/Serialization.h
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/dex/Dex.cpp
clangd/index/dex/Dex.h
clangd/index/dex/dexp/Dexp.cpp
clangd/tool/ClangdMain.cpp
test/clangd/protocol.test
unittests/clangd/BackgroundIndexTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/DexTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/FindSymbolsTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
unittests/clangd/TestTU.cpp
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -227,8 +227,8 @@
: InMemoryFileSystem(new vfs::InMemoryFileSystem),
TestHeaderName(testPath("symbol.h")),
TestFileName(testPath("symbol.cc")) {
-TestHeaderURI = URI::createFile(TestHeaderName).toString();
-TestFileURI = URI::createFile(TestFileName).toString();
+TestHeaderURI = URI::create(TestHeaderName).toString();
+TestFileURI = URI::create(TestFileName).toString();
}
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
@@ -391,7 +391,7 @@
- (void)someMethodName3:(void*)name3;
@end
)";
- TestFileName = "test.m";
+ TestFileName = testPath("test.m");
runSymbolCollector(Header, /*Main=*/"", {"-fblocks", "-xobjective-c++"});
EXPECT_THAT(Symbols,
UnorderedElementsAre(
@@ -534,38 +534,22 @@
TEST_F(SymbolCollectorTest, SymbolRelativeWithFallback) {
TestHeaderName = "x.h";
TestFileName = "x.cpp";
- TestHeaderURI = URI::createFile(testPath(TestHeaderName)).toString();
+ TestHeaderURI = URI::create(testPath(TestHeaderName)).toString();
CollectorOpts.FallbackDir = testRoot();
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols,
UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI;
}
-TEST_F(SymbolCollectorTest, CustomURIScheme) {
+TEST_F(SymbolCollectorTest, UnittestURIScheme) {
// Use test URI scheme from URITests.cpp
- CollectorOpts.URISchemes.insert(CollectorOpts.URISchemes.begin(), "unittest");
TestHeaderName = testPath("x.h");
TestFileName = testPath("x.cpp");
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols, UnorderedElementsAre(
AllOf(QName("Foo"), DeclURI("unittest:///x.h";
}
-TEST_F(SymbolCollectorTest, InvalidURIScheme) {
- // Use test URI scheme from URITests.cpp
- CollectorOpts.URISchemes = {"invalid"};
- runSymbolCollector("class Foo {};", /*Main=*/"");
- EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("Foo"), DeclURI("";
-}
-
-TEST_F(SymbolCollectorTest, FallbackToFileURI) {
- // Use test URI scheme from URITests.cpp
- CollectorOpts.URISchemes = {"invalid", "file"};
- runSymbolCollector("class Foo {};", /*Main=*/"");
- EXPECT_THAT(Symbols, UnorderedElementsAre(
- AllOf(QName("Foo"), DeclURI(TestHeaderURI;
-}
-
TEST_F(SymbolCollectorTest, IncludeEnums) {
const std::string Header = R"(
enum {
@@ -606,7 +590,7 @@
QName("(anonymous struct)::a")));
}
-TEST_F(SymbolCollectorTest, SymbolFormedFromMacro) {
+TEST_F(SymbolCollectorTest, SymbolFormedFromRegisteredSchemeFromMacro) {
Annotations Header(R"(
#define FF(name) \
@@ -765,7 +749,7 @@
// bits/basic_string.h$ should be mapped to
TestHeaderName = "/nasty/bits/basic_string.h";
TestFileName = "/nasty/bits/basic_string.cpp";
- TestHeaderURI = URI::createFile(TestHeaderName).toString();
+ TestHeaderURI = URI::create(TestHeaderName).toString();
runSymbolCollector("class string {};", /*Main=*/"");
EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("string"),
DeclURI(TestHeaderURI),
@@ -835,7 +819,7 @@
Includes.addMapping(TestHeaderName, "");
CollectorOpts.Includes =
auto IncFile = testPath("test.inc");
- auto IncURI = URI::createFile(IncFile).toString();
+ auto IncURI = URI::create(IncFile).toString();
InMemoryFileSystem->addFile(IncFile, 0,
MemoryBuffer::getMemBuffer("class X {};"));
runSymbolCollector("#include \"test.inc\"\nclass Y
[clang-tools-extra] r347467 - [clangd] Cleanup: stop passing around list of supported URI schemes.
Author: ioeric
Date: Thu Nov 22 07:02:05 2018
New Revision: 347467
URL: http://llvm.org/viewvc/llvm-project?rev=347467=rev
Log:
[clangd] Cleanup: stop passing around list of supported URI schemes.
Summary:
Instead of passing around a list of supported URI schemes in clangd, we
expose an interface to convert a path to URI using any compatible scheme
that has been registered. It favors customized schemes and falls
back to "file" when no other scheme works.
Changes in this patch are:
- URI::create(AbsPath, URISchemes) -> URI::create(AbsPath). The new API finds a
compatible scheme from the registry.
- Remove URISchemes option everywhere (ClangdServer, SymbolCollecter, FileIndex
etc).
- Unit tests will use "unittest" by default.
- Move "test" scheme from ClangdLSPServer to ClangdMain.cpp, and only
register the test scheme when lit-test or enable-lit-scheme is set.
(The new flag is added to make lit protocol.test work; I wonder if there
is alternative here.)
Reviewers: sammccall
Reviewed By: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits
Differential Revision: https://reviews.llvm.org/D54800
Modified:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/URI.cpp
clang-tools-extra/trunk/clangd/URI.h
clang-tools-extra/trunk/clangd/index/Background.cpp
clang-tools-extra/trunk/clangd/index/Background.h
clang-tools-extra/trunk/clangd/index/FileIndex.cpp
clang-tools-extra/trunk/clangd/index/FileIndex.h
clang-tools-extra/trunk/clangd/index/Serialization.cpp
clang-tools-extra/trunk/clangd/index/Serialization.h
clang-tools-extra/trunk/clangd/index/SymbolCollector.cpp
clang-tools-extra/trunk/clangd/index/SymbolCollector.h
clang-tools-extra/trunk/clangd/index/dex/Dex.cpp
clang-tools-extra/trunk/clangd/index/dex/Dex.h
clang-tools-extra/trunk/clangd/index/dex/dexp/Dexp.cpp
clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
clang-tools-extra/trunk/test/clangd/protocol.test
clang-tools-extra/trunk/unittests/clangd/BackgroundIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
clang-tools-extra/trunk/unittests/clangd/DexTests.cpp
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/FindSymbolsTests.cpp
clang-tools-extra/trunk/unittests/clangd/IndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
clang-tools-extra/trunk/unittests/clangd/TestTU.cpp
Modified: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp?rev=347467=347466=347467=diff
==
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp Thu Nov 22 07:02:05 2018
@@ -23,43 +23,6 @@ namespace clang {
namespace clangd {
namespace {
-/// \brief Supports a test URI scheme with relaxed constraints for lit tests.
-/// The path in a test URI will be combined with a platform-specific fake
-/// directory to form an absolute path. For example, test:///a.cpp is resolved
-/// C:\clangd-test\a.cpp on Windows and /clangd-test/a.cpp on Unix.
-class TestScheme : public URIScheme {
-public:
- Expected getAbsolutePath(StringRef /*Authority*/, StringRef
Body,
-StringRef /*HintPath*/) const override
{
-using namespace llvm::sys;
-// Still require "/" in body to mimic file scheme, as we want lengths of an
-// equivalent URI in both schemes to be the same.
-if (!Body.startswith("/"))
- return make_error(
- "Expect URI body to be an absolute path starting with '/': " + Body,
- inconvertibleErrorCode());
-Body = Body.ltrim('/');
-#ifdef _WIN32
-constexpr char TestDir[] = "C:\\clangd-test";
-#else
-constexpr char TestDir[] = "/clangd-test";
-#endif
-SmallVector Path(Body.begin(), Body.end());
-path::native(Path);
-auto Err = fs::make_absolute(TestDir, Path);
-if (Err)
- llvm_unreachable("Failed to make absolute path in test scheme.");
-return std::string(Path.begin(), Path.end());
- }
-
- Expected uriFromAbsolutePath(StringRef AbsolutePath) const override {
-llvm_unreachable("Clangd must never create a test URI.");
- }
-};
-
-static URISchemeRegistry::Add
-X("test", "Test scheme for clangd lit tests.");
-
SymbolKindBitset defaultSymbolKinds() {
SymbolKindBitset Defaults;
for (size_t I = SymbolKindMin; I <= static_cast(SymbolKind::Array);
Modified: clang-tools-extra/trunk/clangd/ClangdServer.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.cpp?rev=347467=347466=347467=diff
[PATCH] D54833: [clangd] Cleanup: use index file instead of header in workspace symbols lit test.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE347466: [clangd] Cleanup: use index file instead of header
in workspace symbols lit… (authored by ioeric, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D54833?vs=175047=175051#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54833
Files:
test/clangd/Inputs/sstream.h
test/clangd/Inputs/symbols.test.yaml
test/clangd/symbols.test
Index: test/clangd/Inputs/symbols.test.yaml
===
--- test/clangd/Inputs/symbols.test.yaml
+++ test/clangd/Inputs/symbols.test.yaml
@@ -0,0 +1,17 @@
+---
+!Symbol
+ID: 057557CEBF6E6B2D
+Name:'vector'
+Scope: 'std::'
+SymInfo:
+ Kind:Class
+ Lang:Cpp
+CanonicalDeclaration:
+ FileURI: 'file:///vector.h'
+ Start:
+Line:215
+Column: 10
+ End:
+Line:215
+Column: 16
+...
Index: test/clangd/symbols.test
===
--- test/clangd/symbols.test
+++ test/clangd/symbols.test
@@ -1,9 +1,9 @@
-# RUN: env CPATH=%S/Inputs clangd -lit-test < %s | FileCheck %s
+# RUN: clangd --index-file=%S/Inputs/symbols.test.yaml -lit-test < %s |
FileCheck %s
{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"workspace":{"symbol":{"symbolKind":{"valueSet":
[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26],"trace":"off"}}
---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"#include
\nvoid foo(); int main() { foo(); }\n"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void
foo(); int main() { foo(); }\n"}}}
---
-{"jsonrpc":"2.0","id":1,"method":"workspace/symbol","params":{"query":"std::basic_ostringstream"}}
+{"jsonrpc":"2.0","id":1,"method":"workspace/symbol","params":{"query":"vector"}}
# CHECK: "id": 1,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT:"result": [
@@ -21,9 +21,9 @@
# CHECK-NEXT: "line": {{.*}}
# CHECK-NEXT:}
# CHECK-NEXT: },
-# CHECK-NEXT: "uri": "file://{{.*}}/sstream.h"
+# CHECK-NEXT: "uri": "file:///vector.h"
# CHECK-NEXT:},
-# CHECK-NEXT:"name": "basic_ostringstream"
+# CHECK-NEXT:"name": "vector"
# CHECK-NEXT: }
# CHECK-NEXT:]
# CHECK-NEXT:}
Index: test/clangd/Inputs/sstream.h
===
--- test/clangd/Inputs/sstream.h
+++ test/clangd/Inputs/sstream.h
@@ -1,3 +0,0 @@
-namespace std {
-class basic_ostringstream {};
-}
Index: test/clangd/Inputs/symbols.test.yaml
===
--- test/clangd/Inputs/symbols.test.yaml
+++ test/clangd/Inputs/symbols.test.yaml
@@ -0,0 +1,17 @@
+---
+!Symbol
+ID: 057557CEBF6E6B2D
+Name:'vector'
+Scope: 'std::'
+SymInfo:
+ Kind:Class
+ Lang:Cpp
+CanonicalDeclaration:
+ FileURI: 'file:///vector.h'
+ Start:
+Line:215
+Column: 10
+ End:
+Line:215
+Column: 16
+...
Index: test/clangd/symbols.test
===
--- test/clangd/symbols.test
+++ test/clangd/symbols.test
@@ -1,9 +1,9 @@
-# RUN: env CPATH=%S/Inputs clangd -lit-test < %s | FileCheck %s
+# RUN: clangd --index-file=%S/Inputs/symbols.test.yaml -lit-test < %s | FileCheck %s
{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"workspace":{"symbol":{"symbolKind":{"valueSet": [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26],"trace":"off"}}
---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"#include \nvoid foo(); int main() { foo(); }\n"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
---
-{"jsonrpc":"2.0","id":1,"method":"workspace/symbol","params":{"query":"std::basic_ostringstream"}}
+{"jsonrpc":"2.0","id":1,"method":"workspace/symbol","params":{"query":"vector"}}
# CHECK: "id": 1,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT:"result": [
@@ -21,9 +21,9 @@
# CHECK-NEXT: "line": {{.*}}
# CHECK-NEXT:}
# CHECK-NEXT: },
-# CHECK-NEXT: "uri": "file://{{.*}}/sstream.h"
+# CHECK-NEXT: "uri": "file:///vector.h"
# CHECK-NEXT:
[clang-tools-extra] r347466 - [clangd] Cleanup: use index file instead of header in workspace symbols lit test.
Author: ioeric
Date: Thu Nov 22 06:59:22 2018
New Revision: 347466
URL: http://llvm.org/viewvc/llvm-project?rev=347466=rev
Log:
[clangd] Cleanup: use index file instead of header in workspace symbols lit
test.
Summary:
The full path of the input header depends on the execution environment
and may result in different behavior (e.g. when different URI schemes are used).
Reviewers: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits
Differential Revision: https://reviews.llvm.org/D54833
Added:
clang-tools-extra/trunk/test/clangd/Inputs/symbols.test.yaml
Removed:
clang-tools-extra/trunk/test/clangd/Inputs/sstream.h
Modified:
clang-tools-extra/trunk/test/clangd/symbols.test
Removed: clang-tools-extra/trunk/test/clangd/Inputs/sstream.h
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/Inputs/sstream.h?rev=347465=auto
==
--- clang-tools-extra/trunk/test/clangd/Inputs/sstream.h (original)
+++ clang-tools-extra/trunk/test/clangd/Inputs/sstream.h (removed)
@@ -1,3 +0,0 @@
-namespace std {
-class basic_ostringstream {};
-}
Added: clang-tools-extra/trunk/test/clangd/Inputs/symbols.test.yaml
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/Inputs/symbols.test.yaml?rev=347466=auto
==
--- clang-tools-extra/trunk/test/clangd/Inputs/symbols.test.yaml (added)
+++ clang-tools-extra/trunk/test/clangd/Inputs/symbols.test.yaml Thu Nov 22
06:59:22 2018
@@ -0,0 +1,17 @@
+---
+!Symbol
+ID: 057557CEBF6E6B2D
+Name:'vector'
+Scope: 'std::'
+SymInfo:
+ Kind:Class
+ Lang:Cpp
+CanonicalDeclaration:
+ FileURI: 'file:///vector.h'
+ Start:
+Line:215
+Column: 10
+ End:
+Line:215
+Column: 16
+...
Modified: clang-tools-extra/trunk/test/clangd/symbols.test
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clangd/symbols.test?rev=347466=347465=347466=diff
==
--- clang-tools-extra/trunk/test/clangd/symbols.test (original)
+++ clang-tools-extra/trunk/test/clangd/symbols.test Thu Nov 22 06:59:22 2018
@@ -1,9 +1,9 @@
-# RUN: env CPATH=%S/Inputs clangd -lit-test < %s | FileCheck %s
+# RUN: clangd --index-file=%S/Inputs/symbols.test.yaml -lit-test < %s |
FileCheck %s
{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"workspace":{"symbol":{"symbolKind":{"valueSet":
[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26],"trace":"off"}}
---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"#include
\nvoid foo(); int main() { foo(); }\n"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void
foo(); int main() { foo(); }\n"}}}
---
-{"jsonrpc":"2.0","id":1,"method":"workspace/symbol","params":{"query":"std::basic_ostringstream"}}
+{"jsonrpc":"2.0","id":1,"method":"workspace/symbol","params":{"query":"vector"}}
# CHECK: "id": 1,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT:"result": [
@@ -21,9 +21,9 @@
# CHECK-NEXT: "line": {{.*}}
# CHECK-NEXT:}
# CHECK-NEXT: },
-# CHECK-NEXT: "uri": "file://{{.*}}/sstream.h"
+# CHECK-NEXT: "uri": "file:///vector.h"
# CHECK-NEXT:},
-# CHECK-NEXT:"name": "basic_ostringstream"
+# CHECK-NEXT:"name": "vector"
# CHECK-NEXT: }
# CHECK-NEXT:]
# CHECK-NEXT:}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov added inline comments.
Comment at: clangd/ExpectedTypes.h:32
+/// this allows the representation to be stored in the index and compared with
+/// types coming from a different AST later.
+class OpaqueType {
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > Does this need to be a separate class rather than using `std::string`?
> > > There are echoes of `SymbolID` here, but there were some factors that
> > > don't apply here:
> > > - it was fixed-width
> > > - memory layout was important as we stored lots of these in memory
> > > - we hashed them a lot and wanted a specific hash function
> > >
> > > I suspect at least initially producing a somewhat readable std::string a
> > > la USRGeneration would be enough.
> > Would still want to keep it as a marker type just for the sake of
> > indicating what we return and documentation purposes.
> > It also adds some type safety (granted, not much) for some use-cases.
> >
> > There's still an option to go strings with `rawStr()` if needed.
> For documentation purposes, `using OpaqueType = std::string` or so seems like
> a reasonable compromise?
>
> This is very heavyweight for the amount of typesafety we get.
> (Apart from the class itself, you've got `==` and `!=`, we should definitely
> have `<<` as well, `DenseMapInfo<>` and `<` may get added down the line...)
As discussed offline, kept the class with an expectation that we'll use the
fixed-size representation at some point. Added a comment that it can be viewed
as a strong typedef to string for now.
Comment at: clangd/ExpectedTypes.h:42
+ /// completion context.
+ static llvm::Optional fromPreferredType(ASTContext ,
+ QualType Type);
sammccall wrote:
> I'd suggest just `fromType`, exposing this as the primary method, and then on
> `fromCompletionResult` document why it's different.
>
> Having the names suggest the underlying structure (that `fromType` is "more
> fundamental") aids understanding, and doesn't really feel like we're
> painting ourselves into a corner.
>
> Alternately, `fromCompletionContext` and `fromCompletionResult` would be more
> clearly symmetrical.
Done. Using `fromType` now.
Comment at: clangd/ExpectedTypes.h:59
+private:
+ static llvm::Optional encode(ASTContext , QualType Type);
+ explicit OpaqueType(std::string Data);
sammccall wrote:
> any reason to put this in the header?
It uses a private constructor of the class, so it seems natural for it to be a
private static function.
Comment at: unittests/clangd/ExpectedTypeTest.cpp:51
+
+class ConvertibleToMatcher
+: public ::testing::MatcherInterface {
sammccall wrote:
> "convertible to" is a problematic description for a couple of reasons:
> - it's a relationship between types, but encapsulates unrelated semantics to
> do with completions
> - it's a higher level of abstraction than the code under test
>
> As discussed offline/below, I think the best remedy here is just to drop this
> matcher - it's only used in one test that can now live with something much
> simpler.
Done. It was needed only for one test, testing it diretly now.
Comment at: unittests/clangd/ExpectedTypeTest.cpp:142
+ decl("iptr"), decl("bptr"), decl("user_type")};
+ EXPECT_THAT(buildEquivClasses(Decls), ClassesAre({{"b", "i", "ui", "ll"},
+{"f", "d"},
sammccall wrote:
> Ooh, we should avoid folding bool with other integer types I think!
>
> You hardly ever want to pass a bool where an int is expected. (The reverse
> int -> bool is somewhat common, but no more than pointer -> bool... type
> equivalence isn't the right hammer to solve that case).
Fair point, changed this. Bool requires a whole different handling anyway, e.g.
I definitely want my pointers to be boosted in if conditions.
Comment at: unittests/clangd/ExpectedTypeTest.cpp:173
+
+TEST_F(ExpectedTypeConversionTest, FunctionReturns) {
+ build(R"cpp(
sammccall wrote:
> I think this test is a bit too high-level - there are big abstractions
> between the test code and the code under test (which is pretty simple).
>
> I'd suggest just
> `EXPECT_EQ(
> OpaqueType::fromCompletionResult(ASTCtx(), decl("returns_int")),
> OpaqueType::fromExpectedType(ASTCtx(), decl("int_"));`
>
> (If you think there's something worth testing for the pointer case, I'd do
> that instead rather than as well)
Done. There is still a helper variable per case (I think it improves the
readability a little), but otherwise the test is more straightforward now.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
___
cfe-commits mailing list
[PATCH] D52273: [clangd] Initial implementation of expected types
ilya-biryukov updated this revision to Diff 175050.
ilya-biryukov marked 11 inline comments as done.
ilya-biryukov added a comment.
- Address comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52273
Files:
clangd/CMakeLists.txt
clangd/ExpectedTypes.cpp
clangd/ExpectedTypes.h
unittests/clangd/CMakeLists.txt
unittests/clangd/ExpectedTypeTest.cpp
Index: unittests/clangd/ExpectedTypeTest.cpp
===
--- /dev/null
+++ unittests/clangd/ExpectedTypeTest.cpp
@@ -0,0 +1,157 @@
+//===-- ExpectedTypeTest.cpp ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ClangdUnit.h"
+#include "ExpectedTypes.h"
+#include "TestTU.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+#include "llvm/ADT/StringRef.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace clang {
+namespace clangd {
+namespace {
+
+using ::testing::Field;
+using ::testing::Matcher;
+using ::testing::SizeIs;
+using ::testing::UnorderedElementsAreArray;
+
+class ExpectedTypeConversionTest : public ::testing::Test {
+protected:
+ void build(llvm::StringRef Code) {
+assert(!AST && "AST built twice");
+AST = TestTU::withCode(Code).build();
+ }
+
+ const ValueDecl *decl(llvm::StringRef Name) {
+return ::cast(findDecl(*AST, Name));
+ }
+
+ QualType typeOf(llvm::StringRef Name) {
+return decl(Name)->getType().getCanonicalType();
+ }
+
+ /// An overload for convenience.
+ Optional fromCompletionResult(const ValueDecl *D) {
+return OpaqueType::fromCompletionResult(
+ASTCtx(), CodeCompletionResult(D, CCP_Declaration));
+ }
+
+ /// A set of DeclNames whose type match each other computed by
+ /// OpaqueType::fromCompletionResult.
+ using EquivClass = std::set;
+
+ Matcher>
+ ClassesAre(llvm::ArrayRef Classes) {
+using MapEntry = std::map::value_type;
+
+std::vector> Elements;
+Elements.reserve(Classes.size());
+for (auto : Classes)
+ Elements.push_back(Field(::second, Cls));
+return UnorderedElementsAreArray(Elements);
+ }
+
+ // Groups \p Decls into equivalence classes based on the result of
+ // 'OpaqueType::fromCompletionResult'.
+ std::map
+ buildEquivClasses(llvm::ArrayRef DeclNames) {
+std::map Classes;
+for (llvm::StringRef Name : DeclNames) {
+ auto Type = OpaqueType::fromType(ASTCtx(), typeOf(Name));
+ Classes[Type->raw()].insert(Name);
+}
+return Classes;
+ }
+
+ ASTContext () { return AST->getASTContext(); }
+
+private:
+ // Set after calling build().
+ llvm::Optional AST;
+};
+
+TEST_F(ExpectedTypeConversionTest, BasicTypes) {
+ build(R"cpp(
+// ints.
+bool b;
+int i;
+unsigned int ui;
+long long ll;
+
+// floats.
+float f;
+double d;
+
+// pointers
+int* iptr;
+bool* bptr;
+
+// user-defined types.
+struct X {};
+X user_type;
+ )cpp");
+
+ EXPECT_THAT(buildEquivClasses({"b", "i", "ui", "ll", "f", "d", "iptr", "bptr",
+ "user_type"}),
+ ClassesAre({{"b"},
+ {"i", "ui", "ll"},
+ {"f", "d"},
+ {"iptr"},
+ {"bptr"},
+ {"user_type"}}));
+}
+
+TEST_F(ExpectedTypeConversionTest, ReferencesDontMatter) {
+ build(R"cpp(
+int noref;
+int & ref = noref;
+const int & const_ref = noref;
+int && rv_ref = 10;
+ )cpp");
+
+ EXPECT_THAT(buildEquivClasses({"noref", "ref", "const_ref", "rv_ref"}),
+ SizeIs(1));
+}
+
+TEST_F(ExpectedTypeConversionTest, ArraysDecay) {
+ build(R"cpp(
+ int arr[2];
+ int (_ref)[2] = arr;
+ int *ptr;
+ )cpp");
+
+ EXPECT_THAT(buildEquivClasses({"arr", "arr_ref", "ptr"}), SizeIs(1));
+}
+
+TEST_F(ExpectedTypeConversionTest, FunctionReturns) {
+ build(R"cpp(
+ int returns_int();
+ int* returns_ptr();
+
+ int int_;
+ int* int_ptr;
+ )cpp");
+
+ OpaqueType IntTy = *OpaqueType::fromType(ASTCtx(), typeOf("int_"));
+ EXPECT_EQ(fromCompletionResult(decl("returns_int")), IntTy);
+
+ OpaqueType IntPtrTy = *OpaqueType::fromType(ASTCtx(), typeOf("int_ptr"));
+ EXPECT_EQ(fromCompletionResult(decl("returns_ptr")), IntPtrTy);
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: unittests/clangd/CMakeLists.txt
===
--- unittests/clangd/CMakeLists.txt
+++ unittests/clangd/CMakeLists.txt
@@ -19,6 +19,7 @@
ContextTests.cpp
DexTests.cpp
DraftStoreTests.cpp
+ ExpectedTypeTest.cpp
FileDistanceTests.cpp
[PATCH] D52311: [clangd] Add support for hierarchical documentSymbol
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: unittests/clangd/FindSymbolsTests.cpp:442
+SymNameRange(Main.range("decl"),
+ AllOf(WithName("f"), WithKind(SymbolKind::Method),
+SymNameRange(Main.range("def");
ilya-biryukov wrote:
> sammccall wrote:
> > this one is to be fixed, right?
> Why? The outline view gives both the declaration and the definition, since
> both were written in the code.
> That seems to be in line with what I'd expect from the outline view.
Sorry, I meant the name should be `Foo::def` instead of `def`, which you
mentioned in a previous comment.
OK to land without this, but I think it deserves a fixme.
(If you think this is the *right* behavior, let's discuss further!)
Comment at: unittests/clangd/FindSymbolsTests.cpp:521
+ChildrenAre(AllOf(WithName("x"),
WithKind(SymbolKind::Field,
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct),
+ChildrenAre(WithName("y"))),
ilya-biryukov wrote:
> sammccall wrote:
> > hmm, this seems pretty confusing - I think `Tmpl` would be a clearer
> > name for a specialization, even if we just have `Tmpl` for the primary
> > template.
> > Partial specializations are confusing, though :-/
> Done. Now prints as `Tmpl`.
> This may also include some arguments not written by the users (e.g. some
> default args), but added a FIXME to fix this, it's not entirely trivial
Nice! I thought this was going to be really hard.
(You can specialize without naming the defaulted args? That sounds... obscure.
Definitely fine to leave for later)
Comment at: unittests/clangd/FindSymbolsTests.cpp:523
+ChildrenAre(WithName("y"))),
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct), NoChildren()),
+ AllOf(WithName("Tmpl"), WithKind(SymbolKind::Struct),
NoChildren(;
ilya-biryukov wrote:
> sammccall wrote:
> > why the change in policy with this patch? (one of these previously was
> > deliberately not indexed, now is)
> We might have different goals in mind here.
> From my perspective, the purpose of the outline is to cover all things
> written in the code: there are 4 decls that the user wrote: one primary
> template, one template specialization and two template instantiations.
>
> What is your model here?
I don't have a particular opinion, just wondering if this was a deliberate
change or fell out of the implementation.
(The old behavior was tested, so I guess Marc-Andre had a reason, but it didn't
come up in the review. I don't think this is common enough to worry much about
either way)
Comment at: unittests/clangd/FindSymbolsTests.cpp:571
addFile(FilePath, R"(
enum {
Red
ilya-biryukov wrote:
> sammccall wrote:
> > hmm, our handling of anonymous enums seems different than anonymous
> > namespaces - why?
> No rigorous reasons. The namespaces tend to group more things together, so
> it's arguably more useful to have an option of folding their subitems in the
> tree.
> Anonymous enums are typically used only in the classes (and on the top-level
> in C to define constants?) and I feel having an extra node there merely
> produces noise.
> Happy to reconsider, what's your preferred behavior?
I *think* I might prefer the opposite - namespaces flat and enums grouped :-)
I'm not certain nor adamant about either, so consider my thoughts below, and
pick something.
**Namespaces**: anonymous namespaces are mostly (entirely?) used to control
visibility. I don't think they *group* in a meaningful way, they just hide
things like `static` does. In a perfect world I think there'd be a "private"
bit on outline elements that was shown in the UI, and we'd put it on both
things-in-anon-namespaces and static-as-in-private decls. The main
counterargument I can see: because we typically don't indent inside namespaces,
the enclosing anonymous namespace can be hard to recognize and jump to.
**Enums**: conversely, I think these _typically_ do group their elements, and
there's often a strong semantic boundary between the last entry in the enum and
the first decl after it. When people use a namespacing prefix (C-style VK_Foo,
VK_Bar) then it's visually noticeable, but this is far from universal in C++
(particularly at class scope).
**Consistency**: It's not *really* confusing if these are different, just
*slightly* confusing. Anonymous structs etc must certainly be grouped, so this
is a (weak) argument for grouping everything.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D52311
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
[PATCH] D54795: [clang-format] Do not treat asm clobber [ as ObjCExpr, refined
This revision was automatically updated to reflect the committed changes.
Closed by commit rC347465: [clang-format] Do not treat asm clobber [ as
ObjCExpr, refined (authored by krasimir, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D54795?vs=175038=175049#toc
Repository:
rC Clang
https://reviews.llvm.org/D54795
Files:
lib/Format/TokenAnnotator.cpp
unittests/Format/FormatTest.cpp
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -399,14 +399,15 @@
bool IsCpp11AttributeSpecifier = isCpp11AttributeSpecifier(*Left) ||
Contexts.back().InCpp11AttributeSpecifier;
+bool InsideInlineASM = Line.startsWith(tok::kw_asm);
bool StartsObjCMethodExpr =
-!CppArrayTemplates && Style.isCpp() && !IsCpp11AttributeSpecifier &&
-Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) &&
+!InsideInlineASM && !CppArrayTemplates && Style.isCpp() &&
+!IsCpp11AttributeSpecifier && Contexts.back().CanBeExpression &&
+Left->isNot(TT_LambdaLSquare) &&
!CurrentToken->isOneOf(tok::l_brace, tok::r_square) &&
(!Parent ||
- (Parent->is(tok::colon) && Parent->isNot(TT_InlineASMColon)) ||
- Parent->isOneOf(tok::l_square, tok::l_paren, tok::kw_return,
- tok::kw_throw) ||
+ Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren,
+ tok::kw_return, tok::kw_throw) ||
Parent->isUnaryOperator() ||
// FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) ||
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12762,6 +12762,30 @@
" : [d] \"=rm\" (d)\n"
" [e] \"rm\" (*e));\n"
"}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " _asm (\"mov %[e], %[d]\"\n"
+ " : [d] \"=rm\" (d)\n"
+ " [e] \"rm\" (*e));\n"
+ "}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " __asm (\"mov %[e], %[d]\"\n"
+ " : [d] \"=rm\" (d)\n"
+ " [e] \"rm\" (*e));\n"
+ "}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " __asm__ (\"mov %[e],
%[d]\"\n"
+ " : [d] \"=rm\" (d)\n"
+ " [e] \"rm\" (*e));\n"
+ "}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " asm (\"mov %[e], %[d]\"\n"
+ " : [d] \"=rm\" (d),\n"
+ " [e] \"rm\" (*e));\n"
+ "}"));
EXPECT_EQ(FormatStyle::LK_Cpp,
guessLanguage("foo.h", "void f() {\n"
" asm volatile (\"mov %[e], %[d]\"\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -399,14 +399,15 @@
bool IsCpp11AttributeSpecifier = isCpp11AttributeSpecifier(*Left) ||
Contexts.back().InCpp11AttributeSpecifier;
+bool InsideInlineASM = Line.startsWith(tok::kw_asm);
bool StartsObjCMethodExpr =
-!CppArrayTemplates && Style.isCpp() && !IsCpp11AttributeSpecifier &&
-Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) &&
+!InsideInlineASM && !CppArrayTemplates && Style.isCpp() &&
+!IsCpp11AttributeSpecifier && Contexts.back().CanBeExpression &&
+Left->isNot(TT_LambdaLSquare) &&
!CurrentToken->isOneOf(tok::l_brace, tok::r_square) &&
(!Parent ||
- (Parent->is(tok::colon) && Parent->isNot(TT_InlineASMColon)) ||
- Parent->isOneOf(tok::l_square, tok::l_paren, tok::kw_return,
-
r347465 - [clang-format] Do not treat asm clobber [ as ObjCExpr, refined
Author: krasimir
Date: Thu Nov 22 06:49:55 2018
New Revision: 347465
URL: http://llvm.org/viewvc/llvm-project?rev=347465=rev
Log:
[clang-format] Do not treat asm clobber [ as ObjCExpr, refined
Summary:
r346756 refined clang-format to not treat the `[` in `asm (...: [] ..)` as an
ObjCExpr. However that's not enough, as we might have a comma-separated list of
such clobbers as in the newly added test.
This updates the detection to instead look at the Line's first token being `asm`
and not mark `[`-s as ObjCExprs in this case.
Reviewers: djasper, benhamilton
Reviewed By: djasper, benhamilton
Subscribers: benhamilton, cfe-commits
Differential Revision: https://reviews.llvm.org/D54795
Modified:
cfe/trunk/lib/Format/TokenAnnotator.cpp
cfe/trunk/unittests/Format/FormatTest.cpp
Modified: cfe/trunk/lib/Format/TokenAnnotator.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Format/TokenAnnotator.cpp?rev=347465=347464=347465=diff
==
--- cfe/trunk/lib/Format/TokenAnnotator.cpp (original)
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp Thu Nov 22 06:49:55 2018
@@ -399,14 +399,15 @@ private:
bool IsCpp11AttributeSpecifier = isCpp11AttributeSpecifier(*Left) ||
Contexts.back().InCpp11AttributeSpecifier;
+bool InsideInlineASM = Line.startsWith(tok::kw_asm);
bool StartsObjCMethodExpr =
-!CppArrayTemplates && Style.isCpp() && !IsCpp11AttributeSpecifier &&
-Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) &&
+!InsideInlineASM && !CppArrayTemplates && Style.isCpp() &&
+!IsCpp11AttributeSpecifier && Contexts.back().CanBeExpression &&
+Left->isNot(TT_LambdaLSquare) &&
!CurrentToken->isOneOf(tok::l_brace, tok::r_square) &&
(!Parent ||
- (Parent->is(tok::colon) && Parent->isNot(TT_InlineASMColon)) ||
- Parent->isOneOf(tok::l_square, tok::l_paren, tok::kw_return,
- tok::kw_throw) ||
+ Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren,
+ tok::kw_return, tok::kw_throw) ||
Parent->isUnaryOperator() ||
// FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) ||
Modified: cfe/trunk/unittests/Format/FormatTest.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Format/FormatTest.cpp?rev=347465=347464=347465=diff
==
--- cfe/trunk/unittests/Format/FormatTest.cpp (original)
+++ cfe/trunk/unittests/Format/FormatTest.cpp Thu Nov 22 06:49:55 2018
@@ -12762,6 +12762,30 @@ TEST_F(FormatTest, GuessedLanguageWithIn
" : [d] \"=rm\" (d)\n"
" [e] \"rm\" (*e));\n"
"}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " _asm (\"mov %[e], %[d]\"\n"
+ " : [d] \"=rm\" (d)\n"
+ " [e] \"rm\" (*e));\n"
+ "}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " __asm (\"mov %[e], %[d]\"\n"
+ " : [d] \"=rm\" (d)\n"
+ " [e] \"rm\" (*e));\n"
+ "}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " __asm__ (\"mov %[e],
%[d]\"\n"
+ " : [d] \"=rm\" (d)\n"
+ " [e] \"rm\" (*e));\n"
+ "}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " asm (\"mov %[e], %[d]\"\n"
+ " : [d] \"=rm\" (d),\n"
+ " [e] \"rm\" (*e));\n"
+ "}"));
EXPECT_EQ(FormatStyle::LK_Cpp,
guessLanguage("foo.h", "void f() {\n"
" asm volatile (\"mov %[e], %[d]\"\n"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54800: [clangd] Cleanup: stop passing around list of supported URI schemes.
ioeric updated this revision to Diff 175048.
ioeric marked 3 inline comments as done.
ioeric added a comment.
- address review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54800
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/URI.cpp
clangd/URI.h
clangd/index/Background.cpp
clangd/index/Background.h
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
clangd/index/Serialization.cpp
clangd/index/Serialization.h
clangd/index/SymbolCollector.cpp
clangd/index/SymbolCollector.h
clangd/index/dex/Dex.cpp
clangd/index/dex/Dex.h
clangd/index/dex/dexp/Dexp.cpp
clangd/tool/ClangdMain.cpp
test/clangd/protocol.test
unittests/clangd/BackgroundIndexTests.cpp
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/DexTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/FindSymbolsTests.cpp
unittests/clangd/IndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
unittests/clangd/TestTU.cpp
Index: unittests/clangd/TestTU.cpp
===
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -59,8 +59,7 @@
std::unique_ptr TestTU::index() const {
auto AST = build();
- auto Idx = llvm::make_unique(
- /*URISchemes=*/std::vector{}, /*UseDex=*/true);
+ auto Idx = llvm::make_unique(/*UseDex=*/true);
Idx->updatePreamble(Filename, AST.getASTContext(), AST.getPreprocessorPtr());
Idx->updateMain(Filename, AST);
return std::move(Idx);
Index: unittests/clangd/SymbolCollectorTests.cpp
===
--- unittests/clangd/SymbolCollectorTests.cpp
+++ unittests/clangd/SymbolCollectorTests.cpp
@@ -227,8 +227,8 @@
: InMemoryFileSystem(new vfs::InMemoryFileSystem),
TestHeaderName(testPath("symbol.h")),
TestFileName(testPath("symbol.cc")) {
-TestHeaderURI = URI::createFile(TestHeaderName).toString();
-TestFileURI = URI::createFile(TestFileName).toString();
+TestHeaderURI = URI::create(TestHeaderName).toString();
+TestFileURI = URI::create(TestFileName).toString();
}
bool runSymbolCollector(StringRef HeaderCode, StringRef MainCode,
@@ -391,7 +391,7 @@
- (void)someMethodName3:(void*)name3;
@end
)";
- TestFileName = "test.m";
+ TestFileName = testPath("test.m");
runSymbolCollector(Header, /*Main=*/"", {"-fblocks", "-xobjective-c++"});
EXPECT_THAT(Symbols,
UnorderedElementsAre(
@@ -534,38 +534,22 @@
TEST_F(SymbolCollectorTest, SymbolRelativeWithFallback) {
TestHeaderName = "x.h";
TestFileName = "x.cpp";
- TestHeaderURI = URI::createFile(testPath(TestHeaderName)).toString();
+ TestHeaderURI = URI::create(testPath(TestHeaderName)).toString();
CollectorOpts.FallbackDir = testRoot();
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols,
UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI;
}
-TEST_F(SymbolCollectorTest, CustomURIScheme) {
+TEST_F(SymbolCollectorTest, UnittestURIScheme) {
// Use test URI scheme from URITests.cpp
- CollectorOpts.URISchemes.insert(CollectorOpts.URISchemes.begin(), "unittest");
TestHeaderName = testPath("x.h");
TestFileName = testPath("x.cpp");
runSymbolCollector("class Foo {};", /*Main=*/"");
EXPECT_THAT(Symbols, UnorderedElementsAre(
AllOf(QName("Foo"), DeclURI("unittest:///x.h";
}
-TEST_F(SymbolCollectorTest, InvalidURIScheme) {
- // Use test URI scheme from URITests.cpp
- CollectorOpts.URISchemes = {"invalid"};
- runSymbolCollector("class Foo {};", /*Main=*/"");
- EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("Foo"), DeclURI("";
-}
-
-TEST_F(SymbolCollectorTest, FallbackToFileURI) {
- // Use test URI scheme from URITests.cpp
- CollectorOpts.URISchemes = {"invalid", "file"};
- runSymbolCollector("class Foo {};", /*Main=*/"");
- EXPECT_THAT(Symbols, UnorderedElementsAre(
- AllOf(QName("Foo"), DeclURI(TestHeaderURI;
-}
-
TEST_F(SymbolCollectorTest, IncludeEnums) {
const std::string Header = R"(
enum {
@@ -606,7 +590,7 @@
QName("(anonymous struct)::a")));
}
-TEST_F(SymbolCollectorTest, SymbolFormedFromMacro) {
+TEST_F(SymbolCollectorTest, SymbolFormedFromRegisteredSchemeFromMacro) {
Annotations Header(R"(
#define FF(name) \
@@ -765,7 +749,7 @@
// bits/basic_string.h$ should be mapped to
TestHeaderName = "/nasty/bits/basic_string.h";
TestFileName = "/nasty/bits/basic_string.cpp";
- TestHeaderURI = URI::createFile(TestHeaderName).toString();
+ TestHeaderURI = URI::create(TestHeaderName).toString();
runSymbolCollector("class string {};", /*Main=*/"");
EXPECT_THAT(Symbols, UnorderedElementsAre(AllOf(QName("string"),
DeclURI(TestHeaderURI),
@@ -835,7 +819,7 @@
r347463 - Revert rL347462 "[ASTMatchers] Add hasSideEffect() matcher."
Author: courbet
Date: Thu Nov 22 06:26:33 2018
New Revision: 347463
URL: http://llvm.org/viewvc/llvm-project?rev=347463=rev
Log:
Revert rL347462 "[ASTMatchers] Add hasSideEffect() matcher."
Breaks some buildbots.
Modified:
cfe/trunk/docs/LibASTMatchersReference.html
cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
Modified: cfe/trunk/docs/LibASTMatchersReference.html
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/LibASTMatchersReference.html?rev=347463=347462=347463=diff
==
--- cfe/trunk/docs/LibASTMatchersReference.html (original)
+++ cfe/trunk/docs/LibASTMatchersReference.html Thu Nov 22 06:26:33 2018
@@ -2817,24 +2817,6 @@ enum class Y {};
-Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprhasSideEffects
-Matches expressions
with potential side effects other than producing
-a value, such as a calling a function, throwing an exception, or reading a
-volatile variable.
-
-Given
- void f(int a, int b, volatile int c) {
-call();
-a = 0;
-a;
-b;
-c;
- }
-expr(hasSideEffects())
- matches 'call()', 'a = 0', 'c', but not '0', 'a', 'b'.
-
-
-
Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprisInstantiationDependent
Matches
expressions that are instantiation-dependent even if it is
neither type- nor value-dependent.
Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=347463=347462=347463=diff
==
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Thu Nov 22 06:26:33 2018
@@ -4118,26 +4118,6 @@ AST_MATCHER_P(IfStmt, hasConditionVariab
InnerMatcher.matches(*DeclarationStatement, Finder, Builder);
}
-/// \brief Matches expressions with potential side effects other than producing
-/// a value, such as a calling a function, throwing an exception, or reading a
-/// volatile variable.
-///
-/// Given
-/// \code
-/// void f(int& a, int b, volatile int c) {
-/// call();
-/// a = 0;
-/// a;
-/// b;
-/// c;
-/// }
-/// \endcode
-/// expr(hasSideEffects())
-/// matches 'call()', 'a = 0', 'c', but not '0', 'a', 'b'.
-AST_MATCHER(Expr, hasSideEffects) {
- return Node.HasSideEffects(Finder->getASTContext());
-}
-
/// Matches the index expression of an array subscript expression.
///
/// Given
Modified: cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp?rev=347463=347462=347463=diff
==
--- cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp (original)
+++ cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp Thu Nov 22 06:26:33 2018
@@ -294,7 +294,6 @@ RegistryMaps::RegistryMaps() {
REGISTER_MATCHER(hasReturnValue);
REGISTER_MATCHER(hasRHS);
REGISTER_MATCHER(hasSelector);
- REGISTER_MATCHER(hasSideEffects);
REGISTER_MATCHER(hasSingleDecl);
REGISTER_MATCHER(hasSize);
REGISTER_MATCHER(hasSizeExpr);
Modified: cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp?rev=347463=347462=347463=diff
==
--- cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp (original)
+++ cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp Thu Nov 22
06:26:33 2018
@@ -2259,21 +2259,5 @@ TEST(Matcher, isMain) {
notMatches("int main2() {}", functionDecl(isMain(;
}
-TEST(Matcher, hasSideEffects) {
- EXPECT_TRUE(matches("void call();"
- "void f() { call(); }",
- expr(hasSideEffects(;
- EXPECT_TRUE(matches("void f(int& a) { a = 0; }", expr(hasSideEffects(;
- EXPECT_TRUE(
- matches("void f(volatile int a) { (void)a; }", expr(hasSideEffects(;
-
- EXPECT_TRUE(notMatches("void call();"
- "void f() { }",
- expr(hasSideEffects(;
- EXPECT_TRUE(
- notMatches("void f(int& a) { (void)a; }", expr(hasSideEffects(;
- EXPECT_TRUE(notMatches("void f(int a) { (void)a; }",
expr(hasSideEffects(;
-}
-
} // namespace ast_matchers
} // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54833: [clangd] Cleanup: use index file instead of header in workspace symbols lit test.
ioeric created this revision.
ioeric added a reviewer: sammccall.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay,
ilya-biryukov.
The full path of the input header depends on the execution environment
and may result in different behavior (e.g. when different URI schemes are used).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54833
Files:
test/clangd/Inputs/sstream.h
test/clangd/Inputs/symbols.test.yaml
test/clangd/symbols.test
Index: test/clangd/symbols.test
===
--- test/clangd/symbols.test
+++ test/clangd/symbols.test
@@ -1,9 +1,9 @@
-# RUN: env CPATH=%S/Inputs clangd -lit-test < %s | FileCheck %s
+# RUN: clangd --index-file=%S/Inputs/symbols.test.yaml -lit-test < %s |
FileCheck %s
{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"workspace":{"symbol":{"symbolKind":{"valueSet":
[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26],"trace":"off"}}
---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"#include
\nvoid foo(); int main() { foo(); }\n"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void
foo(); int main() { foo(); }\n"}}}
---
-{"jsonrpc":"2.0","id":1,"method":"workspace/symbol","params":{"query":"std::basic_ostringstream"}}
+{"jsonrpc":"2.0","id":1,"method":"workspace/symbol","params":{"query":"vector"}}
# CHECK: "id": 1,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT:"result": [
@@ -21,9 +21,9 @@
# CHECK-NEXT: "line": {{.*}}
# CHECK-NEXT:}
# CHECK-NEXT: },
-# CHECK-NEXT: "uri": "file://{{.*}}/sstream.h"
+# CHECK-NEXT: "uri": "file:///vector.h"
# CHECK-NEXT:},
-# CHECK-NEXT:"name": "basic_ostringstream"
+# CHECK-NEXT:"name": "vector"
# CHECK-NEXT: }
# CHECK-NEXT:]
# CHECK-NEXT:}
Index: test/clangd/Inputs/symbols.test.yaml
===
--- /dev/null
+++ test/clangd/Inputs/symbols.test.yaml
@@ -0,0 +1,17 @@
+---
+!Symbol
+ID: 057557CEBF6E6B2D
+Name:'vector'
+Scope: 'std::'
+SymInfo:
+ Kind:Class
+ Lang:Cpp
+CanonicalDeclaration:
+ FileURI: 'file:///vector.h'
+ Start:
+Line:215
+Column: 10
+ End:
+Line:215
+Column: 16
+...
Index: test/clangd/Inputs/sstream.h
===
--- test/clangd/Inputs/sstream.h
+++ /dev/null
@@ -1,3 +0,0 @@
-namespace std {
-class basic_ostringstream {};
-}
Index: test/clangd/symbols.test
===
--- test/clangd/symbols.test
+++ test/clangd/symbols.test
@@ -1,9 +1,9 @@
-# RUN: env CPATH=%S/Inputs clangd -lit-test < %s | FileCheck %s
+# RUN: clangd --index-file=%S/Inputs/symbols.test.yaml -lit-test < %s | FileCheck %s
{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{"workspace":{"symbol":{"symbolKind":{"valueSet": [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26],"trace":"off"}}
---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"#include \nvoid foo(); int main() { foo(); }\n"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"void foo(); int main() { foo(); }\n"}}}
---
-{"jsonrpc":"2.0","id":1,"method":"workspace/symbol","params":{"query":"std::basic_ostringstream"}}
+{"jsonrpc":"2.0","id":1,"method":"workspace/symbol","params":{"query":"vector"}}
# CHECK: "id": 1,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT:"result": [
@@ -21,9 +21,9 @@
# CHECK-NEXT: "line": {{.*}}
# CHECK-NEXT:}
# CHECK-NEXT: },
-# CHECK-NEXT: "uri": "file://{{.*}}/sstream.h"
+# CHECK-NEXT: "uri": "file:///vector.h"
# CHECK-NEXT:},
-# CHECK-NEXT:"name": "basic_ostringstream"
+# CHECK-NEXT:"name": "vector"
# CHECK-NEXT: }
# CHECK-NEXT:]
# CHECK-NEXT:}
Index: test/clangd/Inputs/symbols.test.yaml
===
--- /dev/null
+++ test/clangd/Inputs/symbols.test.yaml
@@ -0,0 +1,17 @@
+---
+!Symbol
+ID: 057557CEBF6E6B2D
+Name:'vector'
+Scope: 'std::'
+SymInfo:
+ Kind:Class
+ Lang:Cpp
+CanonicalDeclaration:
+ FileURI: 'file:///vector.h'
+ Start:
+Line:215
+
[PATCH] D54832: [clang-tidy] No fixes for auto new expression in smart check
hokein created this revision.
hokein added a reviewer: aaron.ballman.
Herald added a subscriber: xazax.hun.
The fix for `auto` new expression is illegal.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54832
Files:
clang-tidy/modernize/MakeSmartPtrCheck.cpp
test/clang-tidy/modernize-make-unique.cpp
Index: test/clang-tidy/modernize-make-unique.cpp
===
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -282,6 +282,11 @@
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
// CHECK-FIXES: PE1 = std::make_unique();
+ // No fixes for `auto` new expression.
+ PE1.reset(new auto(E()));
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
+ // CHECK-FIXES: PE1.reset(new auto(E()));
+
//
// NOTE: For initlializer-list constructors, the check only gives warnings,
// and no fixes are generated.
Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -252,6 +252,9 @@
bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder ,
const CXXNewExpr *New, SourceManager ,
ASTContext *Ctx) {
+ // Skip when this is a new-expression with `auto`, e.g. "new auto(1)"."
+ if (New->getType()->getPointeeType()->getContainedAutoType())
+return false;
auto SkipParensParents = [&](const Expr *E) {
for (const Expr *OldE = nullptr; E != OldE;) {
OldE = E;
Index: test/clang-tidy/modernize-make-unique.cpp
===
--- test/clang-tidy/modernize-make-unique.cpp
+++ test/clang-tidy/modernize-make-unique.cpp
@@ -282,6 +282,11 @@
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
// CHECK-FIXES: PE1 = std::make_unique();
+ // No fixes for `auto` new expression.
+ PE1.reset(new auto(E()));
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use std::make_unique instead
+ // CHECK-FIXES: PE1.reset(new auto(E()));
+
//
// NOTE: For initlializer-list constructors, the check only gives warnings,
// and no fixes are generated.
Index: clang-tidy/modernize/MakeSmartPtrCheck.cpp
===
--- clang-tidy/modernize/MakeSmartPtrCheck.cpp
+++ clang-tidy/modernize/MakeSmartPtrCheck.cpp
@@ -252,6 +252,9 @@
bool MakeSmartPtrCheck::replaceNew(DiagnosticBuilder ,
const CXXNewExpr *New, SourceManager ,
ASTContext *Ctx) {
+ // Skip when this is a new-expression with `auto`, e.g. "new auto(1)"."
+ if (New->getType()->getPointeeType()->getContainedAutoType())
+return false;
auto SkipParensParents = [&](const Expr *E) {
for (const Expr *OldE = nullptr; E != OldE;) {
OldE = E;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54830: [ASTMatchers] Add hasSideEffect() matcher.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC347462: [ASTMatchers] Add hasSideEffect() matcher. (authored
by courbet, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D54830?vs=175044=175045#toc
Repository:
rC Clang
https://reviews.llvm.org/D54830
Files:
docs/LibASTMatchersReference.html
include/clang/ASTMatchers/ASTMatchers.h
lib/ASTMatchers/Dynamic/Registry.cpp
unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -294,6 +294,7 @@
REGISTER_MATCHER(hasReturnValue);
REGISTER_MATCHER(hasRHS);
REGISTER_MATCHER(hasSelector);
+ REGISTER_MATCHER(hasSideEffects);
REGISTER_MATCHER(hasSingleDecl);
REGISTER_MATCHER(hasSize);
REGISTER_MATCHER(hasSizeExpr);
Index: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2259,5 +2259,21 @@
notMatches("int main2() {}", functionDecl(isMain(;
}
+TEST(Matcher, hasSideEffects) {
+ EXPECT_TRUE(matches("void call();"
+ "void f() { call(); }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(matches("void f(int& a) { a = 0; }", expr(hasSideEffects(;
+ EXPECT_TRUE(
+ matches("void f(volatile int a) { (void)a; }", expr(hasSideEffects(;
+
+ EXPECT_TRUE(notMatches("void call();"
+ "void f() { }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(
+ notMatches("void f(int& a) { (void)a; }", expr(hasSideEffects(;
+ EXPECT_TRUE(notMatches("void f(int a) { (void)a; }",
expr(hasSideEffects(;
+}
+
} // namespace ast_matchers
} // namespace clang
Index: docs/LibASTMatchersReference.html
===
--- docs/LibASTMatchersReference.html
+++ docs/LibASTMatchersReference.html
@@ -2817,6 +2817,24 @@
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprhasSideEffects
+Matches expressions
with potential side effects other than producing
+a value, such as a calling a function, throwing an exception, or reading a
+volatile variable.
+
+Given
+ void f(int a, int b, volatile int c) {
+call();
+a = 0;
+a;
+b;
+c;
+ }
+expr(hasSideEffects())
+ matches 'call()', 'a = 0', 'c', but not '0', 'a', 'b'.
+
+
+
Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprisInstantiationDependent
Matches
expressions that are instantiation-dependent even if it is
neither type- nor value-dependent.
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -4118,6 +4118,26 @@
InnerMatcher.matches(*DeclarationStatement, Finder, Builder);
}
+/// \brief Matches expressions with potential side effects other than producing
+/// a value, such as a calling a function, throwing an exception, or reading a
+/// volatile variable.
+///
+/// Given
+/// \code
+/// void f(int& a, int b, volatile int c) {
+/// call();
+/// a = 0;
+/// a;
+/// b;
+/// c;
+/// }
+/// \endcode
+/// expr(hasSideEffects())
+/// matches 'call()', 'a = 0', 'c', but not '0', 'a', 'b'.
+AST_MATCHER(Expr, hasSideEffects) {
+ return Node.HasSideEffects(Finder->getASTContext());
+}
+
/// Matches the index expression of an array subscript expression.
///
/// Given
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -294,6 +294,7 @@
REGISTER_MATCHER(hasReturnValue);
REGISTER_MATCHER(hasRHS);
REGISTER_MATCHER(hasSelector);
+ REGISTER_MATCHER(hasSideEffects);
REGISTER_MATCHER(hasSingleDecl);
REGISTER_MATCHER(hasSize);
REGISTER_MATCHER(hasSizeExpr);
Index: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2259,5 +2259,21 @@
notMatches("int main2() {}", functionDecl(isMain(;
}
+TEST(Matcher, hasSideEffects) {
+ EXPECT_TRUE(matches("void call();"
+ "void f() { call(); }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(matches("void f(int& a) { a = 0; }", expr(hasSideEffects(;
+ EXPECT_TRUE(
+ matches("void f(volatile int a) { (void)a; }", expr(hasSideEffects(;
+
+ EXPECT_TRUE(notMatches("void call();"
+ "void f() { }",
+
r347462 - [ASTMatchers] Add hasSideEffect() matcher.
Author: courbet
Date: Thu Nov 22 06:00:56 2018
New Revision: 347462
URL: http://llvm.org/viewvc/llvm-project?rev=347462=rev
Log:
[ASTMatchers] Add hasSideEffect() matcher.
Summary: Exposes Expr::HasSideEffects.
Reviewers: aaron.ballman
Subscribers: cfe-commits
Differential Revision: https://reviews.llvm.org/D54830
Modified:
cfe/trunk/docs/LibASTMatchersReference.html
cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
Modified: cfe/trunk/docs/LibASTMatchersReference.html
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/LibASTMatchersReference.html?rev=347462=347461=347462=diff
==
--- cfe/trunk/docs/LibASTMatchersReference.html (original)
+++ cfe/trunk/docs/LibASTMatchersReference.html Thu Nov 22 06:00:56 2018
@@ -2817,6 +2817,24 @@ enum class Y {};
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprhasSideEffects
+Matches expressions
with potential side effects other than producing
+a value, such as a calling a function, throwing an exception, or reading a
+volatile variable.
+
+Given
+ void f(int a, int b, volatile int c) {
+call();
+a = 0;
+a;
+b;
+c;
+ }
+expr(hasSideEffects())
+ matches 'call()', 'a = 0', 'c', but not '0', 'a', 'b'.
+
+
+
Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprisInstantiationDependent
Matches
expressions that are instantiation-dependent even if it is
neither type- nor value-dependent.
Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=347462=347461=347462=diff
==
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original)
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Thu Nov 22 06:00:56 2018
@@ -4118,6 +4118,26 @@ AST_MATCHER_P(IfStmt, hasConditionVariab
InnerMatcher.matches(*DeclarationStatement, Finder, Builder);
}
+/// \brief Matches expressions with potential side effects other than producing
+/// a value, such as a calling a function, throwing an exception, or reading a
+/// volatile variable.
+///
+/// Given
+/// \code
+/// void f(int& a, int b, volatile int c) {
+/// call();
+/// a = 0;
+/// a;
+/// b;
+/// c;
+/// }
+/// \endcode
+/// expr(hasSideEffects())
+/// matches 'call()', 'a = 0', 'c', but not '0', 'a', 'b'.
+AST_MATCHER(Expr, hasSideEffects) {
+ return Node.HasSideEffects(Finder->getASTContext());
+}
+
/// Matches the index expression of an array subscript expression.
///
/// Given
Modified: cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp?rev=347462=347461=347462=diff
==
--- cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp (original)
+++ cfe/trunk/lib/ASTMatchers/Dynamic/Registry.cpp Thu Nov 22 06:00:56 2018
@@ -294,6 +294,7 @@ RegistryMaps::RegistryMaps() {
REGISTER_MATCHER(hasReturnValue);
REGISTER_MATCHER(hasRHS);
REGISTER_MATCHER(hasSelector);
+ REGISTER_MATCHER(hasSideEffects);
REGISTER_MATCHER(hasSingleDecl);
REGISTER_MATCHER(hasSize);
REGISTER_MATCHER(hasSizeExpr);
Modified: cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp?rev=347462=347461=347462=diff
==
--- cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp (original)
+++ cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp Thu Nov 22
06:00:56 2018
@@ -2259,5 +2259,21 @@ TEST(Matcher, isMain) {
notMatches("int main2() {}", functionDecl(isMain(;
}
+TEST(Matcher, hasSideEffects) {
+ EXPECT_TRUE(matches("void call();"
+ "void f() { call(); }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(matches("void f(int& a) { a = 0; }", expr(hasSideEffects(;
+ EXPECT_TRUE(
+ matches("void f(volatile int a) { (void)a; }", expr(hasSideEffects(;
+
+ EXPECT_TRUE(notMatches("void call();"
+ "void f() { }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(
+ notMatches("void f(int& a) { (void)a; }", expr(hasSideEffects(;
+ EXPECT_TRUE(notMatches("void f(int a) { (void)a; }",
expr(hasSideEffects(;
+}
+
} // namespace ast_matchers
} // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54830: [ASTMatchers] Add hasSideEffect() matcher.
courbet updated this revision to Diff 175044.
courbet added a comment.
address comment
Repository:
rC Clang
https://reviews.llvm.org/D54830
Files:
docs/LibASTMatchersReference.html
include/clang/ASTMatchers/ASTMatchers.h
lib/ASTMatchers/Dynamic/Registry.cpp
unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
Index: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2259,5 +2259,21 @@
notMatches("int main2() {}", functionDecl(isMain(;
}
+TEST(Matcher, hasSideEffects) {
+ EXPECT_TRUE(matches("void call();"
+ "void f() { call(); }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(matches("void f(int& a) { a = 0; }", expr(hasSideEffects(;
+ EXPECT_TRUE(
+ matches("void f(volatile int a) { (void)a; }", expr(hasSideEffects(;
+
+ EXPECT_TRUE(notMatches("void call();"
+ "void f() { }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(
+ notMatches("void f(int& a) { (void)a; }", expr(hasSideEffects(;
+ EXPECT_TRUE(notMatches("void f(int a) { (void)a; }",
expr(hasSideEffects(;
+}
+
} // namespace ast_matchers
} // namespace clang
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -294,6 +294,7 @@
REGISTER_MATCHER(hasReturnValue);
REGISTER_MATCHER(hasRHS);
REGISTER_MATCHER(hasSelector);
+ REGISTER_MATCHER(hasSideEffects);
REGISTER_MATCHER(hasSingleDecl);
REGISTER_MATCHER(hasSize);
REGISTER_MATCHER(hasSizeExpr);
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -4118,6 +4118,26 @@
InnerMatcher.matches(*DeclarationStatement, Finder, Builder);
}
+/// \brief Matches expressions with potential side effects other than producing
+/// a value, such as a calling a function, throwing an exception, or reading a
+/// volatile variable.
+///
+/// Given
+/// \code
+/// void f(int& a, int b, volatile int c) {
+/// call();
+/// a = 0;
+/// a;
+/// b;
+/// c;
+/// }
+/// \endcode
+/// expr(hasSideEffects())
+/// matches 'call()', 'a = 0', 'c', but not '0', 'a', 'b'.
+AST_MATCHER(Expr, hasSideEffects) {
+ return Node.HasSideEffects(Finder->getASTContext());
+}
+
/// Matches the index expression of an array subscript expression.
///
/// Given
Index: docs/LibASTMatchersReference.html
===
--- docs/LibASTMatchersReference.html
+++ docs/LibASTMatchersReference.html
@@ -2817,6 +2817,24 @@
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprhasSideEffects
+Matches expressions
with potential side effects other than producing
+a value, such as a calling a function, throwing an exception, or reading a
+volatile variable.
+
+Given
+ void f(int a, int b, volatile int c) {
+call();
+a = 0;
+a;
+b;
+c;
+ }
+expr(hasSideEffects())
+ matches 'call()', 'a = 0', 'c', but not '0', 'a', 'b'.
+
+
+
Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprisInstantiationDependent
Matches
expressions that are instantiation-dependent even if it is
neither type- nor value-dependent.
Index: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2259,5 +2259,21 @@
notMatches("int main2() {}", functionDecl(isMain(;
}
+TEST(Matcher, hasSideEffects) {
+ EXPECT_TRUE(matches("void call();"
+ "void f() { call(); }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(matches("void f(int& a) { a = 0; }", expr(hasSideEffects(;
+ EXPECT_TRUE(
+ matches("void f(volatile int a) { (void)a; }", expr(hasSideEffects(;
+
+ EXPECT_TRUE(notMatches("void call();"
+ "void f() { }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(
+ notMatches("void f(int& a) { (void)a; }", expr(hasSideEffects(;
+ EXPECT_TRUE(notMatches("void f(int a) { (void)a; }", expr(hasSideEffects(;
+}
+
} // namespace ast_matchers
} // namespace clang
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -294,6 +294,7 @@
REGISTER_MATCHER(hasReturnValue);
REGISTER_MATCHER(hasRHS);
REGISTER_MATCHER(hasSelector);
+ REGISTER_MATCHER(hasSideEffects);
[PATCH] D54830: [ASTMatchers] Add hasSideEffect() matcher.
courbet marked an inline comment as done. courbet added a comment. Thanks. Repository: rC Clang https://reviews.llvm.org/D54830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54829: [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy
sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clangd/ClangdServer.h:132 /// Remove \p File from list of tracked files, schedule a request to free - /// resources associated with it. + /// resources associated with it. After this function returns, we guarantee + /// that no diagnostics for \p File will be delivered even if there are I'm not sure we actually want to *guarantee* this in the API: this implementation serializes removeDocument and the diagnostics callback, but only because it was the most convenient. I'd suggest just "pending diagnostics for closed files may not be delivered". (and similarly for TUScheduler). Up to you though. Comment at: clangd/TUScheduler.cpp:256 + std::mutex DiagsMu; + /// When set to false, no diagnostics will be reported. Used to ensure we + /// don't report any diagnostics after a shutdown of the worker is requested. I think the second sentence is a bit obvious and doesn't get into the subtlety here. Maybe: ``` Used to prevent remove document + leading to out-of-order diagnostics: The lifetime of the old/new ASTWorkers will overlap, but their handles don't. When the old handle is destroyed, the old worker will stop reporting diagnostics. ``` Comment at: clangd/TUScheduler.cpp:538 + } RequestsCV.notify_all(); } `Done` is covered by `RequestsCV`, and `ReportDiagnostics` is not, it seems a little weird to have the diags critical section in between here. (I don't have an opinion about the relative order of the sections, but notify_all should immediately follow the `Mutex` section for clarity, I think) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54829 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54817: [clangd] Put direct headers into srcs section.
sammccall added a comment.
Going to leave awkward comments suggesting we expand the scope a bit.
Full disclosure: file dependency information is something that's come up as
useful in lots of contexts.
Comment at: clangd/index/Serialization.cpp:469
+ // There's no point in putting headers without digest of the source file.
+ // Because they will only be needed whenever we have an up-to-date index
file.
This seems like unneccesary coupling between layers. On the contrary, I think
it's going to be really useful to have file and dependency information in the
index data and in the in-memory index.
(e.g. for #include completion, more accurately guessing compile commands for
headers, deciding which CC file corresponds to a .h file and vice versa...)
Comment at: clangd/index/Serialization.h:46
+ // URIs of headers directly included in the source file.
+ llvm::Optional> DirectIncludes;
};
This seems a little off to me conceptually: an index contains symbols that
might be from many files. (Also true of Digest, which I missed).
I'd suggest a richer structure which can represent one or many files:
```
std::unordered_map; // keys are URIs
struct SourceFile {
bool IsTU;
StringRef URI; // points back into key
FileDigest Digest;
vector Headers; // points back into keys
// maybe later: compile command
}
```
(though probably wrap the map into a class to ensure the string ownership
doesn't get messed up)
For auto-index shards we could include full SourceFile info for the main file
(with IsTU = true, and headers) and each of the headers would have a skeletal
entry with IsTU = false and no headers stored.
But clangd-indexer could spit out an index that contains full information for
all the transitive include of all indexed TUs.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54817
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54829: [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy
ilya-biryukov added inline comments.
Comment at: clangd/TUScheduler.cpp:424
if (*AST) {
OnUpdated((*AST)->getDiagnostics());
trace::Span Span("Running main AST callback");
sammccall wrote:
> as discussed offline, this doesn't guarantee we're not going to send
> OnUpdated calls for destroyed FileData objects. Intuitively because
> ShuttingDown may be set e.g. while building the AST, more subtly because
> atomic only prevents reordering with respect to other atomic operations.
>
> I believe we need to hold a lock:
> ```
> {
> mutex_lock Lock(SendDiagnosticsMu);
> if (SendDiagnostics)
> OnUpdated(...);
> }
> ```
> and set SendDiagnostics to false under the same lock.
Done. I've made the change before looking at the comment, so using
`ReportDiagnostics` name instead of `SendDiagnostics`. Happy to change if you
like the latter more.
Comment at: clangd/TUScheduler.cpp:426
trace::Span Span("Running main AST callback");
Callbacks.onMainAST(FileName, **AST);
DiagsWereReported = true;
sammccall wrote:
> This patch prevents the AST from being indexed after removal - for the same
> reasons, right?
>
> But we don't prevent onPreambleAST from being called...
> Not sure if it makes sense to fix this, but deserves at least a comment I
> think.
Added a comment.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54829
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54829: [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy
ilya-biryukov updated this revision to Diff 175042.
ilya-biryukov marked 3 inline comments as done.
ilya-biryukov added a comment.
- Added comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54829
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -124,6 +124,8 @@
S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
[&](std::vector Diags) { ++CallbackCount; });
Ready.notify();
+
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
EXPECT_EQ(2, CallbackCount);
}
@@ -147,6 +149,8 @@
std::this_thread::sleep_for(std::chrono::seconds(2));
S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
[&](std::vector Diags) { ++CallbackCount; });
+
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
EXPECT_EQ(2, CallbackCount);
}
@@ -304,6 +308,7 @@
}
}
}
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
} // TUScheduler destructor waits for all operations to finish.
std::lock_guard Lock(Mut);
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -748,7 +748,8 @@
BlockingRequests[RequestIndex]();
}
}
- } // Wait for ClangdServer to shutdown before proceeding.
+ASSERT_TRUE(Server.blockUntilIdleForTest());
+ }
// Check some invariants about the state of the program.
std::vector Stats = DiagConsumer.takeFileStats();
Index: clangd/TUScheduler.h
===
--- clangd/TUScheduler.h
+++ clangd/TUScheduler.h
@@ -105,7 +105,9 @@
llvm::unique_function)> OnUpdated);
/// Remove \p File from the list of tracked files and schedule removal of its
- /// resources.
+ /// resources. After this function returns, we guarantee that no diagnostics
+ /// for \p File will be delivered even if there are pending updates with
+ /// WantDiags::Yes or WantDiags::Auto.
void remove(PathRef File);
/// Schedule an async task with no dependencies.
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -251,6 +251,11 @@
bool Done;/* GUARDED_BY(Mutex) */
std::deque Requests; /* GUARDED_BY(Mutex) */
mutable std::condition_variable RequestsCV;
+ /// Guards a critical section for running the diagnostics callbacks.
+ std::mutex DiagsMu;
+ /// When set to false, no diagnostics will be reported. Used to ensure we
+ /// don't report any diagnostics after a shutdown of the worker is requested.
+ bool ReportDiagnostics = true; /* GUARDED_BY(DiagMu) */
};
/// A smart-pointer-like class that points to an active ASTWorker.
@@ -405,6 +410,14 @@
if (WantDiags == WantDiagnostics::No)
return;
+{
+ std::lock_guard Lock(DiagsMu);
+ // No need to rebuild the AST if we won't send the diagnotics. However,
+ // note that we don't prevent preamble rebuilds.
+ if (!ReportDiagnostics)
+return;
+}
+
// Get the AST for diagnostics.
Optional> AST = IdleASTs.take(this);
if (!AST) {
@@ -417,7 +430,11 @@
// spam us with updates.
// Note *AST can still be null if buildAST fails.
if (*AST) {
- OnUpdated((*AST)->getDiagnostics());
+ {
+std::lock_guard Lock(DiagsMu);
+if (ReportDiagnostics)
+ OnUpdated((*AST)->getDiagnostics());
+ }
trace::Span Span("Running main AST callback");
Callbacks.onMainAST(FileName, **AST);
DiagsWereReported = true;
@@ -514,6 +531,10 @@
assert(!Done && "stop() called twice");
Done = true;
}
+ {
+std::lock_guard Lock(DiagsMu);
+ReportDiagnostics = false;
+ }
RequestsCV.notify_all();
}
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -129,7 +129,9 @@
WantDiagnostics WD = WantDiagnostics::Auto);
/// Remove \p File from list of tracked files, schedule a request to free
- /// resources associated with it.
+ /// resources associated with it. After this function returns, we guarantee
+ /// that no diagnostics for \p File will be delivered even if there are
+ /// pending updates with WantDiags::Yes or WantDiags::Auto.
void removeDocument(PathRef File);
/// Run code completion for \p File at \p Pos.
@@ -226,20 +228,12 @@
formatCode(llvm::StringRef Code, PathRef File,
[PATCH] D54830: [ASTMatchers] Add hasSideEffect() matcher.
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a minor commenting request. Comment at: include/clang/ASTMatchers/ASTMatchers.h:4121 +/// \brief Matches expressions with potential side effects. +/// side effects. -> side effects other than producing a value, such as a calling a function, throwing an exception, or reading a volatile variable. Repository: rC Clang https://reviews.llvm.org/D54830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r347460 - [clangd] Fix use-after-scope in unit test
Author: d0k
Date: Thu Nov 22 04:54:25 2018
New Revision: 347460
URL: http://llvm.org/viewvc/llvm-project?rev=347460=rev
Log:
[clangd] Fix use-after-scope in unit test
The scheduler holds a reference to `Proceed`, so it has to be destroyed
after the scheduler. Found by asan.
Modified:
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Modified: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp?rev=347460=347459=347460=diff
==
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp Thu Nov 22
04:54:25 2018
@@ -221,6 +221,7 @@ TEST_F(TUSchedulerTests, Cancellation) {
//R3 <-- cancelled
std::vector DiagsSeen, ReadsSeen, ReadsCanceled;
{
+Notification Proceed; // Ensure we schedule everything.
TUScheduler S(
getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
/*ASTCallbacks=*/nullptr,
@@ -255,7 +256,6 @@ TEST_F(TUSchedulerTests, Cancellation) {
return std::move(T.second);
};
-Notification Proceed; // Ensure we schedule everything.
S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
[&](std::vector Diags) { Proceed.wait(); });
// The second parens indicate cancellation, where present.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54795: [clang-format] Do not treat asm clobber [ as ObjCExpr, refined
krasimir added a comment. In https://reviews.llvm.org/D54795#1305395, @djasper wrote: > Does this also work for _asm and __asm? Thanks! Turns out, yes. I added tests for those. Repository: rC Clang https://reviews.llvm.org/D54795 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54795: [clang-format] Do not treat asm clobber [ as ObjCExpr, refined
krasimir updated this revision to Diff 175038.
krasimir added a comment.
- Add tests for _asm etc
Repository:
rC Clang
https://reviews.llvm.org/D54795
Files:
lib/Format/TokenAnnotator.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12762,6 +12762,30 @@
" : [d] \"=rm\" (d)\n"
" [e] \"rm\" (*e));\n"
"}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " _asm (\"mov %[e], %[d]\"\n"
+ " : [d] \"=rm\" (d)\n"
+ " [e] \"rm\" (*e));\n"
+ "}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " __asm (\"mov %[e], %[d]\"\n"
+ " : [d] \"=rm\" (d)\n"
+ " [e] \"rm\" (*e));\n"
+ "}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " __asm__ (\"mov %[e],
%[d]\"\n"
+ " : [d] \"=rm\" (d)\n"
+ " [e] \"rm\" (*e));\n"
+ "}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " asm (\"mov %[e], %[d]\"\n"
+ " : [d] \"=rm\" (d),\n"
+ " [e] \"rm\" (*e));\n"
+ "}"));
EXPECT_EQ(FormatStyle::LK_Cpp,
guessLanguage("foo.h", "void f() {\n"
" asm volatile (\"mov %[e], %[d]\"\n"
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -399,14 +399,15 @@
bool IsCpp11AttributeSpecifier = isCpp11AttributeSpecifier(*Left) ||
Contexts.back().InCpp11AttributeSpecifier;
+bool InsideInlineASM = Line.startsWith(tok::kw_asm);
bool StartsObjCMethodExpr =
-!CppArrayTemplates && Style.isCpp() && !IsCpp11AttributeSpecifier &&
-Contexts.back().CanBeExpression && Left->isNot(TT_LambdaLSquare) &&
+!InsideInlineASM && !CppArrayTemplates && Style.isCpp() &&
+!IsCpp11AttributeSpecifier && Contexts.back().CanBeExpression &&
+Left->isNot(TT_LambdaLSquare) &&
!CurrentToken->isOneOf(tok::l_brace, tok::r_square) &&
(!Parent ||
- (Parent->is(tok::colon) && Parent->isNot(TT_InlineASMColon)) ||
- Parent->isOneOf(tok::l_square, tok::l_paren, tok::kw_return,
- tok::kw_throw) ||
+ Parent->isOneOf(tok::colon, tok::l_square, tok::l_paren,
+ tok::kw_return, tok::kw_throw) ||
Parent->isUnaryOperator() ||
// FIXME(bug 36976): ObjC return types shouldn't use TT_CastRParen.
Parent->isOneOf(TT_ObjCForIn, TT_CastRParen) ||
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -12762,6 +12762,30 @@
" : [d] \"=rm\" (d)\n"
" [e] \"rm\" (*e));\n"
"}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " _asm (\"mov %[e], %[d]\"\n"
+ " : [d] \"=rm\" (d)\n"
+ " [e] \"rm\" (*e));\n"
+ "}"));
+ EXPECT_EQ(FormatStyle::LK_Cpp, guessLanguage("foo.h",
+ "void f() {\n"
+ " __asm (\"mov %[e], %[d]\"\n"
+ " : [d] \"=rm\" (d)\n"
+ " [e] \"rm\" (*e));\n"
+
[PATCH] D54829: [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy
ilya-biryukov updated this revision to Diff 175037.
ilya-biryukov added a comment.
- Ensure consistency for diagnostic callbacks with a critical section.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54829
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/TUScheduler.cpp
unittests/clangd/ClangdTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -124,6 +124,8 @@
S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
[&](std::vector Diags) { ++CallbackCount; });
Ready.notify();
+
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
EXPECT_EQ(2, CallbackCount);
}
@@ -147,6 +149,8 @@
std::this_thread::sleep_for(std::chrono::seconds(2));
S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
[&](std::vector Diags) { ++CallbackCount; });
+
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
EXPECT_EQ(2, CallbackCount);
}
@@ -304,6 +308,7 @@
}
}
}
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
} // TUScheduler destructor waits for all operations to finish.
std::lock_guard Lock(Mut);
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -748,7 +748,8 @@
BlockingRequests[RequestIndex]();
}
}
- } // Wait for ClangdServer to shutdown before proceeding.
+ASSERT_TRUE(Server.blockUntilIdleForTest());
+ }
// Check some invariants about the state of the program.
std::vector Stats = DiagConsumer.takeFileStats();
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -251,6 +251,11 @@
bool Done;/* GUARDED_BY(Mutex) */
std::deque Requests; /* GUARDED_BY(Mutex) */
mutable std::condition_variable RequestsCV;
+ /// Guards a critical section for running the diagnostics callbacks.
+ std::mutex DiagsMu;
+ /// When set to false, no diagnostics will be reported. Used to ensure we
+ /// don't report any diagnostics after a shutdown of the worker is requested.
+ bool ReportDiagnostics = true; /* GUARDED_BY(DiagMu) */
};
/// A smart-pointer-like class that points to an active ASTWorker.
@@ -401,10 +406,17 @@
}
}
-// We only need to build the AST if diagnostics were requested.
+// We only need to build the AST if diagnostics were requested and the
+// callers are still interested in this ASTWorker (it's not shutting down).
if (WantDiags == WantDiagnostics::No)
return;
+{
+ std::lock_guard Lock(DiagsMu);
+ if (!ReportDiagnostics)
+return;
+}
+
// Get the AST for diagnostics.
Optional> AST = IdleASTs.take(this);
if (!AST) {
@@ -417,7 +429,11 @@
// spam us with updates.
// Note *AST can still be null if buildAST fails.
if (*AST) {
- OnUpdated((*AST)->getDiagnostics());
+ {
+std::lock_guard Lock(DiagsMu);
+if (ReportDiagnostics)
+ OnUpdated((*AST)->getDiagnostics());
+ }
trace::Span Span("Running main AST callback");
Callbacks.onMainAST(FileName, **AST);
DiagsWereReported = true;
@@ -514,6 +530,10 @@
assert(!Done && "stop() called twice");
Done = true;
}
+ {
+std::lock_guard Lock(DiagsMu);
+ReportDiagnostics = false;
+ }
RequestsCV.notify_all();
}
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -226,20 +226,12 @@
formatCode(llvm::StringRef Code, PathRef File,
ArrayRef Ranges);
- typedef uint64_t DocVersion;
-
- void consumeDiagnostics(PathRef File, DocVersion Version,
- std::vector Diags);
-
tooling::CompileCommand getCompileCommand(PathRef File);
const GlobalCompilationDatabase
DiagnosticsConsumer
const FileSystemProvider
- /// Used to synchronize diagnostic responses for added and removed files.
- llvm::StringMap InternalVersion;
-
Path ResourceDir;
// The index used to look up symbols. This could be:
// - null (all index functionality is optional)
@@ -259,12 +251,6 @@
llvm::Optional WorkspaceRoot;
std::shared_ptr PCHs;
- /// Used to serialize diagnostic callbacks.
- /// FIXME(ibiryukov): get rid of an extra map and put all version counters
- /// into CppFile.
- std::mutex DiagnosticsMutex;
- /// Maps from a filename to the latest version of reported diagnostics.
- llvm::StringMap ReportedDiagnosticVersions;
// WorkScheduler has to be the last member, because its destructor has
[PATCH] D53488: [clang-tidy] Improving narrowing conversions
gchatelet updated this revision to Diff 175033.
gchatelet marked an inline comment as done.
gchatelet added a comment.
- Reverting the WarnOnFloatingPointNarrowingConversion option to be on by
default
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53488
Files:
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
test/clang-tidy/cppcoreguidelines-narrowing-conversions-extras.cpp
test/clang-tidy/cppcoreguidelines-narrowing-conversions-long-is-32bits.cpp
test/clang-tidy/cppcoreguidelines-narrowing-conversions-unsigned-char.cpp
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -config="{CheckOptions: [{key: "cppcoreguidelines-narrowing-conversions.WarnOnFloatingPointNarrowingConversion", value: 0}]}" -- -target x86_64-unknown-linux -fsigned-char
float ceil(float);
namespace std {
@@ -9,47 +9,284 @@
namespace floats {
struct ConvertsToFloat {
- operator float() const { return 0.5; }
+ operator float() const { return 0.5f; }
};
-float operator "" _Pa(unsigned long long);
+float operator"" _float(unsigned long long);
-void not_ok(double d) {
+void narrow_fp_to_int_not_ok(double d) {
int i = 0;
i = d;
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
i = 0.5f;
- // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
i = static_cast(d);
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
i = ConvertsToFloat();
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
- i = 15_Pa;
+ i = 15_float;
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
-}
-
-void not_ok_binary_ops(double d) {
- int i = 0;
+ i += d;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
i += 0.5;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
i += 0.5f;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
- i += d;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
- // We warn on the following even though it's not dangerous because there is no
- // reason to use a double literal here.
- // TODO(courbet): Provide an automatic fix.
- i += 2.0;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
- i += 2.0f;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
-
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
i *= 0.5f;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
i /= 0.5f;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
i += (double)0.5f;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i += 2.0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from
[PATCH] D54829: [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy
sammccall added inline comments.
Comment at: clangd/TUScheduler.cpp:424
if (*AST) {
OnUpdated((*AST)->getDiagnostics());
trace::Span Span("Running main AST callback");
as discussed offline, this doesn't guarantee we're not going to send OnUpdated
calls for destroyed FileData objects. Intuitively because ShuttingDown may be
set e.g. while building the AST, more subtly because atomic only prevents
reordering with respect to other atomic operations.
I believe we need to hold a lock:
```
{
mutex_lock Lock(SendDiagnosticsMu);
if (SendDiagnostics)
OnUpdated(...);
}
```
and set SendDiagnostics to false under the same lock.
Comment at: clangd/TUScheduler.cpp:426
trace::Span Span("Running main AST callback");
Callbacks.onMainAST(FileName, **AST);
DiagsWereReported = true;
This patch prevents the AST from being indexed after removal - for the same
reasons, right?
But we don't prevent onPreambleAST from being called...
Not sure if it makes sense to fix this, but deserves at least a comment I think.
Comment at: clangd/TUScheduler.cpp:515
void ASTWorker::stop() {
+ ShuttingDown = true;
I think we need to add public docs to TUScheduler::remove and
ClangdServer::removeDocument indicating that diagnostics may not be delivered
after documents are removed (regardless of WantDiagnostics).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54829
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54781: [clangd] Add 'Switch header/source' command in clangd-vscode
hokein added inline comments.
Comment at: clangd/clients/clangd-vscode/src/extension.ts:17
+export namespace SwitchSourceHeaderRequest {
+export const type = new RequestType('textDocument/switchSourceHeader');
+}
hokein wrote:
> Is `textDocument/switchSourceHeader` a built-in support in vscode? Is it
> documented somewhere? I couldn't find any official document at
> https://code.visualstudio.com. Am I missing anything here?
>
nvm, just realize that this is an LSP extension implemented inside clangd.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54781
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54817: [clangd] Put direct headers into srcs section.
kadircet updated this revision to Diff 175032.
kadircet added a comment.
- Use include depth to get includes for all files, rather than just main file.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54817
Files:
clangd/index/Background.cpp
clangd/index/Background.h
clangd/index/Serialization.cpp
clangd/index/Serialization.h
unittests/clangd/BackgroundIndexTests.cpp
unittests/clangd/SerializationTests.cpp
Index: unittests/clangd/SerializationTests.cpp
===
--- unittests/clangd/SerializationTests.cpp
+++ unittests/clangd/SerializationTests.cpp
@@ -173,7 +173,7 @@
UnorderedElementsAreArray(YAMLFromRefs(*In->Refs)));
}
-TEST(SerializationTest, HashTest) {
+TEST(SerializationTest, SrcsTest) {
auto In = readIndexFile(YAML);
EXPECT_TRUE(bool(In)) << In.takeError();
@@ -185,19 +185,39 @@
IndexFileOut Out(*In);
Out.Format = IndexFileFormat::RIFF;
Out.Digest =
- std::string Serialized = to_string(Out);
-
- auto In2 = readIndexFile(Serialized);
- ASSERT_TRUE(bool(In2)) << In.takeError();
- ASSERT_EQ(In2->Digest, Digest);
- ASSERT_TRUE(In2->Symbols);
- ASSERT_TRUE(In2->Refs);
-
- // Assert the YAML serializations match, for nice comparisons and diffs.
- EXPECT_THAT(YAMLFromSymbols(*In2->Symbols),
- UnorderedElementsAreArray(YAMLFromSymbols(*In->Symbols)));
- EXPECT_THAT(YAMLFromRefs(*In2->Refs),
- UnorderedElementsAreArray(YAMLFromRefs(*In->Refs)));
+ {
+std::string Serialized = to_string(Out);
+
+auto In = readIndexFile(Serialized);
+ASSERT_TRUE(bool(In)) << In.takeError();
+ASSERT_EQ(*In->Digest, Digest);
+ASSERT_TRUE(In->Symbols);
+ASSERT_TRUE(In->Refs);
+// Assert the YAML serializations match, for nice comparisons and diffs.
+EXPECT_THAT(YAMLFromSymbols(*In->Symbols),
+UnorderedElementsAreArray(YAMLFromSymbols(*In->Symbols)));
+EXPECT_THAT(YAMLFromRefs(*In->Refs),
+UnorderedElementsAreArray(YAMLFromRefs(*In->Refs)));
+ }
+
+ std::vector DirectIncludes = {"inc1", "inc2"};
+ Out.DirectIncludes =
+ {
+std::string Serialized = to_string(Out);
+
+auto In = readIndexFile(Serialized);
+ASSERT_TRUE(bool(In)) << In.takeError();
+ASSERT_EQ(*In->Digest, Digest);
+ASSERT_TRUE(In->Symbols);
+ASSERT_TRUE(In->Refs);
+EXPECT_THAT(*In->DirectIncludes, UnorderedElementsAreArray(DirectIncludes));
+
+// Assert the YAML serializations match, for nice comparisons and diffs.
+EXPECT_THAT(YAMLFromSymbols(*In->Symbols),
+UnorderedElementsAreArray(YAMLFromSymbols(*In->Symbols)));
+EXPECT_THAT(YAMLFromRefs(*In->Refs),
+UnorderedElementsAreArray(YAMLFromRefs(*In->Refs)));
+ }
}
} // namespace
Index: unittests/clangd/BackgroundIndexTests.cpp
===
--- unittests/clangd/BackgroundIndexTests.cpp
+++ unittests/clangd/BackgroundIndexTests.cpp
@@ -161,5 +161,41 @@
EXPECT_EQ(*ShardSource->Digest, Digest);
}
+TEST(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")};
+ {
+BackgroundIndex Idx(Context::empty(), "", FS, /*URISchemes=*/{"unittest"},
+[&](llvm::StringRef) { return });
+Idx.enqueue(testPath("root"), Cmd);
+Idx.blockUntilIdleForTest();
+ }
+
+ auto ShardSource = MSS.loadShard(testPath("root/A.cc"));
+ EXPECT_THAT(*ShardSource->DirectIncludes,
+ UnorderedElementsAre("unittest:///root/A.h"));
+
+ auto ShardHeader = MSS.loadShard(testPath("root/A.h"));
+ EXPECT_THAT(*ShardHeader->DirectIncludes,
+ UnorderedElementsAre("unittest:///root/B.h"));
+}
+
} // namespace clangd
} // namespace clang
Index: clangd/index/Serialization.h
===
--- clangd/index/Serialization.h
+++ clangd/index/Serialization.h
@@ -42,6 +42,8 @@
llvm::Optional Refs;
// Digest of the source file that generated the contents.
llvm::Optional Digest;
+ // URIs of headers directly included in the source file.
+ llvm::Optional> DirectIncludes;
};
// Parse an index file. The input must be a RIFF or YAML file.
llvm::Expected readIndexFile(llvm::StringRef);
@@ -52,6 +54,8 @@
const RefSlab *Refs = nullptr;
// Digest of the source file that generated the contents.
const
[PATCH] D53488: [clang-tidy] Improving narrowing conversions
gchatelet marked 2 inline comments as done. gchatelet added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:42-44 i += 2.0; - // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions] + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions] i += 2.0f; aaron.ballman wrote: > I don't think these should diagnose. They're both harmless as the literal > values are exactly representable in the destination type. Yes indeed they are harmless but why not write the correct literal in the first place? I'm keeping these warnings for now. Let me know if you feel strongly about it. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54830: [ASTMatchers] Add hasSideEffect() matcher.
courbet updated this revision to Diff 175031.
courbet added a comment.
rebase
Repository:
rC Clang
https://reviews.llvm.org/D54830
Files:
docs/LibASTMatchersReference.html
include/clang/ASTMatchers/ASTMatchers.h
lib/ASTMatchers/Dynamic/Registry.cpp
unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
Index: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2259,5 +2259,21 @@
notMatches("int main2() {}", functionDecl(isMain(;
}
+TEST(Matcher, hasSideEffects) {
+ EXPECT_TRUE(matches("void call();"
+ "void f() { call(); }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(matches("void f(int& a) { a = 0; }", expr(hasSideEffects(;
+ EXPECT_TRUE(
+ matches("void f(volatile int a) { (void)a; }", expr(hasSideEffects(;
+
+ EXPECT_TRUE(notMatches("void call();"
+ "void f() { }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(
+ notMatches("void f(int& a) { (void)a; }", expr(hasSideEffects(;
+ EXPECT_TRUE(notMatches("void f(int a) { (void)a; }",
expr(hasSideEffects(;
+}
+
} // namespace ast_matchers
} // namespace clang
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -294,6 +294,7 @@
REGISTER_MATCHER(hasReturnValue);
REGISTER_MATCHER(hasRHS);
REGISTER_MATCHER(hasSelector);
+ REGISTER_MATCHER(hasSideEffects);
REGISTER_MATCHER(hasSingleDecl);
REGISTER_MATCHER(hasSize);
REGISTER_MATCHER(hasSizeExpr);
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -4118,6 +4118,24 @@
InnerMatcher.matches(*DeclarationStatement, Finder, Builder);
}
+/// \brief Matches expressions with potential side effects.
+///
+/// Given
+/// \code
+/// void f(int& a, int b, volatile int c) {
+/// call();
+/// a = 0;
+/// a;
+/// b;
+/// c;
+/// }
+/// \endcode
+/// expr(hasSideEffects())
+/// matches 'call()', 'a = 0', 'c', but not '0', 'a', 'b'.
+AST_MATCHER(Expr, hasSideEffects) {
+ return Node.HasSideEffects(Finder->getASTContext());
+}
+
/// Matches the index expression of an array subscript expression.
///
/// Given
Index: docs/LibASTMatchersReference.html
===
--- docs/LibASTMatchersReference.html
+++ docs/LibASTMatchersReference.html
@@ -2817,6 +2817,22 @@
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprhasSideEffects
+Matches expressions
with potential side effects.
+
+Given
+ void f(int a, int b, volatile int c) {
+call();
+a = 0;
+a;
+b;
+c;
+ }
+expr(hasSideEffects())
+ matches 'call()', 'a = 0', 'c', but not '0', 'a', 'b'.
+
+
+
Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprisInstantiationDependent
Matches
expressions that are instantiation-dependent even if it is
neither type- nor value-dependent.
Index: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2259,5 +2259,21 @@
notMatches("int main2() {}", functionDecl(isMain(;
}
+TEST(Matcher, hasSideEffects) {
+ EXPECT_TRUE(matches("void call();"
+ "void f() { call(); }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(matches("void f(int& a) { a = 0; }", expr(hasSideEffects(;
+ EXPECT_TRUE(
+ matches("void f(volatile int a) { (void)a; }", expr(hasSideEffects(;
+
+ EXPECT_TRUE(notMatches("void call();"
+ "void f() { }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(
+ notMatches("void f(int& a) { (void)a; }", expr(hasSideEffects(;
+ EXPECT_TRUE(notMatches("void f(int a) { (void)a; }", expr(hasSideEffects(;
+}
+
} // namespace ast_matchers
} // namespace clang
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -294,6 +294,7 @@
REGISTER_MATCHER(hasReturnValue);
REGISTER_MATCHER(hasRHS);
REGISTER_MATCHER(hasSelector);
+ REGISTER_MATCHER(hasSideEffects);
REGISTER_MATCHER(hasSingleDecl);
REGISTER_MATCHER(hasSize);
REGISTER_MATCHER(hasSizeExpr);
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++
[PATCH] D53488: [clang-tidy] Improving narrowing conversions
gchatelet updated this revision to Diff 175030.
gchatelet marked an inline comment as done.
gchatelet added a comment.
- Refactored the code to make it simpler, added more tests
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53488
Files:
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
test/clang-tidy/cppcoreguidelines-narrowing-conversions-extras.cpp
test/clang-tidy/cppcoreguidelines-narrowing-conversions-long-is-32bits.cpp
test/clang-tidy/cppcoreguidelines-narrowing-conversions-unsigned-char.cpp
test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t -- -- -target x86_64-unknown-linux -fsigned-char
float ceil(float);
namespace std {
@@ -9,47 +9,284 @@
namespace floats {
struct ConvertsToFloat {
- operator float() const { return 0.5; }
+ operator float() const { return 0.5f; }
};
-float operator "" _Pa(unsigned long long);
+float operator"" _float(unsigned long long);
-void not_ok(double d) {
+void narrow_fp_to_int_not_ok(double d) {
int i = 0;
i = d;
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
i = 0.5f;
- // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
i = static_cast(d);
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
i = ConvertsToFloat();
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
- i = 15_Pa;
+ i = 15_float;
// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
-}
-
-void not_ok_binary_ops(double d) {
- int i = 0;
+ i += d;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
i += 0.5;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
i += 0.5f;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
- i += d;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
- // We warn on the following even though it's not dangerous because there is no
- // reason to use a double literal here.
- // TODO(courbet): Provide an automatic fix.
- i += 2.0;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
- i += 2.0f;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
-
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
i *= 0.5f;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
i /= 0.5f;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
i += (double)0.5f;
- // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i += 2.0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion from constant 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+ i += 2.0f;
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: narrowing conversion
r347453 - [ASTMatchers] Re-generate ast matchers doc after rL346455.
Author: courbet
Date: Thu Nov 22 02:44:36 2018
New Revision: 347453
URL: http://llvm.org/viewvc/llvm-project?rev=347453=rev
Log:
[ASTMatchers] Re-generate ast matchers doc after rL346455.
Modified:
cfe/trunk/docs/LibASTMatchersReference.html
Modified: cfe/trunk/docs/LibASTMatchersReference.html
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/LibASTMatchersReference.html?rev=347453=347452=347453=diff
==
--- cfe/trunk/docs/LibASTMatchersReference.html (original)
+++ cfe/trunk/docs/LibASTMatchersReference.html Thu Nov 22 02:44:36 2018
@@ -795,6 +795,17 @@ Example matches a ? b : c
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>StmtconstantExprMatcherhttps://clang.llvm.org/doxygen/classclang_1_1ConstantExpr.html;>ConstantExpr...
+Matches a constant
expression wrapper.
+
+Example matches the constant in the case statement:
+(matcher = constantExpr())
+ switch (a) {
+ case 37: break;
+ }
+
+
+
Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Stmt.html;>StmtcontinueStmtMatcherhttps://clang.llvm.org/doxygen/classclang_1_1ContinueStmt.html;>ContinueStmt...
Matches continue
statements.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54830: [ASTMatchers] Add hasSideEffect() matcher.
courbet created this revision.
courbet added a reviewer: aaron.ballman.
Herald added a subscriber: cfe-commits.
Exposes Expr::HasSideEffects.
Repository:
rC Clang
https://reviews.llvm.org/D54830
Files:
docs/LibASTMatchersReference.html
include/clang/ASTMatchers/ASTMatchers.h
lib/ASTMatchers/Dynamic/Registry.cpp
unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
Index: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2259,5 +2259,21 @@
notMatches("int main2() {}", functionDecl(isMain(;
}
+TEST(Matcher, hasSideEffects) {
+ EXPECT_TRUE(matches("void call();"
+ "void f() { call(); }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(matches("void f(int& a) { a = 0; }", expr(hasSideEffects(;
+ EXPECT_TRUE(
+ matches("void f(volatile int a) { (void)a; }", expr(hasSideEffects(;
+
+ EXPECT_TRUE(notMatches("void call();"
+ "void f() { }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(
+ notMatches("void f(int& a) { (void)a; }", expr(hasSideEffects(;
+ EXPECT_TRUE(notMatches("void f(int a) { (void)a; }",
expr(hasSideEffects(;
+}
+
} // namespace ast_matchers
} // namespace clang
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -294,6 +294,7 @@
REGISTER_MATCHER(hasReturnValue);
REGISTER_MATCHER(hasRHS);
REGISTER_MATCHER(hasSelector);
+ REGISTER_MATCHER(hasSideEffects);
REGISTER_MATCHER(hasSingleDecl);
REGISTER_MATCHER(hasSize);
REGISTER_MATCHER(hasSizeExpr);
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -4118,6 +4118,24 @@
InnerMatcher.matches(*DeclarationStatement, Finder, Builder);
}
+/// \brief Matches expressions with potential side effects.
+///
+/// Given
+/// \code
+/// void f(int& a, int b, volatile int c) {
+/// call();
+/// a = 0;
+/// a;
+/// b;
+/// c;
+/// }
+/// \endcode
+/// expr(hasSideEffects())
+/// matches 'call()', 'a = 0', 'c', but not '0', 'a', 'b'.
+AST_MATCHER(Expr, hasSideEffects) {
+ return Node.HasSideEffects(Finder->getASTContext());
+}
+
/// Matches the index expression of an array subscript expression.
///
/// Given
Index: docs/LibASTMatchersReference.html
===
--- docs/LibASTMatchersReference.html
+++ docs/LibASTMatchersReference.html
@@ -2806,6 +2806,22 @@
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprhasSideEffects
+Matches expressions
with potential side effects.
+
+Given
+ void f(int a, int b, volatile int c) {
+call();
+a = 0;
+a;
+b;
+c;
+ }
+expr(hasSideEffects())
+ matches 'call()', 'a = 0', 'c', but not '0', 'a', 'b'.
+
+
+
Matcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>ExprisInstantiationDependent
Matches
expressions that are instantiation-dependent even if it is
neither type- nor value-dependent.
Index: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -2259,5 +2259,21 @@
notMatches("int main2() {}", functionDecl(isMain(;
}
+TEST(Matcher, hasSideEffects) {
+ EXPECT_TRUE(matches("void call();"
+ "void f() { call(); }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(matches("void f(int& a) { a = 0; }", expr(hasSideEffects(;
+ EXPECT_TRUE(
+ matches("void f(volatile int a) { (void)a; }", expr(hasSideEffects(;
+
+ EXPECT_TRUE(notMatches("void call();"
+ "void f() { }",
+ expr(hasSideEffects(;
+ EXPECT_TRUE(
+ notMatches("void f(int& a) { (void)a; }", expr(hasSideEffects(;
+ EXPECT_TRUE(notMatches("void f(int a) { (void)a; }", expr(hasSideEffects(;
+}
+
} // namespace ast_matchers
} // namespace clang
Index: lib/ASTMatchers/Dynamic/Registry.cpp
===
--- lib/ASTMatchers/Dynamic/Registry.cpp
+++ lib/ASTMatchers/Dynamic/Registry.cpp
@@ -294,6 +294,7 @@
REGISTER_MATCHER(hasReturnValue);
REGISTER_MATCHER(hasRHS);
REGISTER_MATCHER(hasSelector);
+ REGISTER_MATCHER(hasSideEffects);
REGISTER_MATCHER(hasSingleDecl);
REGISTER_MATCHER(hasSize);
REGISTER_MATCHER(hasSizeExpr);
Index: include/clang/ASTMatchers/ASTMatchers.h
[PATCH] D54829: [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy
ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:241 ParseInputs FileInputs; + /// Used to indicate to the tasks running inside the worker queue that AST is + /// being destroyed and the AST queue is being drained. Note to myself: should reuse `Done` instead by making turning it into `atomic` Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54829 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54800: [clangd] Cleanup: stop passing around list of supported URI schemes.
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Awesome!
Comment at: clangd/URI.cpp:202
+("Not a valid absolute path: " + AbsolutePath).str().c_str());
+ for (auto I = URISchemeRegistry::begin(), E = URISchemeRegistry::end();
+ I != E; ++I) {
for (auto& Entry : URISchemeRegistry::entries())
Comment at: clangd/tool/ClangdMain.cpp:93
+
+static cl::opt EnablTestScheme(
+"enable-test-scheme",
EnableTestScheme
should the flag include "uri"?
Comment at: test/clangd/did-change-configuration-params.test:6
---
-{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabaseChanges":{"/clangd-test/foo.c":
{"workingDirectory":"/clangd-test", "compilationCommand": ["clang", "-c",
"foo.c"]}
+{"jsonrpc":"2.0","method":"workspace/didChangeConfiguration","params":{"settings":{"compilationDatabaseChanges":{"/clangd-test/foo.c":
{"workingDirectory":"/clangd-test", "compilationCommand": ["clang", "-c",
"/clangd-test/foo.c"]}
---
what's up with this change? :-)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54800
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54760: [clangd] Cleanup: make the diags callback global in TUScheduler
ilya-biryukov planned changes to this revision. ilya-biryukov added a comment. Would like to land https://reviews.llvm.org/D54829 first and rebase on top of it. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54760 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54760: [clangd] Cleanup: make the diags callback global in TUScheduler
ilya-biryukov updated this revision to Diff 175028.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
- Remove accidentally added redundant 'private:' section
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54760
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
@@ -10,6 +10,7 @@
#include "Context.h"
#include "TUScheduler.h"
#include "TestFS.h"
+#include "llvm/ADT/ScopeExit.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include
@@ -28,7 +29,6 @@
using ::testing::Pointee;
using ::testing::UnorderedElementsAre;
-void ignoreUpdate(Optional>) {}
void ignoreError(Error Err) {
handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});
}
@@ -40,11 +40,62 @@
buildTestFS(Files, Timestamps), std::move(Contents)};
}
+void updateWithCallback(TUScheduler , PathRef File, StringRef Contents,
+WantDiagnostics WD, llvm::unique_function CB) {
+ WithContextValue Ctx(llvm::make_scope_exit(std::move(CB)));
+ S.update(File, getInputs(File, Contents), WD);
+}
+
+static Key)>>
+DiagsCallbackKey;
+
+/// A diagnostics callback that should be passed to TUScheduler when it's used
+/// in updateWithDiags.
+static std::unique_ptr captureDiags() {
+ class CaptureDiags : public ParsingCallbacks {
+void onDiagnostics(PathRef File, std::vector Diags) override {
+ auto D = Context::current().get(DiagsCallbackKey);
+ if (!D)
+return;
+ const_cast)> &> (
+ *D)(File, Diags);
+}
+ };
+ return llvm::make_unique();
+}
+
+/// Schedule an update and call \p CB with the diagnostics it produces, if any.
+/// The TUScheduler should be created with captureDiags as a DiagsCallback for
+/// this to work.
+void updateWithDiags(TUScheduler , PathRef File, ParseInputs Inputs,
+ WantDiagnostics WD,
+ llvm::unique_function)> CB) {
+ Path OrigFile = File.str();
+ WithContextValue Ctx(
+ DiagsCallbackKey,
+ Bind(
+ [OrigFile](decltype(CB) CB, PathRef File, std::vector Diags) {
+assert(File == OrigFile);
+CB(std::move(Diags));
+ },
+ std::move(CB)));
+ S.update(File, std::move(Inputs), WD);
+}
+
+ void updateWithDiags(TUScheduler , PathRef File, llvm::StringRef Contents,
+ WantDiagnostics WD,
+ llvm::unique_function)> CB) {
+return updateWithDiags(S, File, getInputs(File, Contents), WD, std::move(CB));
+ }
+
StringMap Files;
StringMap Timestamps;
MockCompilationDatabase CDB;
};
+Key)>>
+TUSchedulerTests::DiagsCallbackKey;
+
TEST_F(TUSchedulerTests, MissingFiles) {
TUScheduler S(getDefaultAsyncThreadsCount(),
/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
@@ -57,7 +108,7 @@
auto Missing = testPath("missing.cpp");
Files[Missing] = "";
- S.update(Added, getInputs(Added, ""), WantDiagnostics::No, ignoreUpdate);
+ S.update(Added, getInputs(Added, ""), WantDiagnostics::No);
// Assert each operation for missing file is an error (even if it's available
// in VFS).
@@ -104,25 +155,25 @@
Notification Ready;
TUScheduler S(
getDefaultAsyncThreadsCount(),
-/*StorePreamblesInMemory=*/true, /*ASTCallbacks=*/nullptr,
+/*StorePreamblesInMemory=*/true, captureDiags(),
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
ASTRetentionPolicy());
auto Path = testPath("foo.cpp");
-S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
- [&](std::vector) { Ready.wait(); });
-
-S.update(Path, getInputs(Path, "request diags"), WantDiagnostics::Yes,
- [&](std::vector Diags) { ++CallbackCount; });
-S.update(Path, getInputs(Path, "auto (clobbered)"), WantDiagnostics::Auto,
- [&](std::vector Diags) {
- ADD_FAILURE() << "auto should have been cancelled by auto";
- });
-S.update(Path, getInputs(Path, "request no diags"), WantDiagnostics::No,
- [&](std::vector Diags) {
- ADD_FAILURE() << "no diags should not be called back";
- });
-S.update(Path, getInputs(Path, "auto (produces)"), WantDiagnostics::Auto,
- [&](std::vector Diags) { ++CallbackCount; });
+updateWithDiags(S, Path, "", WantDiagnostics::Yes,
+[&](std::vector) { Ready.wait(); });
+updateWithDiags(S, Path, "request diags", WantDiagnostics::Yes,
+[&](std::vector) { ++CallbackCount; });
+updateWithDiags(S, Path, "auto (clobbered)",
+WantDiagnostics::Auto, [&](std::vector) {
+
[PATCH] D54760: [clangd] Cleanup: make the diags callback global in TUScheduler
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:77
+FileIndex *FIndex,
+llvm::unique_function)> OnDiags) {
+ using DiagsCallback = decltype(OnDiags);
sammccall wrote:
> hmm, the double-indirection for diags seems a bit funny, especially since we
> have a dedicated method on ClangdServer so the lambda is trivial.
>
> Maybe we should just make the CB class nested or friend in ClangdServer, and
> pass it a ClangdServer*? Then it can access DynamicIdx and consumeDiagnostics
> directly. The decoupling here isn't that strong to start with.
I would avoid doing that, because ClangdServer is not thread-safe and giving
the async callbacks that run on a different threads an access to the full class
is scary.
Giving a narrower interface seems better, even if that means double-indirection
After D54829, there's no need for double-indirection too, the callback is
simply directly calling into the diagnostics consumer, so it shouldn't be an
issue.
Comment at: clangd/ClangdServer.h:232
+ /// addDocument. Used to avoid races when sending diagnostics to the clients.
+ static Key DocVersionKey;
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > I'm not sure using context here buys much: there aren't many layers,
> > > they're fairly coupled, and this information would look pretty natural in
> > > the interfaces.
> > >
> > > What about:
> > > - move the definition of DocVersion to TUScheduler
> > > - make DocVersion a member of ParseInputs
> > > - pass (PathRef, DocVersion, vector) to the TUScheduler's diag
> > > callback
> > I'd keep it separate: `ParseInputs` is defined in `ClangdUnit.cpp` and it
> > serves the purpose of defining everything we need to run the compiler.
> > Adding `DocVersion` there would make no sense: it's of no use to the actual
> > code doing preamble builds or parsing.
> >
> > I would rather aim to get rid of `DocVersion` (for this particular purpose,
> > there are other ways to fix the raciness that the DocVersions workaround).
> > WDYT?
> Yeah, separating it from ParseInputs makes sense conceptually, and getting
> rid of it is indeed better.
>
> I don't think this justifies using Context.
>
> Passing it as a separate parameter seems fine to me. Putting it in
> ParseInputs with a FIXME is a little less principled but avoids interface
> churn.
The versions are not too hard to remove, here's the change that does this:
D54829
That would avoid the need for putting them into either the context or the
`ParseInputs`.
Comment at: clangd/TUScheduler.cpp:156
class ASTWorker {
+private:
friend class ASTWorkerHandle;
sammccall wrote:
> nit: we generally have the private section at the top without explicitly
> introducing it (as common in LLVM), or at the bottom. I'm fine with either
> style but would prefer not to add a third.
Yeah, sorry, that was an accidental change.
Comment at: clangd/TUScheduler.h:92
+ ASTRetentionPolicy RetentionPolicy,
+ DiagsCallback OnDiags = nullptr);
~TUScheduler();
sammccall wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > ISTM the callback would fit nicely on the ASTCallbacks?
> > > It even gets plumbed through to ASTWorker correctly by reference.
> > Done. This is definitely less plumbing, but IMO the public interface looks
> > a bit worse now.
> > Consuming ASTs and diagnostics are two independent concerns and it's a bit
> > awkward to combine them in the same struct.
> >
> > But we don't have too many callers, updating those is easy.
> (Separate yes, but I'm not sure they're that much more separate than
> consuming preambles vs ASTs, really. Outside of the 2/1 split of where the
> data goes today...)
> but I'm not sure they're that much more separate than consuming preambles vs
> ASTs
ASTs and resulting diagnostics are on different layers of abstraction, so I
don't think they fit well in the same interface, even if the data-flow for
those is similar.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54760
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54829: [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy
ilya-biryukov created this revision.
ilya-biryukov added a reviewer: sammccall.
Herald added subscribers: kadircet, jfb, arphaman, jkorous, MaskRay, ioeric,
javed.absar.
Previously, removeDoc followed by an addDoc to TUScheduler resulted in
racy diagnostic responses, i.e. the old dianostics could be delivered
to the client after the new ones by TUScheduler.
To workaround this, we tracked a version number in ClangdServer and
discarded stale diagnostics. After this commit, the TUScheduler will
stop delivering diagnostics for removed files and the workaround in
ClangdServer is not required anymore.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54829
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/TUScheduler.cpp
unittests/clangd/ClangdTests.cpp
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -147,6 +147,8 @@
std::this_thread::sleep_for(std::chrono::seconds(2));
S.update(Path, getInputs(Path, "auto (shut down)"), WantDiagnostics::Auto,
[&](std::vector Diags) { ++CallbackCount; });
+
+ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
}
EXPECT_EQ(2, CallbackCount);
}
Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -748,7 +748,8 @@
BlockingRequests[RequestIndex]();
}
}
- } // Wait for ClangdServer to shutdown before proceeding.
+ASSERT_TRUE(Server.blockUntilIdleForTest());
+ }
// Check some invariants about the state of the program.
std::vector Stats = DiagConsumer.takeFileStats();
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -238,6 +238,9 @@
Semaphore
/// Inputs, corresponding to the current state of AST.
ParseInputs FileInputs;
+ /// Used to indicate to the tasks running inside the worker queue that AST is
+ /// being destroyed and the AST queue is being drained.
+ std::atomic ShuttingDown{false};
/// Whether the diagnostics for the current FileInputs were reported to the
/// users before.
bool DiagsWereReported = false;
@@ -401,8 +404,9 @@
}
}
-// We only need to build the AST if diagnostics were requested.
-if (WantDiags == WantDiagnostics::No)
+// We only need to build the AST if diagnostics were requested and the
+// callers are still interested in this ASTWorker (it's not shutting down).
+if (WantDiags == WantDiagnostics::No || ShuttingDown)
return;
// Get the AST for diagnostics.
@@ -509,6 +513,7 @@
bool ASTWorker::isASTCached() const { return IdleASTs.getUsedBytes(this) != 0; }
void ASTWorker::stop() {
+ ShuttingDown = true;
{
std::lock_guard Lock(Mutex);
assert(!Done && "stop() called twice");
Index: clangd/ClangdServer.h
===
--- clangd/ClangdServer.h
+++ clangd/ClangdServer.h
@@ -226,20 +226,12 @@
formatCode(llvm::StringRef Code, PathRef File,
ArrayRef Ranges);
- typedef uint64_t DocVersion;
-
- void consumeDiagnostics(PathRef File, DocVersion Version,
- std::vector Diags);
-
tooling::CompileCommand getCompileCommand(PathRef File);
const GlobalCompilationDatabase
DiagnosticsConsumer
const FileSystemProvider
- /// Used to synchronize diagnostic responses for added and removed files.
- llvm::StringMap InternalVersion;
-
Path ResourceDir;
// The index used to look up symbols. This could be:
// - null (all index functionality is optional)
@@ -259,12 +251,6 @@
llvm::Optional WorkspaceRoot;
std::shared_ptr PCHs;
- /// Used to serialize diagnostic callbacks.
- /// FIXME(ibiryukov): get rid of an extra map and put all version counters
- /// into CppFile.
- std::mutex DiagnosticsMutex;
- /// Maps from a filename to the latest version of reported diagnostics.
- llvm::StringMap ReportedDiagnosticVersions;
// WorkScheduler has to be the last member, because its destructor has to be
// called before all other members to stop the worker thread that references
// ClangdServer.
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -134,19 +134,17 @@
void ClangdServer::addDocument(PathRef File, StringRef Contents,
WantDiagnostics WantDiags) {
- DocVersion Version = ++InternalVersion[File];
ParseInputs Inputs = {getCompileCommand(File), FSProvider.getFileSystem(),
Contents.str()};
-
Path FileStr = File.str();
WorkScheduler.update(File, std::move(Inputs),
[PATCH] D54746: [clangd] Respect task cancellation in TUScheduler.
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rL347450: [clangd] Respect task cancellation in TUScheduler.
(authored by sammccall, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D54746?vs=174738=175024#toc
Repository:
rL LLVM
https://reviews.llvm.org/D54746
Files:
clang-tools-extra/trunk/clangd/Cancellation.cpp
clang-tools-extra/trunk/clangd/Cancellation.h
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
@@ -209,6 +209,75 @@
EXPECT_EQ(2, CallbackCount);
}
+TEST_F(TUSchedulerTests, Cancellation) {
+ // We have the following update/read sequence
+ // U0
+ // U1(WantDiags=Yes) <-- cancelled
+ //R1 <-- cancelled
+ // U2(WantDiags=Yes) <-- cancelled
+ //R2A <-- cancelled
+ //R2B
+ // U3(WantDiags=Yes)
+ //R3 <-- cancelled
+ std::vector DiagsSeen, ReadsSeen, ReadsCanceled;
+ {
+TUScheduler S(
+getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
+auto Path = testPath("foo.cpp");
+// Helper to schedule a named update and return a function to cancel it.
+auto Update = [&](std::string ID) -> Canceler {
+ auto T = cancelableTask();
+ WithContext C(std::move(T.first));
+ S.update(Path, getInputs(Path, "//" + ID), WantDiagnostics::Yes,
+ [&, ID](std::vector Diags) { DiagsSeen.push_back(ID); });
+ return std::move(T.second);
+};
+// Helper to schedule a named read and return a function to cancel it.
+auto Read = [&](std::string ID) -> Canceler {
+ auto T = cancelableTask();
+ WithContext C(std::move(T.first));
+ S.runWithAST(ID, Path, [&, ID](llvm::Expected E) {
+if (auto Err = E.takeError()) {
+ if (Err.isA()) {
+ReadsCanceled.push_back(ID);
+consumeError(std::move(Err));
+ } else {
+ADD_FAILURE() << "Non-cancelled error for " << ID << ": "
+ << llvm::toString(std::move(Err));
+ }
+} else {
+ ReadsSeen.push_back(ID);
+}
+ });
+ return std::move(T.second);
+};
+
+Notification Proceed; // Ensure we schedule everything.
+S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
+ [&](std::vector Diags) { Proceed.wait(); });
+// The second parens indicate cancellation, where present.
+Update("U1")();
+Read("R1")();
+Update("U2")();
+Read("R2A")();
+Read("R2B");
+Update("U3");
+Read("R3")();
+Proceed.notify();
+ }
+ EXPECT_THAT(DiagsSeen, ElementsAre("U2", "U3"))
+ << "U1 and all dependent reads were cancelled. "
+ "U2 has a dependent read R2A. "
+ "U3 was not cancelled.";
+ EXPECT_THAT(ReadsSeen, ElementsAre("R2B"))
+ << "All reads other than R2B were cancelled";
+ EXPECT_THAT(ReadsCanceled, ElementsAre("R1", "R2A", "R3"))
+ << "All reads other than R2B were cancelled";
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
Index: clang-tools-extra/trunk/clangd/Cancellation.cpp
===
--- clang-tools-extra/trunk/clangd/Cancellation.cpp
+++ clang-tools-extra/trunk/clangd/Cancellation.cpp
@@ -24,8 +24,8 @@
};
}
-bool isCancelled() {
- if (auto *Flag = Context::current().get(FlagKey))
+bool isCancelled(const Context ) {
+ if (auto *Flag = Ctx.get(FlagKey))
return **Flag;
return false; // Not in scope of a task.
}
Index: clang-tools-extra/trunk/clangd/TUScheduler.cpp
===
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp
@@ -43,6 +43,7 @@
// immediately.
#include "TUScheduler.h"
+#include "Cancellation.h"
#include "Logger.h"
#include "Trace.h"
#include "clang/Frontend/CompilerInvocation.h"
@@ -432,6 +433,8 @@
void ASTWorker::runWithAST(
StringRef Name, unique_function)> Action) {
auto Task = [=](decltype(Action) Action) {
+if (isCancelled())
+ return Action(make_error());
Optional> AST = IdleASTs.take(this);
if (!AST) {
std::unique_ptr Invocation =
@@ -588,6 +591,26 @@
Deadline ASTWorker::scheduleLocked() {
if (Requests.empty())
return Deadline::infinity(); // Wait
[PATCH] D54746: [clangd] Respect task cancellation in TUScheduler.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE347450: [clangd] Respect task cancellation in TUScheduler.
(authored by sammccall, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D54746?vs=174738=175025#toc
Repository:
rL LLVM
https://reviews.llvm.org/D54746
Files:
clangd/Cancellation.cpp
clangd/Cancellation.h
clangd/TUScheduler.cpp
clangd/TUScheduler.h
unittests/clangd/TUSchedulerTests.cpp
Index: unittests/clangd/TUSchedulerTests.cpp
===
--- unittests/clangd/TUSchedulerTests.cpp
+++ unittests/clangd/TUSchedulerTests.cpp
@@ -209,6 +209,75 @@
EXPECT_EQ(2, CallbackCount);
}
+TEST_F(TUSchedulerTests, Cancellation) {
+ // We have the following update/read sequence
+ // U0
+ // U1(WantDiags=Yes) <-- cancelled
+ //R1 <-- cancelled
+ // U2(WantDiags=Yes) <-- cancelled
+ //R2A <-- cancelled
+ //R2B
+ // U3(WantDiags=Yes)
+ //R3 <-- cancelled
+ std::vector DiagsSeen, ReadsSeen, ReadsCanceled;
+ {
+TUScheduler S(
+getDefaultAsyncThreadsCount(), /*StorePreamblesInMemory=*/true,
+/*ASTCallbacks=*/nullptr,
+/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
+ASTRetentionPolicy());
+auto Path = testPath("foo.cpp");
+// Helper to schedule a named update and return a function to cancel it.
+auto Update = [&](std::string ID) -> Canceler {
+ auto T = cancelableTask();
+ WithContext C(std::move(T.first));
+ S.update(Path, getInputs(Path, "//" + ID), WantDiagnostics::Yes,
+ [&, ID](std::vector Diags) { DiagsSeen.push_back(ID); });
+ return std::move(T.second);
+};
+// Helper to schedule a named read and return a function to cancel it.
+auto Read = [&](std::string ID) -> Canceler {
+ auto T = cancelableTask();
+ WithContext C(std::move(T.first));
+ S.runWithAST(ID, Path, [&, ID](llvm::Expected E) {
+if (auto Err = E.takeError()) {
+ if (Err.isA()) {
+ReadsCanceled.push_back(ID);
+consumeError(std::move(Err));
+ } else {
+ADD_FAILURE() << "Non-cancelled error for " << ID << ": "
+ << llvm::toString(std::move(Err));
+ }
+} else {
+ ReadsSeen.push_back(ID);
+}
+ });
+ return std::move(T.second);
+};
+
+Notification Proceed; // Ensure we schedule everything.
+S.update(Path, getInputs(Path, ""), WantDiagnostics::Yes,
+ [&](std::vector Diags) { Proceed.wait(); });
+// The second parens indicate cancellation, where present.
+Update("U1")();
+Read("R1")();
+Update("U2")();
+Read("R2A")();
+Read("R2B");
+Update("U3");
+Read("R3")();
+Proceed.notify();
+ }
+ EXPECT_THAT(DiagsSeen, ElementsAre("U2", "U3"))
+ << "U1 and all dependent reads were cancelled. "
+ "U2 has a dependent read R2A. "
+ "U3 was not cancelled.";
+ EXPECT_THAT(ReadsSeen, ElementsAre("R2B"))
+ << "All reads other than R2B were cancelled";
+ EXPECT_THAT(ReadsCanceled, ElementsAre("R1", "R2A", "R3"))
+ << "All reads other than R2B were cancelled";
+}
+
TEST_F(TUSchedulerTests, ManyUpdates) {
const int FilesCount = 3;
const int UpdatesPerFile = 10;
Index: clangd/Cancellation.cpp
===
--- clangd/Cancellation.cpp
+++ clangd/Cancellation.cpp
@@ -24,8 +24,8 @@
};
}
-bool isCancelled() {
- if (auto *Flag = Context::current().get(FlagKey))
+bool isCancelled(const Context ) {
+ if (auto *Flag = Ctx.get(FlagKey))
return **Flag;
return false; // Not in scope of a task.
}
Index: clangd/TUScheduler.cpp
===
--- clangd/TUScheduler.cpp
+++ clangd/TUScheduler.cpp
@@ -43,6 +43,7 @@
// immediately.
#include "TUScheduler.h"
+#include "Cancellation.h"
#include "Logger.h"
#include "Trace.h"
#include "clang/Frontend/CompilerInvocation.h"
@@ -432,6 +433,8 @@
void ASTWorker::runWithAST(
StringRef Name, unique_function)> Action) {
auto Task = [=](decltype(Action) Action) {
+if (isCancelled())
+ return Action(make_error());
Optional> AST = IdleASTs.take(this);
if (!AST) {
std::unique_ptr Invocation =
@@ -588,6 +591,26 @@
Deadline ASTWorker::scheduleLocked() {
if (Requests.empty())
return Deadline::infinity(); // Wait for new requests.
+ // Handle cancelled requests first so the rest of the scheduler doesn't.
+ for (auto I = Requests.begin(), E = Requests.end(); I != E; ++I) {
+if (!isCancelled(I->Ctx)) {
+ // Cancellations after the first read don't affect current scheduling.
+ if (I->UpdateType == None)
+break;
+ continue;
+}
+// Cancelled reads are moved to the front of the queue and run
[clang-tools-extra] r347450 - [clangd] Respect task cancellation in TUScheduler.
Author: sammccall
Date: Thu Nov 22 02:22:16 2018
New Revision: 347450
URL: http://llvm.org/viewvc/llvm-project?rev=347450=rev
Log:
[clangd] Respect task cancellation in TUScheduler.
Summary:
- Reads are never executed if canceled before ready-to run.
In practice, we finalize cancelled reads eagerly and out-of-order.
- Cancelled reads don't prevent prior updates from being elided, as they don't
actually depend on the result of the update.
- Updates are downgraded from WantDiagnostics::Yes to WantDiagnostics::Auto when
cancelled, which allows them to be elided when all dependent reads are
cancelled and there are subsequent writes. (e.g. when the queue is backed up
with cancelled requests).
The queue operations aren't optimal (we scan the whole queue for cancelled
tasks every time the scheduler runs, and check cancellation twice in the end).
However I believe these costs are still trivial in practice (compared to any
AST operation) and the logic can be cleanly separated from the rest of the
scheduler.
Reviewers: ilya-biryukov
Subscribers: javed.absar, ioeric, MaskRay, jkorous, arphaman, kadircet,
cfe-commits
Differential Revision: https://reviews.llvm.org/D54746
Modified:
clang-tools-extra/trunk/clangd/Cancellation.cpp
clang-tools-extra/trunk/clangd/Cancellation.h
clang-tools-extra/trunk/clangd/TUScheduler.cpp
clang-tools-extra/trunk/clangd/TUScheduler.h
clang-tools-extra/trunk/unittests/clangd/TUSchedulerTests.cpp
Modified: clang-tools-extra/trunk/clangd/Cancellation.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Cancellation.cpp?rev=347450=347449=347450=diff
==
--- clang-tools-extra/trunk/clangd/Cancellation.cpp (original)
+++ clang-tools-extra/trunk/clangd/Cancellation.cpp Thu Nov 22 02:22:16 2018
@@ -24,8 +24,8 @@ std::pair cancelableT
};
}
-bool isCancelled() {
- if (auto *Flag = Context::current().get(FlagKey))
+bool isCancelled(const Context ) {
+ if (auto *Flag = Ctx.get(FlagKey))
return **Flag;
return false; // Not in scope of a task.
}
Modified: clang-tools-extra/trunk/clangd/Cancellation.h
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Cancellation.h?rev=347450=347449=347450=diff
==
--- clang-tools-extra/trunk/clangd/Cancellation.h (original)
+++ clang-tools-extra/trunk/clangd/Cancellation.h Thu Nov 22 02:22:16 2018
@@ -79,7 +79,7 @@ std::pair cancelableT
/// True if the current context is within a cancelable task which was
cancelled.
/// Always false if there is no active cancelable task.
/// This isn't free (context lookup) - don't call it in a tight loop.
-bool isCancelled();
+bool isCancelled(const Context = Context::current());
/// Conventional error when no result is returned due to cancellation.
class CancelledError : public llvm::ErrorInfo {
Modified: clang-tools-extra/trunk/clangd/TUScheduler.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/TUScheduler.cpp?rev=347450=347449=347450=diff
==
--- clang-tools-extra/trunk/clangd/TUScheduler.cpp (original)
+++ clang-tools-extra/trunk/clangd/TUScheduler.cpp Thu Nov 22 02:22:16 2018
@@ -43,6 +43,7 @@
// immediately.
#include "TUScheduler.h"
+#include "Cancellation.h"
#include "Logger.h"
#include "Trace.h"
#include "clang/Frontend/CompilerInvocation.h"
@@ -432,6 +433,8 @@ void ASTWorker::update(ParseInputs Input
void ASTWorker::runWithAST(
StringRef Name, unique_function)> Action) {
auto Task = [=](decltype(Action) Action) {
+if (isCancelled())
+ return Action(make_error());
Optional> AST = IdleASTs.take(this);
if (!AST) {
std::unique_ptr Invocation =
@@ -588,6 +591,26 @@ void ASTWorker::run() {
Deadline ASTWorker::scheduleLocked() {
if (Requests.empty())
return Deadline::infinity(); // Wait for new requests.
+ // Handle cancelled requests first so the rest of the scheduler doesn't.
+ for (auto I = Requests.begin(), E = Requests.end(); I != E; ++I) {
+if (!isCancelled(I->Ctx)) {
+ // Cancellations after the first read don't affect current scheduling.
+ if (I->UpdateType == None)
+break;
+ continue;
+}
+// Cancelled reads are moved to the front of the queue and run immediately.
+if (I->UpdateType == None) {
+ Request R = std::move(*I);
+ Requests.erase(I);
+ Requests.push_front(std::move(R));
+ return Deadline::zero();
+}
+// Cancelled updates are downgraded to auto-diagnostics, and may be elided.
+if (I->UpdateType == WantDiagnostics::Yes)
+ I->UpdateType = WantDiagnostics::Auto;
+ }
+
while (shouldSkipHeadLocked())
Requests.pop_front();
assert(!Requests.empty() && "skipped the whole queue");
Modified:
[PATCH] D54798: Move the llvm lit test dependencies to clang-tools-extra.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL347449: Move the llvm lit test dependencies to
clang-tools-extra. (authored by hokein, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D54798
Files:
clang-tools-extra/trunk/test/CMakeLists.txt
Index: clang-tools-extra/trunk/test/CMakeLists.txt
===
--- clang-tools-extra/trunk/test/CMakeLists.txt
+++ clang-tools-extra/trunk/test/CMakeLists.txt
@@ -71,6 +71,17 @@
clangd-indexer
dexp
)
+
+# Add lit test dependencies.
+set(LLVM_UTILS_DEPS
+ FileCheck count not
+)
+foreach(dep ${LLVM_UTILS_DEPS})
+ if(TARGET ${dep})
+list(APPEND CLANGD_TEST_DEPS ${dep})
+ endif()
+endforeach()
+
foreach(clangd_dep ${CLANGD_TEST_DEPS})
list(APPEND CLANG_TOOLS_TEST_DEPS
${clangd_dep})
Index: clang-tools-extra/trunk/test/CMakeLists.txt
===
--- clang-tools-extra/trunk/test/CMakeLists.txt
+++ clang-tools-extra/trunk/test/CMakeLists.txt
@@ -71,6 +71,17 @@
clangd-indexer
dexp
)
+
+# Add lit test dependencies.
+set(LLVM_UTILS_DEPS
+ FileCheck count not
+)
+foreach(dep ${LLVM_UTILS_DEPS})
+ if(TARGET ${dep})
+list(APPEND CLANGD_TEST_DEPS ${dep})
+ endif()
+endforeach()
+
foreach(clangd_dep ${CLANGD_TEST_DEPS})
list(APPEND CLANG_TOOLS_TEST_DEPS
${clangd_dep})
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r347449 - Move the llvm lit test dependencies to clang-tools-extra.
Author: hokein
Date: Thu Nov 22 02:14:55 2018
New Revision: 347449
URL: http://llvm.org/viewvc/llvm-project?rev=347449=rev
Log:
Move the llvm lit test dependencies to clang-tools-extra.
Summary: Part of revert r343473
Reviewers: mgorny
Subscribers: cfe-commits
Differential Revision: https://reviews.llvm.org/D54798
Modified:
clang-tools-extra/trunk/test/CMakeLists.txt
Modified: clang-tools-extra/trunk/test/CMakeLists.txt
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/CMakeLists.txt?rev=347449=347448=347449=diff
==
--- clang-tools-extra/trunk/test/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/test/CMakeLists.txt Thu Nov 22 02:14:55 2018
@@ -71,6 +71,17 @@ set(CLANGD_TEST_DEPS
clangd-indexer
dexp
)
+
+# Add lit test dependencies.
+set(LLVM_UTILS_DEPS
+ FileCheck count not
+)
+foreach(dep ${LLVM_UTILS_DEPS})
+ if(TARGET ${dep})
+list(APPEND CLANGD_TEST_DEPS ${dep})
+ endif()
+endforeach()
+
foreach(clangd_dep ${CLANGD_TEST_DEPS})
list(APPEND CLANG_TOOLS_TEST_DEPS
${clangd_dep})
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54781: [clangd] Add 'Switch header/source' command in clangd-vscode
hokein added inline comments.
Comment at: clangd/clients/clangd-vscode/src/extension.ts:4
import { realpathSync } from 'fs';
+import { RequestType, TextDocumentIdentifier } from 'vscode-languageclient';
We have imported the whole module as `vscodelc`, maybe just use
`vscodelc.RequestType`, `vscodelc.TextDocumentIdentifier`?
Comment at: clangd/clients/clangd-vscode/src/extension.ts:16
+export namespace SwitchSourceHeaderRequest {
+export const type = new RequestType('textDocument/switchSourceHeader');
Do we want to export this `SwitchSourceHeaderRequest`? it seems that we only
use it in this module.
Comment at: clangd/clients/clangd-vscode/src/extension.ts:17
+export namespace SwitchSourceHeaderRequest {
+export const type = new RequestType('textDocument/switchSourceHeader');
+}
Is `textDocument/switchSourceHeader` a built-in support in vscode? Is it
documented somewhere? I couldn't find any official document at
https://code.visualstudio.com. Am I missing anything here?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54781
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates
balazske added inline comments.
Comment at: lib/AST/DeclBase.cpp:1469
assert(Pos != Map->end() && "no lookup entry for decl");
-if (Pos->second.getAsVector() || Pos->second.getAsDecl() == ND)
+// Remove the decl only if it is contained.
+if ((Pos->second.getAsVector() && Pos->second.containsInVector(ND)) ||
martong wrote:
> Szelethus wrote:
> > Contained in?
> Indeed, `containedInVector` sounds better, so I renamed.
For me, `containsInVector` is the better name, or `hasInVector` ("contains" is
already used at other places but not "contained" and it sounds like it is not
contained any more).
Repository:
rC Clang
https://reviews.llvm.org/D53655
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54798: Move the llvm lit test dependencies to clang-tools-extra.
mgorny accepted this revision. mgorny added a comment. This revision is now accepted and ready to land. LGTM (assuming you've tested it). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54798 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54253: [OpenCL] Launch opencl-types.cl test only on x86
asavonic added a comment. FWIW, I'd vote for the first revision of this patch. From my understanding, the test verifies that libclang is able to parse OpenCL code correctly. It doesn't do anything specific to x86: target for x86 just happens to support a set of OpenCL extensions. https://reviews.llvm.org/D54253 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54781: [clangd] Add 'Switch header/source' command in clangd-vscode
ilya-biryukov added a comment. And many thanks for the change! I've tried it out, will definitely be one of the most-used clangd features for me :-) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54781: [clangd] Add 'Switch header/source' command in clangd-vscode
ilya-biryukov added a comment.
Overall LG, merely stylistic NITs.
Comment at: clangd/clients/clangd-vscode/src/extension.ts:69
+const docIdentifier = TextDocumentIdentifier.create(uri.toString());
+clangdClient.sendRequest(SwitchSourceHeaderRequest.type,
docIdentifier).then(sourceUri => {
+if (sourceUri !== undefined) {
Maybe use async/await to improve readabiity? I.e.
```
/*...*/push(registerCommand('...', async() => {
// ...
const sourceUri = await
clangdClient.sendRequest(SwitchSourceHeaderRequest.type, docIdentifier);
if (!sourceUri) {
return;
}
const doc = await //
}
```
Comment at: clangd/clients/clangd-vscode/src/extension.ts:70
+clangdClient.sendRequest(SwitchSourceHeaderRequest.type,
docIdentifier).then(sourceUri => {
+if (sourceUri !== undefined) {
+
vscode.workspace.openTextDocument(vscode.Uri.parse(sourceUri)).then(doc =>
vscode.window.showTextDocument(doc));
Maybe do `if (!sourceUri)` to use consistent style with the first check in the
lambda (i.e. `if (!uri)`)
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54781
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54760: [clangd] Cleanup: make the diags callback global in TUScheduler
sammccall added a comment.
Just the context thing. Rest is some optional simplifications.
Comment at: clangd/ClangdServer.cpp:77
+FileIndex *FIndex,
+llvm::unique_function)> OnDiags) {
+ using DiagsCallback = decltype(OnDiags);
hmm, the double-indirection for diags seems a bit funny, especially since we
have a dedicated method on ClangdServer so the lambda is trivial.
Maybe we should just make the CB class nested or friend in ClangdServer, and
pass it a ClangdServer*? Then it can access DynamicIdx and consumeDiagnostics
directly. The decoupling here isn't that strong to start with.
Comment at: clangd/ClangdServer.h:232
+ /// addDocument. Used to avoid races when sending diagnostics to the clients.
+ static Key DocVersionKey;
ilya-biryukov wrote:
> sammccall wrote:
> > I'm not sure using context here buys much: there aren't many layers,
> > they're fairly coupled, and this information would look pretty natural in
> > the interfaces.
> >
> > What about:
> > - move the definition of DocVersion to TUScheduler
> > - make DocVersion a member of ParseInputs
> > - pass (PathRef, DocVersion, vector) to the TUScheduler's diag
> > callback
> I'd keep it separate: `ParseInputs` is defined in `ClangdUnit.cpp` and it
> serves the purpose of defining everything we need to run the compiler.
> Adding `DocVersion` there would make no sense: it's of no use to the actual
> code doing preamble builds or parsing.
>
> I would rather aim to get rid of `DocVersion` (for this particular purpose,
> there are other ways to fix the raciness that the DocVersions workaround).
> WDYT?
Yeah, separating it from ParseInputs makes sense conceptually, and getting rid
of it is indeed better.
I don't think this justifies using Context.
Passing it as a separate parameter seems fine to me. Putting it in ParseInputs
with a FIXME is a little less principled but avoids interface churn.
Comment at: clangd/TUScheduler.cpp:156
class ASTWorker {
+private:
friend class ASTWorkerHandle;
nit: we generally have the private section at the top without explicitly
introducing it (as common in LLVM), or at the bottom. I'm fine with either
style but would prefer not to add a third.
Comment at: clangd/TUScheduler.h:92
+ ASTRetentionPolicy RetentionPolicy,
+ DiagsCallback OnDiags = nullptr);
~TUScheduler();
ilya-biryukov wrote:
> sammccall wrote:
> > ISTM the callback would fit nicely on the ASTCallbacks?
> > It even gets plumbed through to ASTWorker correctly by reference.
> Done. This is definitely less plumbing, but IMO the public interface looks a
> bit worse now.
> Consuming ASTs and diagnostics are two independent concerns and it's a bit
> awkward to combine them in the same struct.
>
> But we don't have too many callers, updating those is easy.
(Separate yes, but I'm not sure they're that much more separate than consuming
preambles vs ASTs, really. Outside of the 2/1 split of where the data goes
today...)
Comment at: unittests/clangd/TUSchedulerTests.cpp:38
+/// cancelled.
+void updateWithCallback(TUScheduler , PathRef File, ParseInputs Inputs,
+WantDiagnostics WD, llvm::unique_function CB) {
ilya-biryukov wrote:
> sammccall wrote:
> > ParseInputs is always getInputs(File), you could do that here (similarly
> > for `updateWithDiags`) to avoid repetition.
> It's actually a bit more complicated: `getInputs(File, Contents)`, note the
> second parameter.
> Done anyway, definitely looks better (and less prone to mistakes!).
>
> The calls are now `updateWith(S, File, Contents...)`, there's one place that
> still relies on the `Inputs` though (checks the VFS does not change in the
> following read).
>
Exactly, thanks!
(I forgot to mention Contents in the comment, doh)
Comment at: unittests/clangd/TUSchedulerTests.cpp:495
/*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
- ASTRetentionPolicy());
+ ASTRetentionPolicy(), captureDiags);
ilya-biryukov wrote:
> sammccall wrote:
> > can we just increment a counter in the callback, and have DoUpdate look for
> > a delta?
> Maybe... I haven't looked closely into the test, would prefer to keep this
> change mostly mechanical and avoid digging into the test logic.
> Happy to take a look into simplifying the test separately, of course.
It seems like a trivial change, and it'd be nice not to depend on the new
machinery you're introducing here when we don't have to.
I'm not going to insist, but this is just...
```
atomic DiagsReceived;
TUScheduler S(..., [&](Diags) { ++ DiagsReceived }, ...);
...
DoUpdate = [&] -> bool { // as before
unsigned PrevDiags = DiagsReceived;
S.update(...); blockUntilIdle(); // as before
return DiagsReceived != PrevDiags;
}
```
Repository:
[PATCH] D54781: [clangd] Add 'Switch header/source' command in clangd-vscode
ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. lgtm Could you run clang-format on the changed lines? Comment at: clangd/clients/clangd-vscode/package.json:81 +"command": "clangd-vscode.switchheadersource", +"title": "C/C++: Switch between Source/Header" +} and objc? Comment at: clangd/clients/clangd-vscode/src/extension.ts:55 protocol2Code: (value: string) => vscode.Uri.file(realpathSync(vscode.Uri.parse(value).fsPath)) } We might want to share the logic for converting protocol URI to vscode URI. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D54781 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54798: Move the llvm lit test dependencies to clang-tools-extra.
hokein updated this revision to Diff 175021.
hokein added a comment.
Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54798
Files:
test/CMakeLists.txt
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -71,6 +71,17 @@
clangd-indexer
dexp
)
+
+# Add lit test dependencies.
+set(LLVM_UTILS_DEPS
+ FileCheck count not
+)
+foreach(dep ${LLVM_UTILS_DEPS})
+ if(TARGET ${dep})
+list(APPEND CLANGD_TEST_DEPS ${dep})
+ endif()
+endforeach()
+
foreach(clangd_dep ${CLANGD_TEST_DEPS})
list(APPEND CLANG_TOOLS_TEST_DEPS
${clangd_dep})
Index: test/CMakeLists.txt
===
--- test/CMakeLists.txt
+++ test/CMakeLists.txt
@@ -71,6 +71,17 @@
clangd-indexer
dexp
)
+
+# Add lit test dependencies.
+set(LLVM_UTILS_DEPS
+ FileCheck count not
+)
+foreach(dep ${LLVM_UTILS_DEPS})
+ if(TARGET ${dep})
+list(APPEND CLANGD_TEST_DEPS ${dep})
+ endif()
+endforeach()
+
foreach(clangd_dep ${CLANGD_TEST_DEPS})
list(APPEND CLANG_TOOLS_TEST_DEPS
${clangd_dep})
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r347446 - The result of is.constant() is unsigned.
Author: void
Date: Thu Nov 22 01:31:08 2018
New Revision: 347446
URL: http://llvm.org/viewvc/llvm-project?rev=347446=rev
Log:
The result of is.constant() is unsigned.
Added:
cfe/trunk/test/CodeGen/builtin-constant-p.c
Modified:
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=347446=347445=347446=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Thu Nov 22 01:31:08 2018
@@ -1943,7 +1943,7 @@ RValue CodeGenFunction::EmitBuiltinExpr(
Value *F = CGM.getIntrinsic(Intrinsic::is_constant, ConvertType(ArgType));
Value *Result = Builder.CreateCall(F, ArgValue);
if (Result->getType() != ResultType)
- Result = Builder.CreateIntCast(Result, ResultType, /*isSigned*/true);
+ Result = Builder.CreateIntCast(Result, ResultType, /*isSigned*/false);
return RValue::get(Result);
}
case Builtin::BI__builtin_object_size: {
Added: cfe/trunk/test/CodeGen/builtin-constant-p.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/builtin-constant-p.c?rev=347446=auto
==
--- cfe/trunk/test/CodeGen/builtin-constant-p.c (added)
+++ cfe/trunk/test/CodeGen/builtin-constant-p.c Thu Nov 22 01:31:08 2018
@@ -0,0 +1,130 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s -O2 |
FileCheck %s
+
+int a = 42;
+
+inline int bcp(int x) {
+ return __builtin_constant_p(x);
+}
+
+/* --- Compound literals */
+
+struct foo { int x, y; };
+
+struct foo test0(int expr) {
+ // CHECK: define i64 @test0(i32 %expr)
+ // CHECK: call i1 @llvm.is.constant.i32(i32 %expr)
+ struct foo f = (struct foo){ __builtin_constant_p(expr), 42 };
+ return f;
+}
+
+/* --- Pointer types */
+
+inline int test1_i(int *x) {
+ return *x;
+}
+
+int test1() {
+ // CHECK: define i32 @test1
+ // CHECK: add nsw i32 %0, -13
+ // CHECK-NEXT: call i1 @llvm.is.constant.i32(i32 %sub)
+ return bcp(test1_i() - 13);
+}
+
+int test2() {
+ // CHECK: define i32 @test2
+ // CHECK: ret i32 0
+ return __builtin_constant_p( - 13);
+}
+
+inline int test3_i(int *x) {
+ return 42;
+}
+
+int test3() {
+ // CHECK: define i32 @test3
+ // CHECK: ret i32 1
+ return bcp(test3_i() - 13);
+}
+
+/* --- Aggregate types */
+
+int b[] = {1, 2, 3};
+
+int test4() {
+ // CHECK: define i32 @test4
+ // CHECK: ret i32 0
+ return __builtin_constant_p(b);
+}
+
+const char test5_c[] = {1, 2, 3, 0};
+
+int test5() {
+ // CHECK: define i32 @test5
+ // CHECK: ret i32 0
+ return __builtin_constant_p(test5_c);
+}
+
+inline char test6_i(const char *x) {
+ return x[1];
+}
+
+int test6() {
+ // CHECK: define i32 @test6
+ // CHECK: ret i32 0
+ return __builtin_constant_p(test6_i(test5_c));
+}
+
+/* --- Non-constant global variables */
+
+int test7() {
+ // CHECK: define i32 @test7
+ // CHECK: call i1 @llvm.is.constant.i32(i32 %0)
+ return bcp(a);
+}
+
+/* --- Constant global variables */
+
+const int c = 42;
+
+int test8() {
+ // CHECK: define i32 @test8
+ // CHECK: ret i32 1
+ return bcp(c);
+}
+
+/* --- Array types */
+
+int arr[] = { 1, 2, 3 };
+const int c_arr[] = { 1, 2, 3 };
+
+int test9() {
+ // CHECK: define i32 @test9
+ // CHECK: call i1 @llvm.is.constant.i32(i32 %0)
+ return __builtin_constant_p(arr[2]);
+}
+
+int test10() {
+ // CHECK: define i32 @test10
+ // CHECK: ret i32 1
+ return __builtin_constant_p(c_arr[2]);
+}
+
+int test11() {
+ // CHECK: define i32 @test11
+ // CHECK: ret i32 0
+ return __builtin_constant_p(c_arr);
+}
+
+/* --- Function pointers */
+
+int test12() {
+ // CHECK: define i32 @test12
+ // CHECK: ret i32 0
+ return __builtin_constant_p();
+}
+
+int test13() {
+ // CHECK: define i32 @test13
+ // CHECK: ret i32 1
+ return __builtin_constant_p( != 0);
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D54796: [clangd] **Prototype**: C++ API for emitting file status
ilya-biryukov added a comment. Have you considered moving **all** status updates into the TUScheduler? TUScheduler has full information about the file status changes, so it seems to most natural place to provide this kind of information. Repository: rCTE Clang Tools Extra 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] D53738: [Fixed Point Arithmetic] Fixed Point Addition
ebevhan added a comment. In https://reviews.llvm.org/D53738#1305498, @leonardchan wrote: > @ebevhan Any more comments on this patch? It's strange, I feel like I've responded to the last comment twice now but there's nothing in Phab. Generally I think it's good! One final note; I assume we could technically reuse/rename the EmitFixedPointAdd function and use it to emit other binops when those are added? Comment at: clang/test/Frontend/fixed_point_add.c:269 + // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = call i15 @llvm.uadd.sat.i15(i15 [[USA_TRUNC]], i15 [[USA_SAT_TRUNC]]) + // UNSIGNED-NEXT: [[SUM_EXT:%[a-z0-9]+]] = zext i15 [[SUM]] to i16 + // UNSIGNED-NEXT: store i16 [[SUM_EXT]], i16* %usa_sat, align 2 leonardchan wrote: > ebevhan wrote: > > This is probably a candidate for an isel optimization. This operation also > > works as an `i16 ssat.add` with a negative-clamp-to-zero afterwards, and if > > the target supports `i16 ssat.add` natively then it will likely be a lot > > more efficient than whatever an `i15 uadd.sat` produces. > Do you think it would be more efficient for now then if instead we did SHL by > 1, saturate, then [AL]SHR by 1? This way we could use `i16 ssat.add` instead > of `i15 ssat.add`? We should probably just do it in isel or instcombine instead. We don't know at this point which intrinsic is a better choice (though, I think power-of-two-types are generally better). Repository: rC Clang https://reviews.llvm.org/D53738 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
