[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing updated this revision to Diff 151605.
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,15 @@
if ((i = 4)) {}
}
+// testing macros
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
+// non-nesting macros
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
void bitwise_rel(unsigned i) {
(void)(i & 0x2 == 0); // expected-warning {{& has lower precedence than ==}} \
// expected-note{{place parentheses around the '==' expression to silence this warning}} \
@@ -96,6 +105,74 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ //===--
+ // Logical operator in macros
+ //===--
+
+ // actionable expression tests
+ NON_NESTING_VOID_0(i && i || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_0((i && i) || i); // no warning.
+
+ // NON_NESTING_VOID_1(&&, ||, i && i || i, i, i);
+ // will be expanded to:
+ // i && i || i && i || i
+ // ^-^ arg position 2 (i && i || i) should be checked because `op ||` and `lhs (i && i)` from same arg position
+ // ^ arg position 0 (&&)
+ //^ arg position 3 (i)
+ // ^ arg position 1 (||) should not be checked because `op ||` and nothing from same arg position
+ // ^ arg position 4 (i)
+ NON_NESTING_VOID_1(&&, ||, i && i || i, i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_1(&&, ||, ((i && i) || i), i, i); // no warning.
+
+ // same as this one
+ NESTING_VOID_WRAPPER(&&, ||, i && i || i, i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NESTING_VOID_WRAPPER(&&, ||, ((i && i) || i), i, i); // no waring.
+
+ // NON_NESTING_VOID_1(&&, ||, i, i && i || i, i);
+ // will be expanded to:
+ // i && i && i || i || i
+ // ^ arg position 2 (i)
+ //^ arg position 0 (&&)
+ // ^-^ arg position 3 (i && i || i) should be checked because `op ||` and `lhs i && i` from same arg position
+ // ^ arg position 1 (||) should not be checked because `op ||` and nothing from same arg postion
+ // ^ arg position 4 (i)
+ NON_NESTING_VOID_1(&&, ||, i, i && i || i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_1(&&, ||, i, (i && i) || i, i); // no warning.
+
+ // same as this one
+ NESTING_VOID_WRAPPER(&&, ||, i, i && i || i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NESTING_VOID_WRAPPER(&&, ||, i, (i && i) || i, i); // no warning.
+
+ // un-actionable expression tests
+ // NON_NESTING_VOID_1(&&, ||, i, i, i);
+ // will be expanded to:
+ // i && i || i
+ // ^ arg position 2 (i)
+ //^ arg position 0 (&&)
+ // ^ arg position 3 (||) should not be checked becaues `op ||` and nothing from same arg position
+ // ^ arg position 1 (i)
+ // ^ arg position 4 (i)
+ NON_NESTING_VOID_1(&&, ||, i, i, i); // no warning.
+ NESTING_VOID_WRAPPER(&&, ||, i, i, i); // no warning.
+
+ // NON_NESTING_VOID_1(&&, ||, i, i, i && i || i);
+ // will be expanded to:
+ // i && i || i && i || i
+ // ^ arg positon 2 (i)
+ //^ arg position 0 (&&)
+ // ^ arg position 3 (i)
+ // ^ arg position 1 (||) should not be checked because `op ||` and nothing from same arg position
+ // ^-^ arg position 4 (i && i || i) should not be checked because `op ||` and nothing from same arg position
+ NON_NESTING_VOID_1(&&, ||, i, i, i && i || i); // no warning.
+ NESTING_VOID_WRAPPER(&&, ||, i, i, i && i || i); // no warning.
+
+ // same for something expression like (i || i && i);
}
_Bool someConditionFunc();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12310,9 +12310,30 @@
//
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing updated this revision to Diff 151604.
Higuoxing added a comment.
Ping, thanks
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,15 @@
if ((i = 4)) {}
}
+// testing macros
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
+// non-nesting macros
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
void bitwise_rel(unsigned i) {
(void)(i & 0x2 == 0); // expected-warning {{& has lower precedence than ==}} \
// expected-note{{place parentheses around the '==' expression to silence this warning}} \
@@ -96,6 +105,78 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ //===--
+ // Logical operator in macros
+ //===--
+
+ // actionable expression tests
+ NON_NESTING_VOID_0(i && i || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_0((i && i) || i); // no warning.
+
+ // NON_NESTING_VOID_1(&&, ||, i && i || i, i, i);
+ // will be expanded to:
+ // i && i || i && i || i
+ // ^-^ arg position 2 (i && i || i) should be checked because `op ||` and `lhs (i && i)` from same arg position
+ // ^ arg position 0 (&&)
+ //^ arg position 3 (i)
+ // ^ arg position 1 (||) should not be checked because `op ||` and nothing from same arg position
+ // ^ arg position 4 (i)
+ NON_NESTING_VOID_1(&&, ||, i && i || i, i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}} \
+ // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_1(&&, ||, ((i && i) || i), i, i); // no warning.
+
+ // same as this one
+ NESTING_VOID_WRAPPER(&&, ||, i && i || i, i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}} \
+ // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NESTING_VOID_WRAPPER(&&, ||, ((i && i) || i), i, i); // no waring.
+
+ // NON_NESTING_VOID_1(&&, ||, i, i && i || i, i);
+ // will be expanded to:
+ // i && i && i || i || i
+ // ^ arg position 2 (i)
+ //^ arg position 0 (&&)
+ // ^-^ arg position 3 (i && i || i) should be checked because `op ||` and `lhs i && i` from same arg position
+ // ^ arg position 1 (||) should not be checked because `op ||` and nothing from same arg postion
+ // ^ arg position 4 (i)
+ NON_NESTING_VOID_1(&&, ||, i, i && i || i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_1(&&, ||, i, (i && i) || i, i); // no warning.
+
+ // same as this one
+ NESTING_VOID_WRAPPER(&&, ||, i, i && i || i, i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around the '&&' expression to silence this warning}}
+ NESTING_VOID_WRAPPER(&&, ||, i, (i && i) || i, i); // no warning.
+
+ // un-actionable expression tests
+ // NON_NESTING_VOID_1(&&, ||, i, i, i);
+ // will be expanded to:
+ // i && i || i
+ // ^ arg position 2 (i)
+ //^ arg position 0 (&&)
+ // ^ arg position 3 (||) should not be checked becaues `op ||` and nothing from same arg position
+ // ^ arg position 1 (i)
+ // ^ arg position 4 (i)
+ NON_NESTING_VOID_1(&&, ||, i, i, i); // no warning.
+ NESTING_VOID_WRAPPER(&&, ||, i, i, i); // no warning.
+
+ // NON_NESTING_VOID_1(&&, ||, i, i, i && i || i);
+ // will be expanded to:
+ // i && i || i && i || i
+ // ^ arg positon 2 (i)
+ //^ arg position 0 (&&)
+ // ^ arg position 3 (i)
+ // ^ arg position 1 (||) should not be checked because `op ||` and nothing from same arg position
+ //
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing updated this revision to Diff 151599.
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,15 @@
if ((i = 4)) {}
}
+// testing macros
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
+// non-nesting macros
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
void bitwise_rel(unsigned i) {
(void)(i & 0x2 == 0); // expected-warning {{& has lower precedence than ==}}
\
// expected-note{{place parentheses around the '=='
expression to silence this warning}} \
@@ -96,6 +105,33 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ //===--
+ // Logical operator in macros
+ //===--
+
+ // actionable expression tests
+ NON_NESTING_VOID_0(i && i || i); // expected-warning {{'&&' within '||'}} \
+ // expected-note {{place parentheses around
the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_0((i && i) || i); // no warning.
+
+ NON_NESTING_VOID_1(&&, ||, i, i && i || i, i); // expected-warning {{'&&'
within '||'}} \
+ // expected-note {{place
parentheses around the '&&' expression to silence this warning}}
+ NON_NESTING_VOID_1(&&, ||, i, (i && i) || i, i); // no warning.
+
+ NESTING_VOID_WRAPPER(&&, ||, i, i && i || i, i); // expected-warning {{'&&'
within '||'}} \
+ // expected-note {{place
parentheses around the '&&' expression to silence this warning}}
+ NESTING_VOID_WRAPPER(&&, ||, i, (i && i) || i, i); // no warning.
+
+ // FIXME: strange things, but I think this could be tolerated
+ NON_NESTING_VOID_1(&&, ||, i && i || i, i, i); // no warning. but expected.
because op and LHS are not from same arg position
+ NON_NESTING_VOID_1(&&, ||, i, i, i && i || i); // no warning. but expected.
because op and RHS are not from same arg position
+ NESTING_VOID_WRAPPER(&&, ||, i && i || i, i, i); // no warning. but
expected. because op and LHS are not from same arg position
+ NESTING_VOID_WRAPPER(&&, ||, i, i, i && i || i); // no warning. but
expected. because op and RHS are not from same arg position
+
+ // un-actionable expression tests
+ NESTING_VOID_WRAPPER(&&, ||, i, i, i); // no warning.
+ NON_NESTING_VOID_1(&&, ||, i, i, i); // no warning.
}
_Bool someConditionFunc();
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12310,9 +12310,26 @@
// Warn about arg1 || arg2 && arg3, as GCC 4.3+ does.
// We don't warn for 'assert(a || b && "bad")' since this is safe.
- if (Opc == BO_LOr && !OpLoc.isMacroID()/* Don't warn in macros. */) {
-DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
-DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+ if (Opc == BO_LOr) {
+if (!OpLoc.isMacroID()) {
+ // Operator is not in macros
+ DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
+ DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+} else {
+ // This Expression is expanded from macro
+ SourceLocation LHSExpansionLoc, OpExpansionLoc, RHSExpansionLoc;
+ if (Self.getSourceManager().isMacroArgExpansion(OpLoc,
+ ) &&
+ Self.getSourceManager().isMacroArgExpansion(LHSExpr->getExprLoc(),
+ ) &&
+ Self.getSourceManager().isMacroArgExpansion(RHSExpr->getExprLoc(),
+ )) {
+if (OpExpansionLoc == LHSExpansionLoc && OpExpansionLoc ==
RHSExpansionLoc) {
+ DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
+ DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
+}
+ }
+}
}
if ((Opc == BO_Shl &&
LHSExpr->getType()->isIntegralType(Self.getASTContext()))
Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -14,6 +14,15 @@
if ((i = 4)) {}
}
+// testing macros
+// nesting macros testing
+#define NESTING_VOID(cond) ( (void)(cond) )
+#define NESTING_VOID_WRAPPER(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
+
+// non-nesting macros
+#define NON_NESTING_VOID_0(cond) ( (void)(cond) )
+#define NON_NESTING_VOID_1(op0, op1, x, y,
r334879 - Call CreateTempAllocaWithoutCast for ActiveFlag
Author: yaxunl
Date: Fri Jun 15 18:20:52 2018
New Revision: 334879
URL: http://llvm.org/viewvc/llvm-project?rev=334879=rev
Log:
Call CreateTempAllocaWithoutCast for ActiveFlag
This is partial re-commit of r332982.
Modified:
cfe/trunk/lib/CodeGen/CGCleanup.cpp
Modified: cfe/trunk/lib/CodeGen/CGCleanup.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCleanup.cpp?rev=334879=334878=334879=diff
==
--- cfe/trunk/lib/CodeGen/CGCleanup.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGCleanup.cpp Fri Jun 15 18:20:52 2018
@@ -283,8 +283,8 @@ void EHScopeStack::popNullFixups() {
void CodeGenFunction::initFullExprCleanup() {
// Create a variable to decide whether the cleanup needs to be run.
- Address active = CreateTempAlloca(Builder.getInt1Ty(), CharUnits::One(),
-"cleanup.cond");
+ Address active = CreateTempAllocaWithoutCast(
+ Builder.getInt1Ty(), CharUnits::One(), "cleanup.cond");
// Initialize it to false at a site that's guaranteed to be run
// before each evaluation.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.
JDevlieghere updated this revision to Diff 151596.
JDevlieghere marked 3 inline comments as done.
JDevlieghere added a comment.
- Feedback Adrian
https://reviews.llvm.org/D48241
Files:
clang/lib/CodeGen/CGDebugInfo.cpp
clang/lib/CodeGen/CGDebugInfo.h
clang/test/CodeGenObjC/debug-info-category.m
Index: clang/test/CodeGenObjC/debug-info-category.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/debug-info-category.m
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -dwarf-version=5 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF5
+// RUN: %clang_cc1 -dwarf-version=4 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF4
+
+@interface Foo {
+ int integer;
+}
+
+- (int)integer;
+- (id)integer:(int)_integer;
+@end
+
+@implementation Foo
+- (int)integer {
+ return integer;
+}
+
+- (id)integer:(int)_integer {
+ integer = _integer;
+ return self;
+}
+@end
+
+@interface Foo (Bar)
+- (id)add:(Foo *)addend;
+@end
+
+@implementation Foo (Bar)
+- (id)add:(Foo *)addend {
+ return [self integer:[self integer] + [addend integer]];
+}
+@end
+
+// CHECK: ![[STRUCT:.*]] = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo"
+
+// DWARF5: !DISubprogram(name: "-[Foo integer]", scope: ![[STRUCT]], {{.*}}isDefinition: false
+// DWARF5: !DISubprogram(name: "-[Foo integer:]", scope: ![[STRUCT]], {{.*}}isDefinition: false
+// DWARF5: !DISubprogram(name: "-[Foo(Bar) add:]", scope: ![[STRUCT]], {{.*}}isDefinition: false
+
+// DWARF4-NOT: !DISubprogram(name: "-[Foo integer]", scope: ![[STRUCT]], {{.*}}isDefinition: false
+// DWARF4-NOT: !DISubprogram(name: "-[Foo integer:]", scope: ![[STRUCT]], {{.*}}isDefinition: false
+// DWARF4-NOT: !DISubprogram(name: "-[Foo(Bar) add:]", scope: ![[STRUCT]], {{.*}}isDefinition: false
+
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer:]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo(Bar) add:]"{{.*}}isDefinition: true
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -98,6 +98,15 @@
/// Cache of previously constructed interfaces which may change.
llvm::SmallVector ObjCInterfaceCache;
+ struct ObjCMethodCacheEntry {
+const ObjCMethodDecl *MD;
+llvm::DISubprogram *DIMethodDecl;
+ };
+
+ /// Cache of forward declarations for methods belonging to the interface.
+ llvm::DenseMap>
+ ObjCMethodCache;
+
/// Cache of references to clang modules and precompiled headers.
llvm::DenseMap ModuleCache;
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3346,6 +3346,26 @@
if (HasDecl && isa(D))
DeclCache[D->getCanonicalDecl()].reset(SP);
+ if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Starting with DWARF V5 method declarations are emitted as children of
+// the interface type.
+if (const auto *OMD = dyn_cast_or_null(D)) {
+ const ObjCInterfaceDecl *ID = OMD->getClassInterface();
+ QualType QTy(ID->getTypeForDecl(), 0);
+ auto it = TypeCache.find(QTy.getAsOpaquePtr());
+ assert(it != TypeCache.end());
+ llvm::DICompositeType *InterfaceDecl =
+ cast(it->second);
+ llvm::DISubprogram *FD = DBuilder.createFunction(
+ InterfaceDecl, Name, LinkageName, Unit, LineNo,
+ getOrCreateFunctionType(D, FnType, Unit), Fn->hasLocalLinkage(),
+ false /*definition*/, ScopeLine, Flags, CGM.getLangOpts().Optimize,
+ TParamsArray.get());
+ DBuilder.finalizeSubprogram(FD);
+ ObjCMethodCache[ID].push_back({OMD, FD});
+}
+ }
+
// Push the function onto the lexical block stack.
LexicalBlockStack.emplace_back(SP);
@@ -4213,6 +4233,30 @@
DBuilder.replaceTemporary(llvm::TempDIType(E.Decl), Ty);
}
+ if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Add methods to interface.
+for (auto p : ObjCMethodCache) {
+ if (p.second.empty())
+continue;
+
+ QualType QTy(p.first->getTypeForDecl(), 0);
+ auto it = TypeCache.find(QTy.getAsOpaquePtr());
+ if (it == TypeCache.end())
+continue;
+
+ llvm::DICompositeType *InterfaceDecl =
+ cast(it->second);
+
+ SmallVector EltTys;
+ auto = InterfaceDecl->getElements();
+ EltTys.append(Elements.begin(), Elements.end());
+ for (auto : p.second)
+EltTys.push_back(M.DIMethodDecl);
+ llvm::DINodeArray Elements = DBuilder.getOrCreateArray(EltTys);
+ DBuilder.replaceArrays(InterfaceDecl, Elements);
+}
+ }
+
for (auto p : ReplaceMap) {
[PATCH] D37035: Implement __builtin_LINE() et. al. to support source location capture.
rsmith added inline comments.
Comment at: include/clang/AST/SourceLocExprScope.h:39
+//static_assert(foo() == __LINE__);
+return OldVal == nullptr || isa(DefaultExpr) ||
+ !OldVal->isInSameContext(EvalContext);
Do we want the `isa` check here? I'd think the same problem would occur for
default initializers:
```
struct A {
int n = __builtin_LINE();
};
struct B {
A a = {};
};
B b = {}; // should give this context as the current one, not the context of
the initializer inside struct B
```
Comment at: include/clang/AST/SourceLocExprScope.h:44
+public:
+ SourceLocExprScopeBase(const Expr *DefaultExpr, SourceLocExprScopeBase
**Dest,
+ EvalContextType *EvalContext)
I think it'd be cleaner to add a
```
struct Current {
SourceLocExprScopeBase *Scope;
};
```
maybe with members to get the location and context for a given `SourceLocExpr`,
and to pass in a `SourceLocExprScopeBase::Current &` here rather than a
`SourceLocExprScopeBase **`.
Comment at: include/clang/Sema/Sema.h:4463
+
+ /// \brief build a potentially resolved SourceLocExpr.
+ ///
Capitalize "build" here.
Comment at: include/clang/Serialization/ASTBitCodes.h:1643
+ /// A SourceLocExpr record
+ EXPR_SOURCE_LOC,
Missing period.
Comment at: lib/AST/Expr.cpp:1924-1931
+ auto CreateString = [&](StringRef SVal) {
+QualType StrTy = BuildSourceLocExprType(Ctx, getIdentType(),
+/*IsArray*/ true, SVal.size() + 1);
+StringLiteral *Lit = StringLiteral::Create(Ctx, SVal, StringLiteral::Ascii,
+ /*Pascal*/ false, StrTy, Loc);
+assert(Lit && "should not be null");
+return Lit;
It doesn't seem reasonable to create a new `StringLiteral` each time a
`SourceLocExpr` is (constant-)evaluated, and seems like it would be unsound in
some cases (we require that reevaluation produces the same result).
I think the `StringLiteral` here is just for convenience, right? (Instead we
could model the `SourceLocExpr` as being its own kind of array lvalue for the
purpose of constant expression evaluation, like we do for `ObjCEncodeExpr` for
instance.)
Comment at: lib/AST/ExprConstant.cpp:698-704
+/// \brief Source location information about the default argument
expression
+/// we're evaluating, if any.
+SourceLocExprScope *CurCXXDefaultArgScope = nullptr;
+
+/// \brief Source location information about the default member initializer
+/// we're evaluating, if any.
+SourceLocExprScope *CurCXXDefaultInitScope = nullptr;
Do we really need both of these? It would seem like one 'current' scope would
be sufficient to me.
Comment at: lib/AST/ExprConstant.cpp:7296
+bool IntExprEvaluator::VisitSourceLocExpr(const SourceLocExpr *E) {
+assert(E && E->isLineOrColumn());
+llvm::APInt Result;
Too much indentation.
Comment at: lib/CodeGen/CGExprAgg.cpp:192-199
+void VisitVAArgExpr(VAArgExpr * E);
- void EmitInitializationToLValue(Expr *E, LValue Address);
- void EmitNullInitializationToLValue(LValue Address);
- // case Expr::ChooseExprClass:
- void VisitCXXThrowExpr(const CXXThrowExpr *E) { CGF.EmitCXXThrowExpr(E); }
- void VisitAtomicExpr(AtomicExpr *E) {
-RValue Res = CGF.EmitAtomicExpr(E);
-EmitFinalDestCopy(E->getType(), Res);
- }
+void EmitInitializationToLValue(Expr * E, LValue Address);
+void EmitNullInitializationToLValue(LValue Address);
+// case Expr::ChooseExprClass:
+void VisitCXXThrowExpr(const CXXThrowExpr *E) { CGF.EmitCXXThrowExpr(E); }
+void VisitAtomicExpr(AtomicExpr * E) {
Something funny happened to the formatting here.
Comment at: lib/CodeGen/CGExprScalar.cpp:595
+} else {
+ StringLiteral *Str = SLE->getStringValue(Ctx, Loc, DC);
+ return CGF.EmitArrayToPointerDecay(Str).getPointer();
Should we make some attempt to reuse the same string literal for multiple
source locations denoting the same file?
https://reviews.llvm.org/D37035
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.
aprantl added inline comments.
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4250
+
+ SmallVector EltTys;
+ EltTys.append(InterfaceDecl->getElements().begin(),
```
auto = InterfaceDecl->getElements();
EltTys.append(Elements.begin(), Elements.end());
for (auto : p.second)
EltTys.push_back(M.DIMethodDecl);
```
Comment at: clang/lib/CodeGen/CGDebugInfo.h:106
+
+ /// Cache of forward declarations for method
+ llvm::DenseMap>
looks like that comment is cut off at the end?
Comment at: clang/test/CodeGenObjC/debug-info-category.m:41
+// DWARF4-NOT: = !DISubprogram(name: "-[Foo integer:]"{{.*}}isDefinition: false
+// DWARF4-NOT: = !DISubprogram(name: "-[Foo(Bar) add:]"{{.*}}isDefinition:
false
+
Shouldn't you also check the scope: of the DISubprograms?
https://reviews.llvm.org/D48241
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48242: [ASTMatchers] Add support for matching the type of a friend decl.
dlj added inline comments.
Comment at: include/clang/ASTMatchers/ASTMatchers.h:2900-2904
+AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
+hasType,
+AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl),
+internal::Matcher, InnerMatcher, 1) {
+ QualType QT = internal::getUnderlyingType(Node);
aaron.ballman wrote:
> I actually prefer the previous formatting, can you restore that?
Hmmm, it barely fits in 80 cols if I use the previous flow, but looks worse to
me (it adds another line just for FriendDecl). I've updated to exactly what
clang-format prefers, which happens to be fairly consistent with the other
declaration of hasType.
Comment at: include/clang/ASTMatchers/ASTMatchers.h:2904
+internal::Matcher, InnerMatcher, 1) {
+ QualType QT = internal::getUnderlyingType(Node);
+ if (!QT.isNull())
klimek wrote:
> In which cases can getUnderlyingType return a null type?
QT can be null for a FriendDecl that that doesn't name a type. That seems to
DTRT here.
Repository:
rC Clang
https://reviews.llvm.org/D48242
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48242: [ASTMatchers] Add support for matching the type of a friend decl.
dlj updated this revision to Diff 151593.
dlj marked 3 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D48242
Files:
include/clang/ASTMatchers/ASTMatchers.h
include/clang/ASTMatchers/ASTMatchersInternal.h
unittests/ASTMatchers/ASTMatchersNodeTest.cpp
Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -160,6 +160,16 @@
valueDecl(hasType(asString("void (void)");
}
+TEST(FriendDecl, Matches) {
+ EXPECT_TRUE(matches("class Y { friend class X; };",
+ friendDecl(hasType(asString("class X");
+ EXPECT_TRUE(matches("class Y { friend class X; };",
+ friendDecl(hasType(recordDecl(hasName("X"));
+
+ EXPECT_TRUE(matches("class Y { friend void f(); };",
+ functionDecl(hasName("f"), hasParent(friendDecl();
+}
+
TEST(Enum, DoesNotMatchClasses) {
EXPECT_TRUE(notMatches("class X {};", enumDecl(hasName("X";
}
Index: include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- include/clang/ASTMatchers/ASTMatchersInternal.h
+++ include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -38,6 +38,7 @@
#include "clang/AST/ASTTypeTraits.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclFriend.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
@@ -120,10 +121,14 @@
inline QualType getUnderlyingType(const ValueDecl ) {
return Node.getType();
}
-
inline QualType getUnderlyingType(const TypedefNameDecl ) {
return Node.getUnderlyingType();
}
+inline QualType getUnderlyingType(const FriendDecl ) {
+ if (const TypeSourceInfo *TSI = Node.getFriendType())
+return TSI->getType();
+ return QualType();
+}
/// Unifies obtaining the FunctionProtoType pointer from both
/// FunctionProtoType and FunctionDecl nodes..
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -2860,13 +2860,17 @@
/// Example matches x (matcher = expr(hasType(cxxRecordDecl(hasName("X")
/// and z (matcher = varDecl(hasType(cxxRecordDecl(hasName("X")
/// and U (matcher = typedefDecl(hasType(asString("int")))
+/// and friend class X (matcher = friendDecl(hasType("X"))
/// \code
/// class X {};
/// void y(X ) { x; X z; }
/// typedef int U;
+/// class Y { friend class X; };
/// \endcode
AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
-hasType, AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, TypedefNameDecl, ValueDecl),
+hasType,
+AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, TypedefNameDecl,
+ValueDecl),
internal::Matcher, InnerMatcher, 0) {
QualType QT = internal::getUnderlyingType(Node);
if (!QT.isNull())
@@ -2885,18 +2889,21 @@
///
/// Example matches x (matcher = expr(hasType(cxxRecordDecl(hasName("X")
/// and z (matcher = varDecl(hasType(cxxRecordDecl(hasName("X")
+/// and friend class X (matcher = friendDecl(hasType("X"))
/// \code
/// class X {};
/// void y(X ) { x; X z; }
+/// class Y { friend class X; };
/// \endcode
///
/// Usable as: Matcher, Matcher
-AST_POLYMORPHIC_MATCHER_P_OVERLOAD(hasType,
- AST_POLYMORPHIC_SUPPORTED_TYPES(Expr,
- ValueDecl),
- internal::Matcher, InnerMatcher, 1) {
- return qualType(hasDeclaration(InnerMatcher))
- .matches(Node.getType(), Finder, Builder);
+AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
+hasType, AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl),
+internal::Matcher, InnerMatcher, 1) {
+ QualType QT = internal::getUnderlyingType(Node);
+ if (!QT.isNull())
+return qualType(hasDeclaration(InnerMatcher)).matches(QT, Finder, Builder);
+ return false;
}
/// Matches if the type location of the declarator decl's type matches
Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -160,6 +160,16 @@
valueDecl(hasType(asString("void (void)");
}
+TEST(FriendDecl, Matches) {
+ EXPECT_TRUE(matches("class Y { friend class X; };",
+ friendDecl(hasType(asString("class X");
+ EXPECT_TRUE(matches("class Y { friend class X; };",
+ friendDecl(hasType(recordDecl(hasName("X"));
+
+ EXPECT_TRUE(matches("class Y { friend void f(); };",
+
[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.
JDevlieghere updated this revision to Diff 151591.
JDevlieghere added a comment.
- Use type cache &
- Simplify method cache struct
- Add custom test that verifies category methods are emitted
https://reviews.llvm.org/D48241
Files:
clang/lib/CodeGen/CGDebugInfo.cpp
clang/lib/CodeGen/CGDebugInfo.h
clang/test/CodeGenObjC/debug-info-category.m
Index: clang/test/CodeGenObjC/debug-info-category.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/debug-info-category.m
@@ -0,0 +1,45 @@
+// RUN: %clang_cc1 -dwarf-version=5 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF5
+// RUN: %clang_cc1 -dwarf-version=4 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF4
+
+@interface Foo {
+ int integer;
+}
+
+- (int)integer;
+- (id)integer:(int)_integer;
+@end
+
+@implementation Foo
+- (int)integer {
+ return integer;
+}
+
+- (id)integer:(int)_integer {
+ integer = _integer;
+ return self;
+}
+@end
+
+@interface Foo (Bar)
+- (id)add:(Foo *)addend;
+@end
+
+@implementation Foo (Bar)
+- (id)add:(Foo *)addend {
+ return [self integer:[self integer] + [addend integer]];
+}
+@end
+
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Foo"
+
+// DWARF5: = !DISubprogram(name: "-[Foo integer]"{{.*}}isDefinition: false
+// DWARF5: = !DISubprogram(name: "-[Foo integer:]"{{.*}}isDefinition: false
+// DWARF5: = !DISubprogram(name: "-[Foo(Bar) add:]"{{.*}}isDefinition: false
+
+// DWARF4-NOT: = !DISubprogram(name: "-[Foo integer]"{{.*}}isDefinition: false
+// DWARF4-NOT: = !DISubprogram(name: "-[Foo integer:]"{{.*}}isDefinition: false
+// DWARF4-NOT: = !DISubprogram(name: "-[Foo(Bar) add:]"{{.*}}isDefinition: false
+
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo integer:]"{{.*}}isDefinition: true
+// CHECK: = distinct !DISubprogram(name: "-[Foo(Bar) add:]"{{.*}}isDefinition: true
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -98,6 +98,15 @@
/// Cache of previously constructed interfaces which may change.
llvm::SmallVector ObjCInterfaceCache;
+ struct ObjCMethodCacheEntry {
+const ObjCMethodDecl *MD;
+llvm::DISubprogram *DIMethodDecl;
+ };
+
+ /// Cache of forward declarations for method
+ llvm::DenseMap>
+ ObjCMethodCache;
+
/// Cache of references to clang modules and precompiled headers.
llvm::DenseMap ModuleCache;
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3346,6 +3346,26 @@
if (HasDecl && isa(D))
DeclCache[D->getCanonicalDecl()].reset(SP);
+ if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Starting with DWARF V5 method declarations are emitted as children of
+// the interface type.
+if (const auto *OMD = dyn_cast_or_null(D)) {
+ const ObjCInterfaceDecl *ID = OMD->getClassInterface();
+ QualType QTy(ID->getTypeForDecl(), 0);
+ auto it = TypeCache.find(QTy.getAsOpaquePtr());
+ assert(it != TypeCache.end());
+ llvm::DICompositeType *InterfaceDecl =
+ cast(it->second);
+ llvm::DISubprogram *FD = DBuilder.createFunction(
+ InterfaceDecl, Name, LinkageName, Unit, LineNo,
+ getOrCreateFunctionType(D, FnType, Unit), Fn->hasLocalLinkage(),
+ false /*definition*/, ScopeLine, Flags, CGM.getLangOpts().Optimize,
+ TParamsArray.get());
+ DBuilder.finalizeSubprogram(FD);
+ ObjCMethodCache[ID].push_back({OMD, FD});
+}
+ }
+
// Push the function onto the lexical block stack.
LexicalBlockStack.emplace_back(SP);
@@ -4213,6 +4233,31 @@
DBuilder.replaceTemporary(llvm::TempDIType(E.Decl), Ty);
}
+ if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Add methods to interface.
+for (auto p : ObjCMethodCache) {
+ if (p.second.empty())
+continue;
+
+ QualType QTy(p.first->getTypeForDecl(), 0);
+ auto it = TypeCache.find(QTy.getAsOpaquePtr());
+ if (it == TypeCache.end())
+continue;
+
+ llvm::DICompositeType *InterfaceDecl =
+ cast(it->second);
+
+ SmallVector EltTys;
+ EltTys.append(InterfaceDecl->getElements().begin(),
+InterfaceDecl->getElements().end());
+ for (auto : p.second) {
+EltTys.push_back(M.DIMethodDecl);
+ }
+ llvm::DINodeArray Elements = DBuilder.getOrCreateArray(EltTys);
+ DBuilder.replaceArrays(InterfaceDecl, Elements);
+}
+ }
+
for (auto p : ReplaceMap) {
assert(p.second);
auto *Ty = cast(p.second);
[PATCH] D48242: [ASTMatchers] Add support for matching the type of a friend decl.
klimek added inline comments.
Comment at: include/clang/ASTMatchers/ASTMatchers.h:2904
+internal::Matcher, InnerMatcher, 1) {
+ QualType QT = internal::getUnderlyingType(Node);
+ if (!QT.isNull())
In which cases can getUnderlyingType return a null type?
Repository:
rC Clang
https://reviews.llvm.org/D48242
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48242: [ASTMatchers] Add support for matching the type of a friend decl.
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM aside from some minor nits.
Comment at: include/clang/ASTMatchers/ASTMatchers.h:2900-2904
+AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
+hasType,
+AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl),
+internal::Matcher, InnerMatcher, 1) {
+ QualType QT = internal::getUnderlyingType(Node);
I actually prefer the previous formatting, can you restore that?
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:128
+inline QualType getUnderlyingType(const FriendDecl ) {
+ if (const TypeSourceInfo *tsi = Node.getFriendType())
+return tsi->getType();
tsi -> TSI
Repository:
rC Clang
https://reviews.llvm.org/D48242
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.
JDevlieghere added a comment.
In https://reviews.llvm.org/D48241#1134240, @dblaikie wrote:
> Not sure I should get too involved in this discussion, knowing so little
> about Objective C - but if it seems sensible, could you provide some examples
> (just in comments/description in this code review) of what the DWARF used to
> look like and what it looks like after this change?
After this change we emit a forward declaration as a child of the
DW_TAG_structure_type. The definition (below) remains unchanged.
0x0027: DW_TAG_structure_type
DW_AT_APPLE_objc_complete_type (true)
DW_AT_name ()
DW_AT_byte_size (0x04)
DW_AT_decl_file ("cat.m")
DW_AT_decl_line (1)
DW_AT_APPLE_runtime_class (0x10)
0x002d: DW_TAG_member
DW_AT_name()
DW_AT_type(0x007b "base ")
DW_AT_decl_file ("cat.m")
DW_AT_decl_line (2)
DW_AT_data_member_location(0x00)
DW_AT_accessibility (DW_ACCESS_protected)
0x0037: DW_TAG_subprogram
DW_AT_name()
DW_AT_decl_file ("cat.m")
DW_AT_decl_line (10)
DW_AT_prototyped (true)
DW_AT_type(0x007b "base ")
DW_AT_declaration (true)
0x003f: DW_TAG_formal_parameter
DW_AT_type (0x007f "structure *")
DW_AT_artificial(true)
0x0044: DW_TAG_formal_parameter
DW_AT_type (0x0084 "structure *")
DW_AT_artificial(true)
0x0049: NULL
...
0x007a: NULL
...
0x00b5: DW_TAG_subprogram
DW_AT_low_pc(0x)
DW_AT_high_pc (0x001c)
DW_AT_frame_base(DW_OP_reg6 RBP)
DW_AT_object_pointer(0x00cf)
DW_AT_name ()
DW_AT_decl_file ("cat.m")
DW_AT_decl_line (10)
DW_AT_prototyped(true)
DW_AT_type (0x007b "base ")
0x00cf: DW_TAG_formal_parameter
DW_AT_location(DW_OP_fbreg -8)
DW_AT_name()
DW_AT_type(0x00b0 "structure *")
DW_AT_artificial (true)
0x00d8: DW_TAG_formal_parameter
DW_AT_location(DW_OP_fbreg -16)
DW_AT_name()
DW_AT_type(0x0195 "structure *")
DW_AT_artificial (true)
0x00e1: NULL
> Does this address the discoverability that's being discussed in the llvm-dev
> thread? There were concerns there about having to search through all the
> instances of a type to find all of its functions - I imagine, since Objective
> C classes aren't closed (if I understand correctly) that would still be a
> concern here? If it is, what is the benefit/tradeoff of this change (if it
> doesn't address the discoverability/still requires the Objective C
> accelerator table)?
Following this approach we have the exact same problem (and I believe we could
use the same solution). The motivation for this change is that there's no clean
way to implement the .apple_objc accelerator table using the debug_names,
because it's conceptually different from the other tables. It maps an interface
name to the DIEs of its methods, as opposed to all the other tables that map a
name to their corresponding DIE. The only way this could work is as a separate
table, because otherwise you'd have to ensure, for every entry, if it's a
"regular" one, which obviously violates the standard. Putting it in a separate
column is also a bad idea, because that means the column is present for all the
entries, including the ones that don't need it.
Comment at: clang/lib/CodeGen/CGDebugInfo.h:111
+// methods.
+llvm::DICompositeType *DIInterfaceDecl;
+std::vector Methods;
aprantl wrote:
> Isn't the interface already the key in the DenseMap?
That's the `ObjCInterfaceDecl` while this is the `DICompositeType`.
https://reviews.llvm.org/D48241
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48249: [analyzer] Add stubs for argument construction contexts for arguments of C++ constructors and Objective-C messages.
NoQ updated this revision to Diff 151584.
NoQ added a comment.
Whoops, that was an old patch.
https://reviews.llvm.org/D48249
Files:
lib/Analysis/CFG.cpp
lib/Analysis/ConstructionContext.cpp
test/Analysis/cfg-rich-constructors.cpp
test/Analysis/cfg-rich-constructors.mm
test/Analysis/temporaries.cpp
test/Analysis/temporaries.mm
Index: test/Analysis/temporaries.mm
===
--- /dev/null
+++ test/Analysis/temporaries.mm
@@ -0,0 +1,23 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker core,cplusplus -verify %s
+
+// expected-no-diagnostics
+
+// Stripped down unique_ptr
+struct IntPtr {
+ IntPtr(): i(new int) {}
+ IntPtr(IntPtr &): i(o.i) { o.i = nullptr; }
+ ~IntPtr() { delete i; }
+
+ int *i;
+};
+
+@interface Foo {}
+ -(void) foo: (IntPtr)arg;
+@end
+
+void bar(Foo *f) {
+ IntPtr ptr;
+ int *i = ptr.i;
+ [f foo: static_cast(ptr)];
+ *i = 99; // no-warning
+}
Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++11
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++17
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++11
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -DTEMPORARY_DTORS -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++17
// Note: The C++17 run-line doesn't -verify yet - it is a no-crash test.
@@ -945,3 +945,29 @@
const C () { return foo1(); } // no-crash
C &() { return foo2(); } // no-crash
} // end namespace pass_references_through
+
+
+namespace ctor_argument {
+// Stripped down unique_ptr
+struct IntPtr {
+ IntPtr(): i(new int) {}
+ IntPtr(IntPtr &): i(o.i) { o.i = 0; }
+ ~IntPtr() { delete i; }
+
+ int *i;
+};
+
+struct Foo {
+ Foo(IntPtr);
+ void bar();
+
+ IntPtr i;
+};
+
+void bar() {
+ IntPtr ptr;
+ int *i = ptr.i;
+ Foo f(static_cast(ptr));
+ *i = 99; // no-warning
+}
+} // namespace ctor_argument
Index: test/Analysis/cfg-rich-constructors.mm
===
--- /dev/null
+++ test/Analysis/cfg-rich-constructors.mm
@@ -0,0 +1,41 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -triple x86_64-apple-darwin12 -std=c++11 -w %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX11,ELIDE,CXX11-ELIDE %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -triple x86_64-apple-darwin12 -std=c++17 -w %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX17,ELIDE,CXX17-ELIDE %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -triple x86_64-apple-darwin12 -std=c++11 -w -analyzer-config elide-constructors=false %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX11,NOELIDE,CXX11-NOELIDE %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.DumpCFG -triple x86_64-apple-darwin12 -std=c++17 -w -analyzer-config elide-constructors=false %s > %t 2>&1
+// RUN: FileCheck --input-file=%t -check-prefixes=CHECK,CXX17,NOELIDE,CXX17-NOELIDE %s
+
+class D {
+public:
+ D();
+ ~D();
+};
+
+@interface E {}
+-(void) foo: (D) d;
+@end
+
+// FIXME: Find construction context for the argument.
+// CHECK: void passArgumentIntoMessage(E *e)
+// CHECK: 1: e
+// CHECK-NEXT: 2: [B1.1] (ImplicitCastExpr, LValueToRValue, E *)
+// CXX11-NEXT: 3: D() (CXXConstructExpr, [B1.4], [B1.6], class D)
+// CXX11-NEXT: 4: [B1.3] (BindTemporary)
+// CXX11-NEXT: 5: [B1.4] (ImplicitCastExpr, NoOp, const class D)
+// CXX11-NEXT: 6: [B1.5]
+// CXX11-NEXT: 7: [B1.6] (CXXConstructExpr, class D)
+// CXX11-NEXT: 8: [B1.7] (BindTemporary)
+// Double brackets trigger FileCheck variables, escape.
+// CXX11-NEXT: 9: {{\[}}[B1.2] foo:[B1.8]]
+// CXX11-NEXT:10: ~D() (Temporary object destructor)
+// CXX11-NEXT:11: ~D() (Temporary object destructor)
+//
[PATCH] D48249: [analyzer] Add stubs for argument construction contexts for arguments of C++ constructors and Objective-C messages.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, mikhail.ramalho, baloghadamsoftware.
This is similar to https://reviews.llvm.org/D45650. Constructors of
pass-by-value arguments aren't similar to other temporary constructors, we
should treat them differently. For now we don't handle them, so we shouldn't
produce temporary-like construction contexts for them.
https://reviews.llvm.org/D45650 fixed this bug for call-expressions, but
construct-expressions and Objective-C message expressions both aren't
call-expressions, so they need a separate fix.
Repository:
rC Clang
https://reviews.llvm.org/D48249
Files:
lib/Analysis/CFG.cpp
lib/Analysis/ConstructionContext.cpp
test/Analysis/temporaries.cpp
test/Analysis/temporaries.mm
Index: test/Analysis/temporaries.mm
===
--- /dev/null
+++ test/Analysis/temporaries.mm
@@ -0,0 +1,23 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker core,cplusplus -verify %s
+
+// expected-no-diagnostics
+
+// Stripped down unique_ptr
+struct IntPtr {
+ IntPtr(): i(new int) {}
+ IntPtr(IntPtr &): i(o.i) { o.i = nullptr; }
+ ~IntPtr() { delete i; }
+
+ int *i;
+};
+
+@interface Foo {}
+ -(void) foo: (IntPtr)arg;
+@end
+
+void bar(Foo *f) {
+ IntPtr ptr;
+ int *i = ptr.i;
+ [f foo: static_cast(ptr)];
+ *i = 99; // no-warning
+}
Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++11
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -DTEMPORARY_DTORS -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++17
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++03 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -analyzer-config cfg-temporary-dtors=false -verify -w -std=c++11 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -DTEMPORARY_DTORS -verify -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++11
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,debug.ExprInspection -DTEMPORARY_DTORS -w -analyzer-config cfg-temporary-dtors=true,c++-temp-dtor-inlining=true %s -std=c++17
// Note: The C++17 run-line doesn't -verify yet - it is a no-crash test.
@@ -945,3 +945,28 @@
const C () { return foo1(); } // no-crash
C &() { return foo2(); } // no-crash
} // end namespace pass_references_through
+
+
+namespace ctor_argument {
+struct IntPtr {
+ IntPtr(): i(new int) {}
+ IntPtr(IntPtr &): i(o.i) { o.i = 0; }
+ ~IntPtr() { delete i; }
+
+ int *i;
+};
+
+struct Foo {
+ Foo(IntPtr);
+ void bar();
+
+ IntPtr i;
+};
+
+void bar() {
+ IntPtr ptr;
+ int *i = ptr.i;
+ Foo f(static_cast(ptr));
+ *i = 99; // no-warning
+}
+} // namespace ctor_argument
Index: lib/Analysis/ConstructionContext.cpp
===
--- lib/Analysis/ConstructionContext.cpp
+++ lib/Analysis/ConstructionContext.cpp
@@ -15,6 +15,7 @@
//===--===//
#include "clang/Analysis/ConstructionContext.h"
+#include "clang/AST/ExprObjC.h"
using namespace clang;
@@ -111,7 +112,9 @@
assert(ParentLayer->isLast());
// This is a constructor into a function argument. Not implemented yet.
-if (isa(ParentLayer->getTriggerStmt()))
+if (isa(ParentLayer->getTriggerStmt()) ||
+isa(ParentLayer->getTriggerStmt()) ||
+isa(ParentLayer->getTriggerStmt()))
return nullptr;
// This is C++17 copy-elided construction into return statement.
if (auto *RS = dyn_cast(ParentLayer->getTriggerStmt())) {
@@ -173,7 +176,9 @@
return create(C, RS);
}
// This is a constructor into a function argument. Not implemented yet.
-if (isa(TopLayer->getTriggerStmt()))
+if (isa(TopLayer->getTriggerStmt()) ||
+isa(TopLayer->getTriggerStmt()) ||
+isa(TopLayer->getTriggerStmt()))
return nullptr;
llvm_unreachable("Unexpected construction context with statement!");
} else if (const CXXCtorInitializer *I = TopLayer->getTriggerInit()) {
Index: lib/Analysis/CFG.cpp
[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.
aprantl added a comment. > could you provide some examples (just in comments/description in this code > review) of what the DWARF used to look like and what it looks like after this > change? Thus far an Objective-C interface is a DW_TAG_structure_type that only has variables and properties as members. Objective-C methods are emitted as freestanding DW_TAG_subprogram function definitions. In addition, the .apple_objc accelerator table provides a mapping from ClassName -> [list of subprogram DIEs for each method in Class or one of its categories]. The idea behind this patch is to get rid of the accelerator table and encode the same information in the DIE hierarchy instead. LLDB was never using the accelerator table to accelerate lookup but only when it was in the middle of parsing a DW_TAG_structure_type of an Objective-C interface, so this shouldn't even come add a performance penalty. https://reviews.llvm.org/D48241 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r334870 - [docs] -fsanitize=cfi only allowed with -fvisibility=
Author: maskray Date: Fri Jun 15 16:11:18 2018 New Revision: 334870 URL: http://llvm.org/viewvc/llvm-project?rev=334870=rev Log: [docs] -fsanitize=cfi only allowed with -fvisibility= Modified: cfe/trunk/docs/SanitizerStats.rst Modified: cfe/trunk/docs/SanitizerStats.rst URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/SanitizerStats.rst?rev=334870=334869=334870=diff == --- cfe/trunk/docs/SanitizerStats.rst (original) +++ cfe/trunk/docs/SanitizerStats.rst Fri Jun 15 16:11:18 2018 @@ -56,7 +56,7 @@ Example: 10 A a; 11 g(); 12 } -$ clang++ -fsanitize=cfi -flto -fuse-ld=gold vcall.cc -fsanitize-stats -g +$ clang++ -fsanitize=cfi -fvisibility=hidden -flto -fuse-ld=gold vcall.cc -fsanitize-stats -g $ SANITIZER_STATS_PATH=a.stats ./a.out $ sanstats a.stats vcall.cc:6 _Z1gP1A cfi-vcall 1 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.
JDevlieghere updated this revision to Diff 151580.
JDevlieghere marked 5 inline comments as done.
JDevlieghere added a comment.
CR feedback
https://reviews.llvm.org/D48241
Files:
clang/lib/CodeGen/CGDebugInfo.cpp
clang/lib/CodeGen/CGDebugInfo.h
clang/test/CodeGenObjC/debug-info-synthesis.m
Index: clang/test/CodeGenObjC/debug-info-synthesis.m
===
--- clang/test/CodeGenObjC/debug-info-synthesis.m
+++ clang/test/CodeGenObjC/debug-info-synthesis.m
@@ -1,4 +1,7 @@
// RUN: %clang_cc1 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s
+// RUN: %clang_cc1 -dwarf-version=5 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF5
+// RUN: %clang_cc1 -dwarf-version=4 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF4
+
# 1 "foo.m" 1
# 1 "foo.m" 2
# 1 "./foo.h" 1
@@ -30,8 +33,14 @@
}
}
+// CHECK: ![[STRUCT:.*]] = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo"
// CHECK: ![[FILE:.*]] = !DIFile(filename: "{{[^"]+}}foo.h"
-// CHECK: !DISubprogram(name: "-[Foo setDict:]"
+// DWARF5: !DISubprogram(name: "-[Foo setDict:]"
+// DWARF5-SAME: scope: ![[STRUCT]],
+// DWARF5-SAME: line: 8,
+// DWARF5-SAME: isLocal: true, isDefinition: false
+// DWARF4-NOT: !DISubprogram(name: "-[Foo setDict:]"{{.*}}isDefinition: false
+// CHECK: distinct !DISubprogram(name: "-[Foo setDict:]"
// CHECK-SAME: file: ![[FILE]],
// CHECK-SAME: line: 8,
// CHECK-SAME: isLocal: true, isDefinition: true
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -98,6 +98,25 @@
/// Cache of previously constructed interfaces which may change.
llvm::SmallVector ObjCInterfaceCache;
+ struct ObjCMethodCacheEntry {
+struct MethodData {
+ const ObjCMethodDecl *MD;
+ llvm::DISubprogram *DIMethodDecl;
+};
+
+// Keep track of the interface so we can update its elements with its
+// methods.
+llvm::DICompositeType *DIInterfaceDecl;
+std::vector Methods;
+
+ObjCMethodCacheEntry(llvm::DICompositeType *DIInterfaceDecl = nullptr)
+: DIInterfaceDecl(DIInterfaceDecl) {}
+ };
+
+ /// Cache of forward declarations for method
+ llvm::DenseMap
+ ObjCMethodCache;
+
/// Cache of references to clang modules and precompiled headers.
llvm::DenseMap ModuleCache;
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2231,6 +2231,13 @@
Mod ? Mod : Unit, ID->getName(), DefUnit, Line, Size, Align, Flags,
nullptr, llvm::DINodeArray(), RuntimeLang);
+ if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Remember the DICompositeType in the ObjCMethodCache.
+assert(ObjCMethodCache[ID].DIInterfaceDecl == nullptr ||
+ ObjCMethodCache[ID].DIInterfaceDecl == RealDecl);
+ObjCMethodCache[ID].DIInterfaceDecl = RealDecl;
+ }
+
QualType QTy(Ty, 0);
TypeCache[QTy.getAsOpaquePtr()].reset(RealDecl);
@@ -3346,6 +3353,21 @@
if (HasDecl && isa(D))
DeclCache[D->getCanonicalDecl()].reset(SP);
+ if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Starting with DWARF V5 method declarations are emitted as children of
+// the interface type.
+if (const auto *OMD = dyn_cast_or_null(D)) {
+ const ObjCInterfaceDecl *ID = OMD->getClassInterface();
+ llvm::DISubprogram *FD = DBuilder.createFunction(
+ ObjCMethodCache[ID].DIInterfaceDecl, Name, LinkageName, Unit, LineNo,
+ getOrCreateFunctionType(D, FnType, Unit), Fn->hasLocalLinkage(),
+ false /*definition*/, ScopeLine, Flags, CGM.getLangOpts().Optimize,
+ TParamsArray.get());
+ DBuilder.finalizeSubprogram(FD);
+ ObjCMethodCache[ID].Methods.push_back({OMD, FD});
+}
+ }
+
// Push the function onto the lexical block stack.
LexicalBlockStack.emplace_back(SP);
@@ -4213,6 +4235,26 @@
DBuilder.replaceTemporary(llvm::TempDIType(E.Decl), Ty);
}
+ if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Add methods to interface.
+for (auto p : ObjCMethodCache) {
+ llvm::DICompositeType *RealDecl = p.second.DIInterfaceDecl;
+ if (!RealDecl)
+continue;
+ if (p.second.Methods.empty())
+continue;
+
+ SmallVector EltTys;
+ EltTys.append(RealDecl->getElements().begin(),
+RealDecl->getElements().end());
+ for (auto : p.second.Methods) {
+EltTys.push_back(M.DIMethodDecl);
+ }
+ llvm::DINodeArray Elements = DBuilder.getOrCreateArray(EltTys);
+ DBuilder.replaceArrays(RealDecl,
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1623 + const CXXRecordDecl *SourceClassDecl = + E->getType().getTypePtr()->getPointeeCXXRecordDecl(); + const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl(); Prazek wrote: > rjmccall wrote: > > Unnecessary `getTypePtr()`. > getType returns QualType and it does not have getPointeeCXXRecordDecl. Am I > missing something? `operator->` on `QualType` drills to the `Type*`. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1647 + } +} + Prazek wrote: > rjmccall wrote: > > Incidentally, how do you protect against code like this? > > > > A *ptr; > > reinterpret_cast(ptr) = new B(); > > ptr->foo(); > > > > Presumably there needs to be a launder/strip here, but I guess it would > > have to be introduced by the middle-end when forwarding the store? The way > > I've written this is an aliasing violation, but (1) I assume your pass > > isn't disabled whenever strict-aliasing is disabled and (2) you can do this > > with a memcpy and still pretty reliably expect that LLVM will be able to > > eventually forward the store. > Can you add more info on what is A and B so I can make sure I understand it > correctly? > Is the prefix of the layout the same for both, but they are not in the > hierarchy? > > I haven't thought about the strict aliasing. I think the only sane way would > be to require strict aliasing for the strict vtable pointers. It's whatever case you're worried about such that you have to launder member accesses and bitcasts. And like I mentioned, relying on strict aliasing isn't enough because you can do it legally with memcpy. Maybe it's okay to consider it UB? I'm not sure about that. Repository: rL LLVM https://reviews.llvm.org/D47299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek marked an inline comment as done. Prazek added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1624 + E->getType().getTypePtr()->getPointeeCXXRecordDecl(); + const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl(); + rjmccall wrote: > The opposite of `Dst` is `Src`. Alternatively, the opposite of `Source` is > `Destination` (or `Result`). Please pick. Not sure if it still holds (if the DestTy should be DstTy, but I haven't declared it myself) Repository: rL LLVM https://reviews.llvm.org/D47299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek added inline comments.
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1633
+ bool DstMayNotBeDynamic =
+ !DstClassDecl || DstClassDecl->mayBeNotDynamicClass();
+ if (SourceMayBeNotDynamic && DstMayBeDynamic) {
rjmccall wrote:
> If you made a couple of tiny helper functions here that you could invoke on
> either `SourceClassDecl` or `DstClassDecl`, you could avoid some redundant
> logic and also make the calls self-documenting enough to legibly inline into
> the `if`-conditions.
>
> ...in fact, since you start from a QualType in every case, maybe you should
> just define the helper as a method there.
oh yea, it is definitely better!
Repository:
rL LLVM
https://reviews.llvm.org/D47299
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek updated this revision to Diff 151575.
Prazek added a comment.
Refactor
Repository:
rL LLVM
https://reviews.llvm.org/D47299
Files:
clang/include/clang/AST/DeclCXX.h
clang/include/clang/AST/Type.h
clang/lib/AST/Type.cpp
clang/lib/CodeGen/CGExpr.cpp
clang/lib/CodeGen/CGExprScalar.cpp
clang/test/CodeGenCXX/strict-vtable-pointers.cpp
Index: clang/test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- clang/test/CodeGenCXX/strict-vtable-pointers.cpp
+++ clang/test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -5,7 +5,8 @@
// RUN: FileCheck --check-prefix=CHECK-LINK-REQ %s < %t.ll
typedef __typeof__(sizeof(0)) size_t;
-void *operator new(size_t, void*) throw();
+void *operator new(size_t, void *) throw();
+using uintptr_t = unsigned long long;
struct NotTrivialDtor {
~NotTrivialDtor();
@@ -17,7 +18,7 @@
};
struct DynamicDerived : DynamicBase1 {
- void foo();
+ void foo() override;
};
struct DynamicBase2 {
@@ -28,8 +29,8 @@
};
struct DynamicDerivedMultiple : DynamicBase1, DynamicBase2 {
- virtual void foo();
- virtual void bar();
+ void foo() override;
+ void bar() override;
};
struct StaticBase {
@@ -47,9 +48,8 @@
struct DynamicFromVirtualStatic2 : virtual StaticBase {
};
-struct DynamicFrom2Virtuals :
-DynamicFromVirtualStatic1,
-DynamicFromVirtualStatic2 {
+struct DynamicFrom2Virtuals : DynamicFromVirtualStatic1,
+ DynamicFromVirtualStatic2 {
};
// CHECK-NEW-LABEL: define void @_Z12LocalObjectsv()
@@ -89,7 +89,6 @@
// CHECK-CTORS: call i8* @llvm.launder.invariant.group.p0i8(
// CHECK-CTORS-LABEL: {{^}}}
-
// CHECK-NEW-LABEL: define void @_Z9Pointers1v()
// CHECK-NEW-NOT: @llvm.launder.invariant.group.p0i8(
// CHECK-NEW-LABEL: call void @_ZN12DynamicBase1C1Ev(
@@ -134,7 +133,6 @@
// CHECK-CTORS-NOT: call i8* @llvm.launder.invariant.group.p0i8(
// CHECK-CTORS-LABEL: {{^}}}
-
struct DynamicDerived;
// CHECK-CTORS-LABEL: define linkonce_odr void @_ZN14DynamicDerivedC2Ev(
@@ -164,14 +162,12 @@
// CHECK-CTORS: call void @_ZN12DynamicBase2C2Ev(
// CHECK-CTORS-NOT: @llvm.launder.invariant.group.p0i8
-
// CHECK-CTORS: %[[THIS10:.*]] = bitcast %struct.DynamicDerivedMultiple* %[[THIS0]] to i32 (...)***
// CHECK-CTORS: store {{.*}} @_ZTV22DynamicDerivedMultiple, i32 0, inrange i32 0, i32 2) {{.*}} %[[THIS10]]
// CHECK-CTORS: %[[THIS11:.*]] = bitcast %struct.DynamicDerivedMultiple* %[[THIS0]] to i8*
// CHECK-CTORS: %[[THIS_ADD:.*]] = getelementptr inbounds i8, i8* %[[THIS11]], i64 16
// CHECK-CTORS: %[[THIS12:.*]] = bitcast i8* %[[THIS_ADD]] to i32 (...)***
-
// CHECK-CTORS: store {{.*}} @_ZTV22DynamicDerivedMultiple, i32 0, inrange i32 1, i32 2) {{.*}} %[[THIS12]]
// CHECK-CTORS-LABEL: {{^}}}
@@ -182,9 +178,10 @@
struct A {
virtual void foo();
+ int m;
};
struct B : A {
- virtual void foo();
+ void foo() override;
};
union U {
@@ -209,7 +206,7 @@
// CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
// CHECK-NEW: call void @_Z2g2P1A(%struct.A*
g2(>b);
- // CHECK-NEW: call void @_Z9changeToAP1U(%union.U*
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U*
changeToA(u);
// CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
// call void @_Z2g2P1A(%struct.A* %a)
@@ -294,21 +291,287 @@
take(u.v3);
}
+// CHECK-NEW-LABEL: define void @_Z7comparev()
+void compare() {
+ A *a = new A;
+ a->foo();
+ // CHECK-NEW: call i8* @llvm.launder.invariant.group.p0i8(i8*
+ A *b = new (a) B;
+
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+ // CHECK-NEW: %cmp = icmp eq %struct.A* %[[a2]], %[[b2]]
+ if (a == b)
+b->foo();
+}
+
+// CHECK-NEW-LABEL: compare2
+bool compare2(A *a, A *a2) {
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.strip.invariant.group.p0i8(i8*
+ // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+ // CHECK-NEW: %cmp = icmp ult %struct.A* %[[a2]], %[[b2]]
+ return a < a2;
+}
+// CHECK-NEW-LABEL: compareIntPointers
+bool compareIntPointers(int *a, int *b) {
+ // CHECK-NEW-NOT: call i8* @llvm.strip.invariant.group
+ return a == b;
+}
+
+struct HoldingOtherVirtuals {
+ B b;
+};
+
+// There is no need to add barriers for comparision of pointer to classes
+// that are not dynamic.
+// CHECK-NEW-LABEL: compare5
+bool compare5(HoldingOtherVirtuals *a, HoldingOtherVirtuals *b) {
+ // CHECK-NEW-NOT: call i8* @llvm.strip.invariant.group
+ return a == b;
+}
+// CHECK-NEW-LABEL: compareNull
+bool compareNull(A *a) {
+ // CHECK-NEW-NOT: call i8* @llvm.strip.invariant.group
+
+ if (a != nullptr)
[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers
Prazek marked 5 inline comments as done. Prazek added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1623 + const CXXRecordDecl *SourceClassDecl = + E->getType().getTypePtr()->getPointeeCXXRecordDecl(); + const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl(); rjmccall wrote: > Unnecessary `getTypePtr()`. getType returns QualType and it does not have getPointeeCXXRecordDecl. Am I missing something? Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1647 + } +} + rjmccall wrote: > Incidentally, how do you protect against code like this? > > A *ptr; > reinterpret_cast(ptr) = new B(); > ptr->foo(); > > Presumably there needs to be a launder/strip here, but I guess it would have > to be introduced by the middle-end when forwarding the store? The way I've > written this is an aliasing violation, but (1) I assume your pass isn't > disabled whenever strict-aliasing is disabled and (2) you can do this with a > memcpy and still pretty reliably expect that LLVM will be able to eventually > forward the store. Can you add more info on what is A and B so I can make sure I understand it correctly? Is the prefix of the layout the same for both, but they are not in the hierarchy? I haven't thought about the strict aliasing. I think the only sane way would be to require strict aliasing for the strict vtable pointers. Repository: rL LLVM https://reviews.llvm.org/D47299 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.
aprantl added a comment.
Thanks! Is there also a companion patch for LLVM that disables the objc
accelerator table?
Note that this is not a 100% replacement of the apple_objc accelerator table,
since the apple_objc table also lists all methods defined in categories of that
interface. Is the idea to also add category methods into the interface's
DW_TAG_struture_type?
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3358
+// Starting with DWARF5, we create declarations for the interface's
+// methods.
+if (const auto *OMD = dyn_cast_or_null(D)) {
`// Starting with DWARF V5 method declarations are emitted as children of the
interface type.`
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4247
+
+ SmallVector EltTys;
+ for (auto *E : RealDecl->getElements()) {
`EltTys.append(RealDecl->getElements().begin(), RealDecl->getElements().end())`
Comment at: clang/lib/CodeGen/CGDebugInfo.h:105
+ llvm::DISubprogram *DIMethodDecl;
+ MethodData(const ObjCMethodDecl *MD, llvm::DISubprogram *DIMethodDecl)
+ : MD(MD), DIMethodDecl(DIMethodDecl) {}
This constructor is probably not necessary if you construct the struct as `{
MD, Decl }`?
Comment at: clang/lib/CodeGen/CGDebugInfo.h:111
+// methods.
+llvm::DICompositeType *DIInterfaceDecl;
+std::vector Methods;
Isn't the interface already the key in the DenseMap?
Comment at: clang/test/CodeGenObjC/debug-info-synthesis.m:35
+// DWARF5: ![[STRUCT:.*]] = !DICompositeType(tag: DW_TAG_structure_type, name:
"Foo"
// CHECK: ![[FILE:.*]] = !DIFile(filename: "{{[^"]+}}foo.h"
We should also check that this does not happen in DWARF 4.
Repository:
rC Clang
https://reviews.llvm.org/D48241
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r334850 - [X86] Lowering sqrt intrinsics to native IR
Author: tkrupa
Date: Fri Jun 15 11:05:59 2018
New Revision: 334850
URL: http://llvm.org/viewvc/llvm-project?rev=334850=rev
Log:
[X86] Lowering sqrt intrinsics to native IR
Reviewers: craig.topper, spatel, RKSimon, igorb, uriel.k
Reviewed By: craig.topper
Subscribers: tkrupa, cfe-commits
Differential Revision: https://reviews.llvm.org/D41168
Modified:
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/test/CodeGen/avx-builtins.c
cfe/trunk/test/CodeGen/avx512f-builtins.c
cfe/trunk/test/CodeGen/avx512vl-builtins.c
cfe/trunk/test/CodeGen/sse-builtins.c
cfe/trunk/test/CodeGen/sse2-builtins.c
Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=334850=334849=334850=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Fri Jun 15 11:05:59 2018
@@ -9889,7 +9889,56 @@ Value *CodeGenFunction::EmitX86BuiltinEx
Function *F = CGM.getIntrinsic(Intrinsic::ctlz, Ops[0]->getType());
return Builder.CreateCall(F, {Ops[0],Builder.getInt1(false)});
}
-
+ case X86::BI__builtin_ia32_sqrtss:
+ case X86::BI__builtin_ia32_sqrtsd: {
+Value *A = Builder.CreateExtractElement(Ops[0], (uint64_t)0);
+Function *F = CGM.getIntrinsic(Intrinsic::sqrt, A->getType());
+A = Builder.CreateCall(F, {A});
+return Builder.CreateInsertElement(Ops[0], A, (uint64_t)0);
+ }
+ case X86::BI__builtin_ia32_sqrtsd_round_mask:
+ case X86::BI__builtin_ia32_sqrtss_round_mask: {
+unsigned CC = cast(Ops[4])->getZExtValue();
+// Support only if the rounding mode is 4 (AKA CUR_DIRECTION),
+// otherwise keep the intrinsic.
+if (CC != 4) {
+ Intrinsic::ID IID = BuiltinID == X86::BI__builtin_ia32_sqrtsd_round_mask
?
+ Intrinsic::x86_avx512_mask_sqrt_sd :
+ Intrinsic::x86_avx512_mask_sqrt_ss;
+ return Builder.CreateCall(CGM.getIntrinsic(IID), Ops);
+}
+Value *A = Builder.CreateExtractElement(Ops[0], (uint64_t)0);
+Function *F = CGM.getIntrinsic(Intrinsic::sqrt, A->getType());
+Value *Src = Builder.CreateExtractElement(Ops[2], (uint64_t)0);
+int MaskSize = Ops[3]->getType()->getScalarSizeInBits();
+llvm::Type *MaskTy = llvm::VectorType::get(Builder.getInt1Ty(), MaskSize);
+Value *Mask = Builder.CreateBitCast(Ops[3], MaskTy);
+Mask = Builder.CreateExtractElement(Mask, (uint64_t)0);
+A = Builder.CreateSelect(Mask, Builder.CreateCall(F, {A}), Src);
+return Builder.CreateInsertElement(Ops[1], A, (uint64_t)0);
+ }
+ case X86::BI__builtin_ia32_sqrtpd256:
+ case X86::BI__builtin_ia32_sqrtpd:
+ case X86::BI__builtin_ia32_sqrtps256:
+ case X86::BI__builtin_ia32_sqrtps: {
+Function *F = CGM.getIntrinsic(Intrinsic::sqrt, Ops[0]->getType());
+return Builder.CreateCall(F, {Ops[0]});
+ }
+ case X86::BI__builtin_ia32_sqrtps512_mask:
+ case X86::BI__builtin_ia32_sqrtpd512_mask: {
+unsigned CC = cast(Ops[3])->getZExtValue();
+// Support only if the rounding mode is 4 (AKA CUR_DIRECTION),
+// otherwise keep the intrinsic.
+if (CC != 4) {
+ Intrinsic::ID IID = BuiltinID == X86::BI__builtin_ia32_sqrtps512_mask ?
+ Intrinsic::x86_avx512_mask_sqrt_ps_512 :
+ Intrinsic::x86_avx512_mask_sqrt_pd_512;
+ return Builder.CreateCall(CGM.getIntrinsic(IID), Ops);
+}
+Function *F = CGM.getIntrinsic(Intrinsic::sqrt, Ops[0]->getType());
+return EmitX86Select(*this, Ops[2], Builder.CreateCall(F, {Ops[0]}),
+ Ops[1]);
+ }
case X86::BI__builtin_ia32_pabsb128:
case X86::BI__builtin_ia32_pabsw128:
case X86::BI__builtin_ia32_pabsd128:
Modified: cfe/trunk/test/CodeGen/avx-builtins.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/avx-builtins.c?rev=334850=334849=334850=diff
==
--- cfe/trunk/test/CodeGen/avx-builtins.c (original)
+++ cfe/trunk/test/CodeGen/avx-builtins.c Fri Jun 15 11:05:59 2018
@@ -1116,13 +1116,13 @@ __m256 test_mm256_shuffle_ps(__m256 A, _
__m256d test_mm256_sqrt_pd(__m256d A) {
// CHECK-LABEL: test_mm256_sqrt_pd
- // CHECK: call <4 x double> @llvm.x86.avx.sqrt.pd.256(<4 x double> %{{.*}})
+ // CHECK: call <4 x double> @llvm.sqrt.v4f64(<4 x double> %{{.*}})
return _mm256_sqrt_pd(A);
}
__m256 test_mm256_sqrt_ps(__m256 A) {
// CHECK-LABEL: test_mm256_sqrt_ps
- // CHECK: call <8 x float> @llvm.x86.avx.sqrt.ps.256(<8 x float> %{{.*}})
+ // CHECK: call <8 x float> @llvm.sqrt.v8f32(<8 x float> %{{.*}})
return _mm256_sqrt_ps(A);
}
Modified: cfe/trunk/test/CodeGen/avx512f-builtins.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/avx512f-builtins.c?rev=334850=334849=334850=diff
==
---
r334839 - bpf: recognize target specific option -mattr=dwarfris in clang
Author: yhs
Date: Fri Jun 15 08:53:31 2018
New Revision: 334839
URL: http://llvm.org/viewvc/llvm-project?rev=334839=rev
Log:
bpf: recognize target specific option -mattr=dwarfris in clang
The following is the usage example with clang:
bash-4.2$ clang -target bpf -O2 -g -c -Xclang -target-feature -Xclang
+dwarfris t.c
bash-4.2$ llvm-objdump -S -d t.o
t.o:file format ELF64-BPF
Disassembly of section .text:
test:
; int test(void) {
0: b7 00 00 00 00 00 00 00 r0 = 0
; return 0;
1: 95 00 00 00 00 00 00 00 exit
bash-4.2$ cat t.c
int test(void) {
return 0;
}
bash-4.2$
Signed-off-by: Yonghong Song
Modified:
cfe/trunk/lib/Basic/Targets/BPF.h
Modified: cfe/trunk/lib/Basic/Targets/BPF.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/BPF.h?rev=334839=334838=334839=diff
==
--- cfe/trunk/lib/Basic/Targets/BPF.h (original)
+++ cfe/trunk/lib/Basic/Targets/BPF.h Fri Jun 15 08:53:31 2018
@@ -47,7 +47,7 @@ public:
MacroBuilder ) const override;
bool hasFeature(StringRef Feature) const override {
-return Feature == "bpf" || Feature == "alu32";
+return Feature == "bpf" || Feature == "alu32" || Feature == "dwarfris";
}
void setFeatureEnabled(llvm::StringMap , StringRef Name,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.
dblaikie added a comment. Not sure I should get too involved in this discussion, knowing so little about Objective C - but if it seems sensible, could you provide some examples (just in comments/description in this code review) of what the DWARF used to look like and what it looks like after this change? Does this address the discoverability that's being discussed in the llvm-dev thread? There were concerns there about having to search through all the instances of a type to find all of its functions - I imagine, since Objective C classes aren't closed (if I understand correctly) that would still be a concern here? If it is, what is the benefit/tradeoff of this change (if it doesn't address the discoverability/still requires the Objective C accelerator table)? Repository: rC Clang https://reviews.llvm.org/D48241 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48242: [ASTMatchers] Add support for matching the type of a friend decl.
dlj created this revision.
dlj added a reviewer: klimek.
dlj added a project: clang.
This allows matchers like:
friendDecl(hasType(cxxRecordDecl(...)))
friendDecl(hasType(asString(...)))
It seems that hasType is probably the most reasonable narrowing matcher to
overload, since it is already used to narrow to other declaration kinds.
Repository:
rC Clang
https://reviews.llvm.org/D48242
Files:
include/clang/ASTMatchers/ASTMatchers.h
include/clang/ASTMatchers/ASTMatchersInternal.h
unittests/ASTMatchers/ASTMatchersNodeTest.cpp
Index: unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -160,6 +160,16 @@
valueDecl(hasType(asString("void (void)");
}
+TEST(FriendDecl, Matches) {
+ EXPECT_TRUE(matches("class Y { friend class X; };",
+ friendDecl(hasType(asString("class X");
+ EXPECT_TRUE(matches("class Y { friend class X; };",
+ friendDecl(hasType(recordDecl(hasName("X"));
+
+ EXPECT_TRUE(matches("class Y { friend void f(); };",
+ functionDecl(hasName("f"), hasParent(friendDecl();
+}
+
TEST(Enum, DoesNotMatchClasses) {
EXPECT_TRUE(notMatches("class X {};", enumDecl(hasName("X";
}
Index: include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- include/clang/ASTMatchers/ASTMatchersInternal.h
+++ include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -38,6 +38,7 @@
#include "clang/AST/ASTTypeTraits.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclFriend.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
@@ -120,10 +121,14 @@
inline QualType getUnderlyingType(const ValueDecl ) {
return Node.getType();
}
-
inline QualType getUnderlyingType(const TypedefNameDecl ) {
return Node.getUnderlyingType();
}
+inline QualType getUnderlyingType(const FriendDecl ) {
+ if (const TypeSourceInfo *tsi = Node.getFriendType())
+return tsi->getType();
+ return QualType();
+}
/// Unifies obtaining the FunctionProtoType pointer from both
/// FunctionProtoType and FunctionDecl nodes..
Index: include/clang/ASTMatchers/ASTMatchers.h
===
--- include/clang/ASTMatchers/ASTMatchers.h
+++ include/clang/ASTMatchers/ASTMatchers.h
@@ -2860,13 +2860,17 @@
/// Example matches x (matcher = expr(hasType(cxxRecordDecl(hasName("X")
/// and z (matcher = varDecl(hasType(cxxRecordDecl(hasName("X")
/// and U (matcher = typedefDecl(hasType(asString("int")))
+/// and friend class X (matcher = friendDecl(hasType("X"))
/// \code
/// class X {};
/// void y(X ) { x; X z; }
/// typedef int U;
+/// class Y { friend class X; };
/// \endcode
AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
-hasType, AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, TypedefNameDecl, ValueDecl),
+hasType,
+AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, TypedefNameDecl,
+ValueDecl),
internal::Matcher, InnerMatcher, 0) {
QualType QT = internal::getUnderlyingType(Node);
if (!QT.isNull())
@@ -2885,18 +2889,22 @@
///
/// Example matches x (matcher = expr(hasType(cxxRecordDecl(hasName("X")
/// and z (matcher = varDecl(hasType(cxxRecordDecl(hasName("X")
+/// and friend class X (matcher = friendDecl(hasType("X"))
/// \code
/// class X {};
/// void y(X ) { x; X z; }
+/// class Y { friend class X; };
/// \endcode
///
/// Usable as: Matcher, Matcher
-AST_POLYMORPHIC_MATCHER_P_OVERLOAD(hasType,
- AST_POLYMORPHIC_SUPPORTED_TYPES(Expr,
- ValueDecl),
- internal::Matcher, InnerMatcher, 1) {
- return qualType(hasDeclaration(InnerMatcher))
- .matches(Node.getType(), Finder, Builder);
+AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
+hasType,
+AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl),
+internal::Matcher, InnerMatcher, 1) {
+ QualType QT = internal::getUnderlyingType(Node);
+ if (!QT.isNull())
+return qualType(hasDeclaration(InnerMatcher)).matches(QT, Finder, Builder);
+ return false;
}
/// Matches if the type location of the declarator decl's type matches
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48241: [DebugInfo] Emit ObjC methods as part of interface.
JDevlieghere created this revision.
JDevlieghere added reviewers: aprantl, dexonsmith, dblaikie, labath.
As brought up during the discussion of the DWARF5 accelerator tables,
there is currently no way to associate Objective-C methods with the
interface they belong to, other than the .apple_objc accelerator table.
After due consideration we came to the conclusion that it makes more
sense to follow Pavel's suggestion of just emitting this information in
the .debug_info section. One concern was that categories were
emitted in the .apple_names as well, but it turns out that LLDB doesn't
rely on the accelerator tables for this information.
This patch changes the codegen behavior to emit subprograms for
structure types, like we do for C++. This will result in the
DW_TAG_subprogram being nested as a child under its
DW_TAG_structure_type. This behavior is only enabled for DWARF5 and
later, so we can have a unique code path in LLDB with regards to
obtaining the class methods.
For more background please refer to the discussion on the mailing list:
http://lists.llvm.org/pipermail/llvm-dev/2018-June/123986.html
Repository:
rC Clang
https://reviews.llvm.org/D48241
Files:
clang/lib/CodeGen/CGDebugInfo.cpp
clang/lib/CodeGen/CGDebugInfo.h
clang/test/CodeGenObjC/debug-info-synthesis.m
Index: clang/test/CodeGenObjC/debug-info-synthesis.m
===
--- clang/test/CodeGenObjC/debug-info-synthesis.m
+++ clang/test/CodeGenObjC/debug-info-synthesis.m
@@ -1,4 +1,6 @@
// RUN: %clang_cc1 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s
+// RUN: %clang_cc1 -dwarf-version=5 -emit-llvm -debug-info-kind=limited -w -triple x86_64-apple-darwin10 %s -o - | FileCheck %s --check-prefix CHECK --check-prefix DWARF5
+
# 1 "foo.m" 1
# 1 "foo.m" 2
# 1 "./foo.h" 1
@@ -30,8 +32,13 @@
}
}
+// DWARF5: ![[STRUCT:.*]] = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo"
// CHECK: ![[FILE:.*]] = !DIFile(filename: "{{[^"]+}}foo.h"
-// CHECK: !DISubprogram(name: "-[Foo setDict:]"
+// DWARF5: !DISubprogram(name: "-[Foo setDict:]"
+// DWARF5-SAME: scope: ![[STRUCT]],
+// DWARF5-SAME: line: 8,
+// DWARF5-SAME: isLocal: true, isDefinition: false
+// CHECK: distinct !DISubprogram(name: "-[Foo setDict:]"
// CHECK-SAME: file: ![[FILE]],
// CHECK-SAME: line: 8,
// CHECK-SAME: isLocal: true, isDefinition: true
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -98,6 +98,27 @@
/// Cache of previously constructed interfaces which may change.
llvm::SmallVector ObjCInterfaceCache;
+ struct ObjCMethodCacheEntry {
+struct MethodData {
+ const ObjCMethodDecl *MD;
+ llvm::DISubprogram *DIMethodDecl;
+ MethodData(const ObjCMethodDecl *MD, llvm::DISubprogram *DIMethodDecl)
+ : MD(MD), DIMethodDecl(DIMethodDecl) {}
+};
+
+// Keep track of the interface so we can update its elements with its
+// methods.
+llvm::DICompositeType *DIInterfaceDecl;
+std::vector Methods;
+
+ObjCMethodCacheEntry(llvm::DICompositeType *DIInterfaceDecl = nullptr)
+: DIInterfaceDecl(DIInterfaceDecl) {}
+ };
+
+ /// Cache of forward declarations for method
+ llvm::DenseMap
+ ObjCMethodCache;
+
/// Cache of references to clang modules and precompiled headers.
llvm::DenseMap ModuleCache;
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -2231,6 +2231,13 @@
Mod ? Mod : Unit, ID->getName(), DefUnit, Line, Size, Align, Flags,
nullptr, llvm::DINodeArray(), RuntimeLang);
+ if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Remember the DICompositeType in the ObjCMethodCache.
+assert(ObjCMethodCache[ID].DIInterfaceDecl == nullptr ||
+ ObjCMethodCache[ID].DIInterfaceDecl == RealDecl);
+ObjCMethodCache[ID].DIInterfaceDecl = RealDecl;
+ }
+
QualType QTy(Ty, 0);
TypeCache[QTy.getAsOpaquePtr()].reset(RealDecl);
@@ -3346,6 +3353,21 @@
if (HasDecl && isa(D))
DeclCache[D->getCanonicalDecl()].reset(SP);
+ if (CGM.getCodeGenOpts().DwarfVersion >= 5) {
+// Starting with DWARF5, we create declarations for the interface's
+// methods.
+if (const auto *OMD = dyn_cast_or_null(D)) {
+ const ObjCInterfaceDecl *ID = OMD->getClassInterface();
+ llvm::DISubprogram *FD = DBuilder.createFunction(
+ ObjCMethodCache[ID].DIInterfaceDecl, Name, LinkageName, Unit, LineNo,
+ getOrCreateFunctionType(D, FnType, Unit), Fn->hasLocalLinkage(),
+ false /*definition*/, ScopeLine, Flags, CGM.getLangOpts().Optimize,
+ TParamsArray.get());
+ DBuilder.finalizeSubprogram(FD);
+
[PATCH] D47196: [Time-report ](2): Recursive timers in Clang
efriedma added a comment. Can you give an example of what the output looks like? Please don't define multiple different classes with the same name SortClassName. It seems like you've scattered the "start" and "stop" calls all over the place; can you put the start and stop calls in the same place, somehow? Comment at: lib/CodeGen/CodeGenAction.cpp:835 +GlobalDecl GD; +return CGM.getMangledName(GD.getWithDecl(FD)); + } The use of getWithDecl here is weird; can you just do `GlobalDecl(FD)`? If not, please add a comment explaining what you're doing. Why does it matter whether the function has a body? IIRC you should be able to mangle the name of functions without a body. https://reviews.llvm.org/D47196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47819: [test] Support using libtirpc on Linux
mgorny added a comment. Gentle ping. Repository: rCRT Compiler Runtime https://reviews.llvm.org/D47819 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47817: [sanitizer_common] Fix using libtirpc on Linux
mgorny added a comment. Gentle ping. Repository: rCRT Compiler Runtime https://reviews.llvm.org/D47817 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call
NoQ accepted this revision.
NoQ added a comment.
Ok, let's land this one and see how it goes! I'm looking forward to seeing the
follow-up patches.
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:359-372
+ // Checking bases.
+ const auto *CXXRD = dyn_cast(RD);
+ if (!CXXRD)
+return ContainsUninitField;
+
+ for (const CXXBaseSpecifier BaseSpec : CXXRD->bases()) {
+const auto *Base = BaseSpec.getType()->getAsCXXRecordDecl();
Szelethus wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Szelethus wrote:
> > > > NoQ wrote:
> > > > > Are there many warnings that will be caused by this but won't be
> > > > > caused by conducting the check for the constructor-within-constructor
> > > > > that's currently disabled? Not sure - is it even syntactically
> > > > > possible to not initialize the base class?
> > > > I'm not a 100% sure what you mean. Can you clarify?
> > > > >Not sure - is it even syntactically possible to not initialize the
> > > > >base class?
> > > > If I understand the question correctly, no, as far as I know.
> > > You have a check for `isCalledByConstructor()` in order to skip base
> > > class constructors but then you check them manually. That's a lot of
> > > code, but it seems that the same result could have been achieved by
> > > simply skipping the descent into the base class.
> > >
> > > Same question for class-type fields, actually.
> > >Same question for class-type fields, actually.
> >
> > My problem with that approach is that the same uninitialized field could've
> > been emitted multiple times in different warnings. From an earlier
> > conversation (assume that its run in pedantic mode):
> >
> > >For this code snippet:
> > >
> > > ```
> > >struct A {
> > > struct B {
> > > int x, y;
> > >
> > > B() {}
> > > } b;
> > > int w;
> > >
> > > A() {
> > > b = B();
> > > }
> > >};
> > >```
> > >the warning message after a call to `A::A()` would be "3 uninitialized
> > >fields[...]", and for `B::B()` inside `A`s constructor would be "2
> > >uninitialized fields[...]", so it wouldn't be filtered out.
> >
> > In my opinion, if `A` contains a field of type `B`, it should be the
> > responsibility of `A`'s constructors to initialize those fields properly.
> > >You have a check for isCalledByConstructor() in order to skip base class
> > >constructors but then you check them manually. That's a lot of code, but
> > >it seems that the same result could have been achieved by simply skipping
> > >the descent into the base class.
> > I also believe that a constructor "should be held responsible" for the
> > initialization of the inherited data members. However, in the case of base
> > classes, uniqueing isn't an issue, as in this case:
> > ```
> > struct B {
> > int a;
> > B() {}
> > };
> >
> > struct D : public B {
> > int b;
> > D() {}
> > };
> > ```
> > a call to `D::D()` would not check the inherited member (`a`), if I didn't
> > implement it explicitly. That also means that if `isCalledByConstructor`
> > was refactored to not filter out base classes, we would get two warning
> > each with a single note, reporting `b` and `a` respectively. (I haven't
> > actually checked it, but its a reasonable assumption)
> >
> > Actually, I'm not 100% sure which approach is better: a warning for each
> > base, or one warning for the object, bases included. I personally think one
> > warning per object results in the best user experience.
> >
> > I like both ideas, but I think if we'd come to the conclusion that a
> > warning per base should be the way to go, it should definitely be
> > implemented in a followup patch, as `isCalledByConstructor` is already in
> > line for some major refactoring.
> >
> > What do you think? :)
> >[...]but it seems that the same result could have been achieved by simply
> >skipping the descent into the base class.
> I can't edit inline comments sadly, but I'd just like to point out that it
> wouldn't achieve the same thing, as highlighted above. But, both solution
> would be okay.
I still don't fully understand the reasoning behind who's responsible for
initializing the field, i.e., the exact rule that you're enforcing seems to
cause warnings to depend heavily on meta-information such as where does the
analysis start, which is confusing. Because we're trying to enforce a coding
guideline here, i believe that the guideline itself should be transparent and
easy to explain. In particular, it should be obvious to the user which
constructor is responsible for initializing a field, and by obvious i mean it
should be obvious both from the guideline itself and from the analyzer's
warning, and both of them should be the same "obvious".
https://reviews.llvm.org/D45532
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35110: [Analyzer] Constraint Manager Negates Difference
NoQ added a comment. I still don't think i fully understand your concern? Could you provide an example and point out what exactly goes wrong? https://reviews.llvm.org/D35110 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35110: [Analyzer] Constraint Manager Negates Difference
NoQ added a comment. > In the iterator checkers we do not know anything about the rearranged > expressions, it has no access to the sum/difference, the whole purpose of > your proposal was to put in into the infrastructure. It wasn't. The purpose was merely to move (de-duplicate) the code that computes the sum/difference away from the checker. The checker can still operate on the result of such calculation if it knows something about that result that the core doesn't. https://reviews.llvm.org/D35110 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48151: [CUDA] Make __host__/__device__ min/max overloads constexpr in C++14.
jlebar added a comment. In https://reviews.llvm.org/D48151#1133954, @rsmith wrote: > LGTM Thank you for the review, Richard. Will check this in once the whole stack is ready -- just need https://reviews.llvm.org/D48036. https://reviews.llvm.org/D48151 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48208: [Fuchsia] Enable static libc++, libc++abi, libunwind
mcgrathr accepted this revision. mcgrathr added a comment. This revision is now accepted and ready to land. lgtm Repository: rC Clang https://reviews.llvm.org/D48208 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r334859 - [Modules] Improve .Private fix-its to handle 'explicit' and 'framework'
Author: bruno
Date: Fri Jun 15 13:13:28 2018
New Revision: 334859
URL: http://llvm.org/viewvc/llvm-project?rev=334859=rev
Log:
[Modules] Improve .Private fix-its to handle 'explicit' and 'framework'
When in the context of suggestion the fix-it from .Private to _Private
for private modules, trim off the 'explicit' and add 'framework' when
appropriate.
rdar://problem/41030554
Modified:
cfe/trunk/lib/Lex/ModuleMap.cpp
cfe/trunk/test/Modules/Inputs/implicit-private-with-submodule/A.framework/Modules/module.modulemap
cfe/trunk/test/Modules/Inputs/implicit-private-with-submodule/A.framework/Modules/module.private.modulemap
cfe/trunk/test/Modules/implicit-private-with-submodule.m
Modified: cfe/trunk/lib/Lex/ModuleMap.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/ModuleMap.cpp?rev=334859=334858=334859=diff
==
--- cfe/trunk/lib/Lex/ModuleMap.cpp (original)
+++ cfe/trunk/lib/Lex/ModuleMap.cpp Fri Jun 15 13:13:28 2018
@@ -1403,6 +1403,13 @@ namespace clang {
void parseConflict();
void parseInferredModuleDecl(bool Framework, bool Explicit);
+/// Private modules are canonicalized as Foo_Private. Clang provides extra
+/// module map search logic to find the appropriate private module when PCH
+/// is used with implicit module maps. Warn when private modules are
written
+/// in other ways (FooPrivate and Foo.Private), providing notes and fixits.
+void diagnosePrivateModules(SourceLocation ExplicitLoc,
+SourceLocation FrameworkLoc);
+
using Attributes = ModuleMap::Attributes;
bool parseOptionalAttributes(Attributes );
@@ -1672,11 +1679,8 @@ namespace {
/// module map search logic to find the appropriate private module when PCH
/// is used with implicit module maps. Warn when private modules are written
/// in other ways (FooPrivate and Foo.Private), providing notes and fixits.
-static void diagnosePrivateModules(const ModuleMap ,
- DiagnosticsEngine ,
- const Module *ActiveModule,
- SourceLocation InlineParent) {
-
+void ModuleMapParser::diagnosePrivateModules(SourceLocation ExplicitLoc,
+ SourceLocation FrameworkLoc) {
auto GenNoteAndFixIt = [&](StringRef BadName, StringRef Canonical,
const Module *M, SourceRange ReplLoc) {
auto D = Diags.Report(ActiveModule->DefinitionLoc,
@@ -1693,6 +1697,7 @@ static void diagnosePrivateModules(const
SmallString<128> FullName(ActiveModule->getFullModuleName());
if (!FullName.startswith(M->Name) && !FullName.endswith("Private"))
continue;
+SmallString<128> FixedPrivModDecl;
SmallString<128> Canonical(M->Name);
Canonical.append("_Private");
@@ -1702,8 +1707,20 @@ static void diagnosePrivateModules(const
Diags.Report(ActiveModule->DefinitionLoc,
diag::warn_mmap_mismatched_private_submodule)
<< FullName;
- GenNoteAndFixIt(FullName, Canonical, M,
- SourceRange(InlineParent, ActiveModule->DefinitionLoc));
+
+ SourceLocation FixItInitBegin = CurrModuleDeclLoc;
+ if (FrameworkLoc.isValid())
+FixItInitBegin = FrameworkLoc;
+ if (ExplicitLoc.isValid())
+FixItInitBegin = ExplicitLoc;
+
+ if (FrameworkLoc.isValid() || ActiveModule->Parent->IsFramework)
+FixedPrivModDecl.append("framework ");
+ FixedPrivModDecl.append("module ");
+ FixedPrivModDecl.append(Canonical);
+
+ GenNoteAndFixIt(FullName, FixedPrivModDecl, M,
+ SourceRange(FixItInitBegin,
ActiveModule->DefinitionLoc));
continue;
}
@@ -1747,6 +1764,7 @@ void ModuleMapParser::parseModuleDecl()
// Parse 'explicit' or 'framework' keyword, if present.
SourceLocation ExplicitLoc;
+ SourceLocation FrameworkLoc;
bool Explicit = false;
bool Framework = false;
@@ -1758,7 +1776,7 @@ void ModuleMapParser::parseModuleDecl()
// Parse 'framework' keyword, if present.
if (Tok.is(MMToken::FrameworkKeyword)) {
-consumeToken();
+FrameworkLoc = consumeToken();
Framework = true;
}
@@ -1800,7 +1818,6 @@ void ModuleMapParser::parseModuleDecl()
}
Module *PreviousActiveModule = ActiveModule;
- SourceLocation LastInlineParentLoc = SourceLocation();
if (Id.size() > 1) {
// This module map defines a submodule. Go find the module of which it
// is a submodule.
@@ -1811,7 +1828,6 @@ void ModuleMapParser::parseModuleDecl()
if (I == 0)
TopLevelModule = Next;
ActiveModule = Next;
-LastInlineParentLoc = Id[I].second;
continue;
}
@@ -1934,7 +1950,7 @@ void ModuleMapParser::parseModuleDecl()
!Diags.isIgnored(diag::warn_mmap_mismatched_private_module_name,
[PATCH] D48232: [analyzer] Fix symbolic-pointer-to-boolean casts during load.
NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet,
rnkovacs.
Herald added subscribers: cfe-commits, mikhail.ramalho, baloghadamsoftware.
The canonical way to represent the result of casting `{$x}` to `bool`
is `($x != 0)`, not `$x`. In fact `$x` is an ill-formed `SVal` (when`$x` is a
loc-type symbol) and it gets caught by https://reviews.llvm.org/D48205. Fix the
cast procedure.
Because our cast code is a spaghetti, the code that was fixed was in fact
executed very rarely, because there's a duplicate guard in `evalCast()` that's
written correctly. But when `evalCastFromLoc()` is called directly (eg., from
`CastRetrievedVal()`), this becomes a problem.
Repository:
rC Clang
https://reviews.llvm.org/D48232
Files:
lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
test/Analysis/casts.cpp
test/Analysis/pr37802.cpp
Index: test/Analysis/pr37802.cpp
===
--- /dev/null
+++ test/Analysis/pr37802.cpp
@@ -0,0 +1,106 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core -verify %s
+
+// expected-no-diagnostics
+
+void *operator new(unsigned long, void *h) { return h; }
+
+// I've no idea what this code does, but it used to crash, so let's keep it.
+namespace pr37802_v1 {
+struct J {
+ int *p;
+};
+class X {
+ void *ar;
+
+public:
+ X(void *t) : ar(t) {}
+ template
+ void f(const T ) {
+new (ar) T(t);
+ }
+};
+class Y {
+public:
+ template
+ void f(T &&);
+ void f(J t) {
+f(*t.p);
+ }
+};
+class Z {
+ int at() const {}
+
+public:
+ Z(const Z ) {
+other.au(X(this));
+ }
+ template
+ void au(T t) const {
+void *c = const_cast(this);
+if (at()) {
+ t.f(*static_cast(c));
+} else {
+ t.f(*static_cast(c));
+}
+ }
+};
+Z g() {
+ Z az = g();
+ Z e = az;
+ Y d;
+ e.au(d);
+}
+} // namespace pr37802_v1
+
+
+// This slightly modified code crashed differently.
+namespace pr37802_v2 {
+struct J {
+ int *p;
+};
+
+class X {
+ void *ar;
+
+public:
+ X(void *t) : ar(t) {}
+ void f(const J ) { new (ar) J(t); }
+ void f(const bool ) { new (ar) bool(t); }
+};
+
+class Y {
+public:
+ void boolf(bool &&);
+ void f(J &&);
+ void f(J t) { boolf(*t.p); }
+};
+
+class Z {
+ int at() const {}
+
+public:
+ Z(const Z ) { other.au(X(this)); }
+ void au(X t) const {
+void *c = const_cast(this);
+if (at()) {
+ t.f(*static_cast(c));
+} else {
+ t.f(*static_cast(c));
+}
+ }
+ void au(Y t) const {
+void *c = const_cast(this);
+if (at()) {
+ t.f(*static_cast(c));
+} else {
+}
+ }
+};
+
+Z g() {
+ Z az = g();
+ Z e = az;
+ Y d;
+ e.au(d);
+}
+} // namespace pr37802_v2
Index: test/Analysis/casts.cpp
===
--- test/Analysis/casts.cpp
+++ test/Analysis/casts.cpp
@@ -35,3 +35,9 @@
bool testCastToIntPtrRValueRef(char *p, int *s) {
return castToIntPtrRValueRef(p) != s; // no-crash
}
+
+bool retrievePointerFromBoolean(int *p) {
+ bool q;
+ *reinterpret_cast() = p;
+ return q;
+}
Index: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -159,7 +159,8 @@
return nonloc::SymbolVal(SymMgr.getExtentSymbol(FTR));
if (const SymbolicRegion *SymR = R->getSymbolicBase())
- return nonloc::SymbolVal(SymR->getSymbol());
+ return makeNonLoc(SymR->getSymbol(), BO_NE,
+BasicVals.getZeroWithPtrWidth(), castTy);
// FALL-THROUGH
LLVM_FALLTHROUGH;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48151: [CUDA] Make __host__/__device__ min/max overloads constexpr in C++14.
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D48151 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48204: [analyzer] Make getDerefExpr() skip cleanups.
NoQ added inline comments.
Comment at: test/Analysis/inlining/inline-defensive-checks.cpp:93
+S *conjure();
+S *get_conjured(S _) {
+ S *s = conjure();
george.karpenkov wrote:
> what is the argument doing?
Causing cleanups (:
Repository:
rC Clang
https://reviews.llvm.org/D48204
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41168: [X86] Lowering X86 avx512 sqrt intrinsics to IR
RKSimon added a comment. LGTM as well Repository: rL LLVM https://reviews.llvm.org/D41168 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D41168: [X86] Lowering X86 avx512 sqrt intrinsics to IR
This revision was not accepted when it landed; it landed in state "Needs
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL334850: [X86] Lowering sqrt intrinsics to native IR
(authored by tkrupa, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D41168
Files:
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/test/CodeGen/avx-builtins.c
cfe/trunk/test/CodeGen/avx512f-builtins.c
cfe/trunk/test/CodeGen/avx512vl-builtins.c
cfe/trunk/test/CodeGen/sse-builtins.c
cfe/trunk/test/CodeGen/sse2-builtins.c
Index: cfe/trunk/test/CodeGen/avx-builtins.c
===
--- cfe/trunk/test/CodeGen/avx-builtins.c
+++ cfe/trunk/test/CodeGen/avx-builtins.c
@@ -1116,13 +1116,13 @@
__m256d test_mm256_sqrt_pd(__m256d A) {
// CHECK-LABEL: test_mm256_sqrt_pd
- // CHECK: call <4 x double> @llvm.x86.avx.sqrt.pd.256(<4 x double> %{{.*}})
+ // CHECK: call <4 x double> @llvm.sqrt.v4f64(<4 x double> %{{.*}})
return _mm256_sqrt_pd(A);
}
__m256 test_mm256_sqrt_ps(__m256 A) {
// CHECK-LABEL: test_mm256_sqrt_ps
- // CHECK: call <8 x float> @llvm.x86.avx.sqrt.ps.256(<8 x float> %{{.*}})
+ // CHECK: call <8 x float> @llvm.sqrt.v8f32(<8 x float> %{{.*}})
return _mm256_sqrt_ps(A);
}
Index: cfe/trunk/test/CodeGen/avx512f-builtins.c
===
--- cfe/trunk/test/CodeGen/avx512f-builtins.c
+++ cfe/trunk/test/CodeGen/avx512f-builtins.c
@@ -5,84 +5,100 @@
__m512d test_mm512_sqrt_pd(__m512d a)
{
// CHECK-LABEL: @test_mm512_sqrt_pd
- // CHECK: @llvm.x86.avx512.mask.sqrt.pd.512
+ // CHECK: call <8 x double> @llvm.sqrt.v8f64(<8 x double> %{{.*}})
return _mm512_sqrt_pd(a);
}
__m512d test_mm512_mask_sqrt_pd (__m512d __W, __mmask8 __U, __m512d __A)
{
// CHECK-LABEL: @test_mm512_mask_sqrt_pd
- // CHECK: @llvm.x86.avx512.mask.sqrt.pd.512
+ // CHECK: call <8 x double> @llvm.sqrt.v8f64(<8 x double> %{{.*}})
+ // CHECK: bitcast i8 %{{.*}} to <8 x i1>
+ // CHECK: select <8 x i1> %{{.*}}, <8 x double> %{{.*}}, <8 x double> %{{.*}}
return _mm512_mask_sqrt_pd (__W,__U,__A);
}
__m512d test_mm512_maskz_sqrt_pd (__mmask8 __U, __m512d __A)
{
// CHECK-LABEL: @test_mm512_maskz_sqrt_pd
- // CHECK: @llvm.x86.avx512.mask.sqrt.pd.512
+ // CHECK: call <8 x double> @llvm.sqrt.v8f64(<8 x double> %{{.*}})
+ // CHECK: bitcast i8 %{{.*}} to <8 x i1>
+ // CHECK: select <8 x i1> %{{.*}}, <8 x double> %{{.*}}, <8 x double> {{.*}}
return _mm512_maskz_sqrt_pd (__U,__A);
}
__m512d test_mm512_mask_sqrt_round_pd(__m512d __W,__mmask8 __U,__m512d __A)
{
// CHECK-LABEL: @test_mm512_mask_sqrt_round_pd
- // CHECK: @llvm.x86.avx512.mask.sqrt.pd.512
+ // CHECK: call <8 x double> @llvm.sqrt.v8f64(<8 x double> %{{.*}})
+ // CHECK: bitcast i8 %{{.*}} to <8 x i1>
+ // CHECK: select <8 x i1> %{{.*}}, <8 x double> %{{.*}}, <8 x double> %{{.*}}
return _mm512_mask_sqrt_round_pd(__W,__U,__A,_MM_FROUND_CUR_DIRECTION);
}
__m512d test_mm512_maskz_sqrt_round_pd(__mmask8 __U,__m512d __A)
{
// CHECK-LABEL: @test_mm512_maskz_sqrt_round_pd
- // CHECK: @llvm.x86.avx512.mask.sqrt.pd.512
+ // CHECK: call <8 x double> @llvm.sqrt.v8f64(<8 x double> %{{.*}})
+ // CHECK: bitcast i8 %{{.*}} to <8 x i1>
+ // CHECK: select <8 x i1> %{{.*}}, <8 x double> %{{.*}}, <8 x double> {{.*}}
return _mm512_maskz_sqrt_round_pd(__U,__A,_MM_FROUND_CUR_DIRECTION);
}
__m512d test_mm512_sqrt_round_pd(__m512d __A)
{
// CHECK-LABEL: @test_mm512_sqrt_round_pd
- // CHECK: @llvm.x86.avx512.mask.sqrt.pd.512
+ // CHECK: call <8 x double> @llvm.sqrt.v8f64(<8 x double> %{{.*}})
return _mm512_sqrt_round_pd(__A,_MM_FROUND_CUR_DIRECTION);
}
__m512 test_mm512_sqrt_ps(__m512 a)
{
// CHECK-LABEL: @test_mm512_sqrt_ps
- // CHECK: @llvm.x86.avx512.mask.sqrt.ps.512
+ // CHECK: call <16 x float> @llvm.sqrt.v16f32(<16 x float> %{{.*}})
return _mm512_sqrt_ps(a);
}
__m512 test_mm512_mask_sqrt_ps(__m512 __W, __mmask16 __U, __m512 __A)
{
// CHECK-LABEL: @test_mm512_mask_sqrt_ps
- // CHECK: @llvm.x86.avx512.mask.sqrt.ps.512
+ // CHECK: call <16 x float> @llvm.sqrt.v16f32(<16 x float> %{{.*}})
+ // CHECK: bitcast i16 %{{.*}} to <16 x i1>
+ // CHECK: select <16 x i1> %{{.*}}, <16 x float> %{{.*}}, <16 x float> %{{.*}}
return _mm512_mask_sqrt_ps( __W, __U, __A);
}
__m512 test_mm512_maskz_sqrt_ps( __mmask16 __U, __m512 __A)
{
// CHECK-LABEL: @test_mm512_maskz_sqrt_ps
- // CHECK: @llvm.x86.avx512.mask.sqrt.ps.512
+ // CHECK: call <16 x float> @llvm.sqrt.v16f32(<16 x float> %{{.*}})
+ // CHECK: bitcast i16 %{{.*}} to <16 x i1>
+ // CHECK: select <16 x i1> %{{.*}}, <16 x float> %{{.*}}, <16 x float> {{.*}}
return _mm512_maskz_sqrt_ps(__U ,__A);
}
__m512 test_mm512_mask_sqrt_round_ps(__m512 __W,__mmask16 __U,__m512 __A)
{
// CHECK-LABEL: @test_mm512_mask_sqrt_round_ps
- // CHECK:
r334847 - [X86] __builtin_ia32_prord512_mask, __builtin_ia32_prorq512_mask, __builtin_ia32_shufpd should only accept an ICE constant.
Author: ctopper Date: Fri Jun 15 10:40:37 2018 New Revision: 334847 URL: http://llvm.org/viewvc/llvm-project?rev=334847=rev Log: [X86] __builtin_ia32_prord512_mask, __builtin_ia32_prorq512_mask, __builtin_ia32_shufpd should only accept an ICE constant. The rotates also need to check for the immediate to fit in 8-bits. Shufpd already checks its immediate range. Modified: cfe/trunk/include/clang/Basic/BuiltinsX86.def cfe/trunk/lib/Sema/SemaChecking.cpp Modified: cfe/trunk/include/clang/Basic/BuiltinsX86.def URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/BuiltinsX86.def?rev=334847=334846=334847=diff == --- cfe/trunk/include/clang/Basic/BuiltinsX86.def (original) +++ cfe/trunk/include/clang/Basic/BuiltinsX86.def Fri Jun 15 10:40:37 2018 @@ -328,7 +328,7 @@ TARGET_BUILTIN(__builtin_ia32_pshufhw, " TARGET_BUILTIN(__builtin_ia32_psadbw128, "V2LLiV16cV16c", "nc", "sse2") TARGET_BUILTIN(__builtin_ia32_sqrtpd, "V2dV2d", "nc", "sse2") TARGET_BUILTIN(__builtin_ia32_sqrtsd, "V2dV2d", "nc", "sse2") -TARGET_BUILTIN(__builtin_ia32_shufpd, "V2dV2dV2di", "nc", "sse2") +TARGET_BUILTIN(__builtin_ia32_shufpd, "V2dV2dV2dIi", "nc", "sse2") TARGET_BUILTIN(__builtin_ia32_cvtpd2dq, "V2LLiV2d", "nc", "sse2") TARGET_BUILTIN(__builtin_ia32_cvtpd2ps, "V4fV2d", "nc", "sse2") TARGET_BUILTIN(__builtin_ia32_cvttpd2dq, "V4iV2d", "nc", "sse2") @@ -1360,8 +1360,8 @@ TARGET_BUILTIN(__builtin_ia32_prolq128_m TARGET_BUILTIN(__builtin_ia32_prolq256_mask, "V4LLiV4LLiIiV4LLiUc", "nc", "avx512vl") TARGET_BUILTIN(__builtin_ia32_prolvd512_mask, "V16iV16iV16iV16iUs", "nc", "avx512f") TARGET_BUILTIN(__builtin_ia32_prolvq512_mask, "V8LLiV8LLiV8LLiV8LLiUc", "nc", "avx512f") -TARGET_BUILTIN(__builtin_ia32_prord512_mask, "V16iV16iiV16iUs", "nc", "avx512f") -TARGET_BUILTIN(__builtin_ia32_prorq512_mask, "V8LLiV8LLiiV8LLiUc", "nc", "avx512f") +TARGET_BUILTIN(__builtin_ia32_prord512_mask, "V16iV16iIiV16iUs", "nc", "avx512f") +TARGET_BUILTIN(__builtin_ia32_prorq512_mask, "V8LLiV8LLiIiV8LLiUc", "nc", "avx512f") TARGET_BUILTIN(__builtin_ia32_prolvd128_mask, "V4iV4iV4iV4iUc", "nc", "avx512vl") TARGET_BUILTIN(__builtin_ia32_prolvd256_mask, "V8iV8iV8iV8iUc", "nc", "avx512vl") TARGET_BUILTIN(__builtin_ia32_prolvq128_mask, "V2LLiV2LLiV2LLiV2LLiUc", "nc", "avx512vl") Modified: cfe/trunk/lib/Sema/SemaChecking.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=334847=334846=334847=diff == --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Jun 15 10:40:37 2018 @@ -2815,6 +2815,8 @@ bool Sema::CheckX86BuiltinFunctionCall(u case X86::BI__builtin_ia32_prold256_mask: case X86::BI__builtin_ia32_prolq128_mask: case X86::BI__builtin_ia32_prolq256_mask: + case X86::BI__builtin_ia32_prord512_mask: + case X86::BI__builtin_ia32_prorq512_mask: case X86::BI__builtin_ia32_prord128_mask: case X86::BI__builtin_ia32_prord256_mask: case X86::BI__builtin_ia32_prorq128_mask: ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
dexonsmith added reviewers: arphaman, ahatanak.
dexonsmith added subscribers: arphaman, ahatanak.
dexonsmith added a comment.
In https://reviews.llvm.org/D47687#1133222, @Higuoxing wrote:
> I think I am almost there.
>
> Here, In my sight
>
> #define foo(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
>
>
> is un-actionable, because `x op0 y op1 z` are from different arguments of
> function-like macro, so, we will not check parentheses for op0 or op1 when we
> call it by
>
> foo(&&, ||, x, y, z)
>
>
> but if we call it by
>
> foo(&&, ||, x && y ||z, y, z)
>
>
> it is actionable, because argument (x && y || z) is from the same arg
> position of macro foo;
> hence we should check `x && y || z` but shouldn't check parentheses for the
> first argument (&&) and second argument (||)
SGTM!
> I think this could cover bunch of cases. But I think my code is not so
> beautiful... So, is there any suggestion?
I made a couple of comments on the tests, but I'd appreciate someone else
reviewing the code. @arphaman? @ahatanak?
Comment at: test/Sema/logical-op-parentheses-in-macros.c:37
+
+void logical_op_from_macro_arguments2(unsigned i) {
+ macro_op_parentheses_check_ops_args(||, &&, i, i, i); // no
warning.
Please also check that there's no warning with nested macros:
```
#define VOID(cond) (void)(cond)
#define BUILD_VOID(op1, op2, x, y, z) VOID(x op1 y op2 z)
void foo(unsigned i) { BUILD_VOID(&&, ||, i, i, i); }
```
Comment at: test/Sema/logical-op-parentheses-in-macros.c:52
+}
\ No newline at end of file
Phabricator seems to be reporting that there's a missing newline at the end of
the file.
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47946: [ASTmporter] Fix infinite recursion on function import with struct definition in parameters
gerazo updated this revision to Diff 151533.
gerazo added a comment.
Updated to not conflict with and use the stuff implemented in
https://reviews.llvm.org/D47445 (so became a bit smaller)
Now, it is ready for a review. Enjoy!
https://reviews.llvm.org/D47946
Files:
lib/AST/ASTImporter.cpp
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -171,13 +171,34 @@
std::string FileName;
std::unique_ptr Unit;
TranslationUnitDecl *TUDecl = nullptr;
+std::unique_ptr Importer;
+
TU(StringRef Code, StringRef FileName, ArgVector Args)
: Code(Code), FileName(FileName),
Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
this->FileName)),
TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {
Unit->enableSourceFileDiagnostics();
}
+
+void lazyInitImporter(ASTUnit *ToAST) {
+ assert(ToAST);
+ if (!Importer) {
+Importer.reset(new ASTImporter(
+ToAST->getASTContext(), ToAST->getFileManager(),
+Unit->getASTContext(), Unit->getFileManager(), false));
+ }
+}
+
+Decl *import(ASTUnit *ToAST, Decl *FromDecl) {
+ lazyInitImporter(ToAST);
+ return Importer->Import(FromDecl);
+ }
+
+QualType import(ASTUnit *ToAST, QualType FromType) {
+ lazyInitImporter(ToAST);
+ return Importer->Import(FromType);
+}
};
// We may have several From contexts and related translation units. In each
@@ -189,6 +210,28 @@
// vector is expanding, with the list we won't have these issues.
std::list FromTUs;
+ void lazyInitToAST(Language ToLang) {
+if (ToAST)
+ return;
+ArgVector ToArgs = getArgVectorForLanguage(ToLang);
+// Build the AST from an empty file.
+ToAST = tooling::buildASTFromCodeWithArgs(/*Code=*/"", ToArgs, "empty.cc");
+ToAST->enableSourceFileDiagnostics();
+ }
+
+ TU *findFromTU(Decl *From) {
+// Create a virtual file in the To Ctx which corresponds to the file from
+// which we want to import the `From` Decl. Without this source locations
+// will be invalid in the ToCtx.
+auto It = std::find_if(FromTUs.begin(), FromTUs.end(), [From](const TU ) {
+ return E.TUDecl == From->getTranslationUnitDecl();
+});
+assert(It != FromTUs.end());
+assert(ToAST);
+createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
+return &*It;
+ }
+
public:
// We may have several From context but only one To context.
std::unique_ptr ToAST;
@@ -221,14 +264,10 @@
ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
ToAST->enableSourceFileDiagnostics();
-ASTContext = FromTU.Unit->getASTContext(),
-= ToAST->getASTContext();
+ASTContext = FromTU.Unit->getASTContext();
createVirtualFileIfNeeded(ToAST.get(), InputFileName, FromTU.Code);
-ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
- FromTU.Unit->getFileManager(), false);
-
IdentifierInfo *ImportedII = (Identifier);
assert(ImportedII && "Declaration with the given identifier "
"should be specified in test!");
@@ -239,7 +278,7 @@
assert(FoundDecls.size() == 1);
-Decl *Imported = Importer.Import(FoundDecls.front());
+Decl *Imported = FromTU.import(ToAST.get(), FoundDecls.front());
assert(Imported);
return std::make_tuple(*FoundDecls.begin(), Imported);
}
@@ -276,30 +315,17 @@
// May be called several times in a given test.
// The different instances of the param From may have different ASTContext.
Decl *Import(Decl *From, Language ToLang) {
-if (!ToAST) {
- ArgVector ToArgs = getArgVectorForLanguage(ToLang);
- // Build the AST from an empty file.
- ToAST =
- tooling::buildASTFromCodeWithArgs(/*Code=*/"", ToArgs, "empty.cc");
- ToAST->enableSourceFileDiagnostics();
-}
-
-// Create a virtual file in the To Ctx which corresponds to the file from
-// which we want to import the `From` Decl. Without this source locations
-// will be invalid in the ToCtx.
-auto It = std::find_if(FromTUs.begin(), FromTUs.end(), [From](const TU ) {
- return E.TUDecl == From->getTranslationUnitDecl();
-});
-assert(It != FromTUs.end());
-createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
-
-ASTContext = From->getASTContext(),
-= ToAST->getASTContext();
-ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
- FromCtx.getSourceManager().getFileManager(), false);
-return Importer.Import(From);
+lazyInitToAST(ToLang);
+TU *FromTU = findFromTU(From);
+return FromTU->import(ToAST.get(), From);
}
+ QualType ImportType(QualType
r334846 - [X86] The immediate argument to getmantpd*_mask should be an ICE and it should only be 4 bits wide.
Author: ctopper Date: Fri Jun 15 10:03:32 2018 New Revision: 334846 URL: http://llvm.org/viewvc/llvm-project?rev=334846=rev Log: [X86] The immediate argument to getmantpd*_mask should be an ICE and it should only be 4 bits wide. We already checked this for the scalar version, but missed the vector version somehow. Modified: cfe/trunk/include/clang/Basic/BuiltinsX86.def cfe/trunk/lib/Sema/SemaChecking.cpp Modified: cfe/trunk/include/clang/Basic/BuiltinsX86.def URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/BuiltinsX86.def?rev=334846=334845=334846=diff == --- cfe/trunk/include/clang/Basic/BuiltinsX86.def (original) +++ cfe/trunk/include/clang/Basic/BuiltinsX86.def Fri Jun 15 10:03:32 2018 @@ -1696,12 +1696,12 @@ TARGET_BUILTIN(__builtin_ia32_insertf32x TARGET_BUILTIN(__builtin_ia32_inserti32x4_256, "V8iV8iV4iIi", "nc", "avx512vl") TARGET_BUILTIN(__builtin_ia32_insertf32x4, "V16fV16fV4fIi", "nc", "avx512f") TARGET_BUILTIN(__builtin_ia32_inserti32x4, "V16iV16iV4iIi", "nc", "avx512f") -TARGET_BUILTIN(__builtin_ia32_getmantpd128_mask, "V2dV2diV2dUc", "nc", "avx512vl") -TARGET_BUILTIN(__builtin_ia32_getmantpd256_mask, "V4dV4diV4dUc", "nc", "avx512vl") -TARGET_BUILTIN(__builtin_ia32_getmantps128_mask, "V4fV4fiV4fUc", "nc", "avx512vl") -TARGET_BUILTIN(__builtin_ia32_getmantps256_mask, "V8fV8fiV8fUc", "nc", "avx512vl") -TARGET_BUILTIN(__builtin_ia32_getmantpd512_mask, "V8dV8diV8dUcIi", "nc", "avx512f") -TARGET_BUILTIN(__builtin_ia32_getmantps512_mask, "V16fV16fiV16fUsIi", "nc", "avx512f") +TARGET_BUILTIN(__builtin_ia32_getmantpd128_mask, "V2dV2dIiV2dUc", "nc", "avx512vl") +TARGET_BUILTIN(__builtin_ia32_getmantpd256_mask, "V4dV4dIiV4dUc", "nc", "avx512vl") +TARGET_BUILTIN(__builtin_ia32_getmantps128_mask, "V4fV4fIiV4fUc", "nc", "avx512vl") +TARGET_BUILTIN(__builtin_ia32_getmantps256_mask, "V8fV8fIiV8fUc", "nc", "avx512vl") +TARGET_BUILTIN(__builtin_ia32_getmantpd512_mask, "V8dV8dIiV8dUcIi", "nc", "avx512f") +TARGET_BUILTIN(__builtin_ia32_getmantps512_mask, "V16fV16fIiV16fUsIi", "nc", "avx512f") TARGET_BUILTIN(__builtin_ia32_getexppd512_mask, "V8dV8dV8dUcIi", "nc", "avx512f") TARGET_BUILTIN(__builtin_ia32_getexpps512_mask, "V16fV16fV16fUsIi", "nc", "avx512f") TARGET_BUILTIN(__builtin_ia32_vfmaddss3_mask, "V4fV4fV4fV4fUcIi", "nc", "avx512f") Modified: cfe/trunk/lib/Sema/SemaChecking.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=334846=334845=334846=diff == --- cfe/trunk/lib/Sema/SemaChecking.cpp (original) +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Jun 15 10:03:32 2018 @@ -2727,6 +2727,12 @@ bool Sema::CheckX86BuiltinFunctionCall(u case X86::BI__builtin_ia32_roundpd: case X86::BI__builtin_ia32_roundps256: case X86::BI__builtin_ia32_roundpd256: + case X86::BI__builtin_ia32_getmantpd128_mask: + case X86::BI__builtin_ia32_getmantpd256_mask: + case X86::BI__builtin_ia32_getmantps128_mask: + case X86::BI__builtin_ia32_getmantps256_mask: + case X86::BI__builtin_ia32_getmantpd512_mask: + case X86::BI__builtin_ia32_getmantps512_mask: case X86::BI__builtin_ia32_vec_ext_v16qi: case X86::BI__builtin_ia32_vec_ext_v16hi: i = 1; l = 0; u = 15; ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48204: [analyzer] Make getDerefExpr() skip cleanups.
george.karpenkov added inline comments.
Comment at: test/Analysis/inlining/inline-defensive-checks.cpp:93
+S *conjure();
+S *get_conjured(S _) {
+ S *s = conjure();
what is the argument doing?
Repository:
rC Clang
https://reviews.llvm.org/D48204
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48205: [analyzer] Assert that nonloc::SymbolVal always wraps a non-Loc-type symbol.
george.karpenkov accepted this revision. george.karpenkov added inline comments. This revision is now accepted and ready to land. Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1241 if (const llvm::APSInt *I = - SVB.getKnownValue(State, nonloc::SymbolVal(S))) + SVB.getKnownValue(State, SVB.makeSymbolVal(S))) return Loc::isLocType(S->getType()) ? (SVal)SVB.makeIntLocVal(*I) So what is the difference here? That `SVB.makeSymbolVal` is not always a nonloc? Repository: rC Clang https://reviews.llvm.org/D48205 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.
Anastasia accepted this revision. Anastasia added a comment. This revision is now accepted and ready to land. LGTM! Thanks! https://reviews.llvm.org/D47630 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48224: Don't let test/Driver/no-canonical-prefixes.c form a symlink cycle the second time it runs.
thakis created this revision. thakis added a reviewer: chandlerc. The test makes %t.fake a symlink to %t.real by running `ln -sf %t.real %t.fake`. If %t.fake already is a symlink to %t.real when this runs (e.g. if the test has run before), then this effectively becomes `ln -sf %t.real %t.real`, symlinking the directory to itself. At least on my mac, this leads to the directory containing itself. As fix, just remove %t.fake before creating the symlink. To clean up build dirs on bots, also remove %t.real for a while. https://reviews.llvm.org/D48224 Files: test/Driver/no-canonical-prefixes.c Index: test/Driver/no-canonical-prefixes.c === --- test/Driver/no-canonical-prefixes.c +++ test/Driver/no-canonical-prefixes.c @@ -1,9 +1,14 @@ // Due to ln -sf: // REQUIRES: shell +// RUN: rm -rf %t.real // RUN: mkdir -p %t.real // RUN: cd %t.real // RUN: ln -sf %clang test-clang // RUN: cd .. +// Important to remove %t.fake: If it already is a symlink to %t.real when +// `ln -sf %t.real %t.fake` runs, then that would symlink %t.real to itself, +// forming a cycle. +// RUN: rm -rf %t.fake // RUN: ln -sf %t.real %t.fake // RUN: cd %t.fake // RUN: ./test-clang -v -S %s 2>&1 | FileCheck --check-prefix=CANONICAL %s Index: test/Driver/no-canonical-prefixes.c === --- test/Driver/no-canonical-prefixes.c +++ test/Driver/no-canonical-prefixes.c @@ -1,9 +1,14 @@ // Due to ln -sf: // REQUIRES: shell +// RUN: rm -rf %t.real // RUN: mkdir -p %t.real // RUN: cd %t.real // RUN: ln -sf %clang test-clang // RUN: cd .. +// Important to remove %t.fake: If it already is a symlink to %t.real when +// `ln -sf %t.real %t.fake` runs, then that would symlink %t.real to itself, +// forming a cycle. +// RUN: rm -rf %t.fake // RUN: ln -sf %t.real %t.fake // RUN: cd %t.fake // RUN: ./test-clang -v -S %s 2>&1 | FileCheck --check-prefix=CANONICAL %s ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r334837 - [NFC] Add CreateMemTempWithoutCast and CreateTempAllocaWithoutCast
Author: yaxunl
Date: Fri Jun 15 08:33:22 2018
New Revision: 334837
URL: http://llvm.org/viewvc/llvm-project?rev=334837=rev
Log:
[NFC] Add CreateMemTempWithoutCast and CreateTempAllocaWithoutCast
This is partial re-commit of r332982
Modified:
cfe/trunk/lib/CodeGen/CGCall.cpp
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.h
Modified: cfe/trunk/lib/CodeGen/CGCall.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCall.cpp?rev=334837=334836=334837=diff
==
--- cfe/trunk/lib/CodeGen/CGCall.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGCall.cpp Fri Jun 15 08:33:22 2018
@@ -3901,9 +3901,8 @@ RValue CodeGenFunction::EmitCall(const C
assert(NumIRArgs == 1);
if (!I->isAggregate()) {
// Make a temporary alloca to pass the argument.
-Address Addr = CreateMemTemp(I->Ty, ArgInfo.getIndirectAlign(),
- "indirect-arg-temp", /*Alloca=*/nullptr,
- /*Cast=*/false);
+Address Addr = CreateMemTempWithoutCast(
+I->Ty, ArgInfo.getIndirectAlign(), "indirect-arg-temp");
IRCallArgs[FirstIRArg] = Addr.getPointer();
I->copyInto(*this, Addr);
@@ -3948,9 +3947,8 @@ RValue CodeGenFunction::EmitCall(const C
}
if (NeedCopy) {
// Create an aligned temporary, and copy to it.
- Address AI = CreateMemTemp(I->Ty, ArgInfo.getIndirectAlign(),
- "byval-temp", /*Alloca=*/nullptr,
- /*Cast=*/false);
+ Address AI = CreateMemTempWithoutCast(
+ I->Ty, ArgInfo.getIndirectAlign(), "byval-temp");
IRCallArgs[FirstIRArg] = AI.getPointer();
I->copyInto(*this, AI);
} else {
Modified: cfe/trunk/lib/CodeGen/CGExpr.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExpr.cpp?rev=334837=334836=334837=diff
==
--- cfe/trunk/lib/CodeGen/CGExpr.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp Fri Jun 15 08:33:22 2018
@@ -61,21 +61,30 @@ llvm::Value *CodeGenFunction::EmitCastTo
/// CreateTempAlloca - This creates a alloca and inserts it into the entry
/// block.
+Address CodeGenFunction::CreateTempAllocaWithoutCast(llvm::Type *Ty,
+ CharUnits Align,
+ const Twine ,
+ llvm::Value *ArraySize) {
+ auto Alloca = CreateTempAlloca(Ty, Name, ArraySize);
+ Alloca->setAlignment(Align.getQuantity());
+ return Address(Alloca, Align);
+}
+
+/// CreateTempAlloca - This creates a alloca and inserts it into the entry
+/// block. The alloca is casted to default address space if necessary.
Address CodeGenFunction::CreateTempAlloca(llvm::Type *Ty, CharUnits Align,
const Twine ,
llvm::Value *ArraySize,
- Address *AllocaAddr,
- bool CastToDefaultAddrSpace) {
- auto Alloca = CreateTempAlloca(Ty, Name, ArraySize);
- Alloca->setAlignment(Align.getQuantity());
+ Address *AllocaAddr) {
+ auto Alloca = CreateTempAllocaWithoutCast(Ty, Align, Name, ArraySize);
if (AllocaAddr)
-*AllocaAddr = Address(Alloca, Align);
- llvm::Value *V = Alloca;
+*AllocaAddr = Alloca;
+ llvm::Value *V = Alloca.getPointer();
// Alloca always returns a pointer in alloca address space, which may
// be different from the type defined by the language. For example,
// in C++ the auto variables are in the default address space. Therefore
// cast alloca to the default address space when necessary.
- if (CastToDefaultAddrSpace && getASTAllocaAddressSpace() != LangAS::Default)
{
+ if (getASTAllocaAddressSpace() != LangAS::Default) {
auto DestAddrSpace = getContext().getTargetAddressSpace(LangAS::Default);
llvm::IRBuilderBase::InsertPointGuard IPG(Builder);
// When ArraySize is nullptr, alloca is inserted at AllocaInsertPt,
@@ -128,19 +137,26 @@ Address CodeGenFunction::CreateIRTemp(Qu
}
Address CodeGenFunction::CreateMemTemp(QualType Ty, const Twine ,
- Address *Alloca,
- bool CastToDefaultAddrSpace) {
+ Address *Alloca) {
// FIXME: Should we prefer the preferred type alignment here?
- return CreateMemTemp(Ty, getContext().getTypeAlignInChars(Ty), Name, Alloca,
- CastToDefaultAddrSpace);
+ return CreateMemTemp(Ty, getContext().getTypeAlignInChars(Ty), Name, Alloca);
}
Address CodeGenFunction::CreateMemTemp(QualType Ty, CharUnits Align,
-
[PATCH] D48151: [CUDA] Make __host__/__device__ min/max overloads constexpr in C++14.
jlebar added a comment. @rsmith friendly ping on this review. https://reviews.llvm.org/D48151 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48036: [CUDA] Make min/max shims host+device.
jlebar added a comment. @rsmith friendly ping on this one. https://reviews.llvm.org/D48036 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48188: [SPIR] Prevent SPIR targets from using half conversion intrinsics
smcgro updated this revision to Diff 151512.
https://reviews.llvm.org/D48188
Files:
lib/Basic/Targets/SPIR.h
test/CodeGen/spir-half-type.cpp
Index: test/CodeGen/spir-half-type.cpp
===
--- /dev/null
+++ test/CodeGen/spir-half-type.cpp
@@ -0,0 +1,143 @@
+// RUN: %clang_cc1 -O0 -triple spir -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -O0 -triple spir64 -emit-llvm %s -o - | FileCheck %s
+
+// This file tests that using the _Float16 type with the spir target will not
+// use the llvm intrinsics but instead will use the half arithmetic
+// instructions directly.
+
+// Previously attempting to use a constant _Float16 with a comparison
+// instruction when the target is spir or spir64 lead to an assert being hit.
+bool fcmp_const() {
+ _Float16 a = 0.0f16;
+ const _Float16 b = 1.0f16;
+
+ // CHECK-NOT: llvm.convert.to.fp16
+ // CHECK-NOT: llvm.convert.from.fp16
+
+ // CHECK: [[REG1:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: fcmp olt half [[REG1]], 0xH3C00
+
+ // CHECK: [[REG2:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: fcmp olt half [[REG2]], 0xH4000
+
+ // CHECK: [[REG3:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: fcmp ogt half [[REG3]], 0xH3C00
+
+ // CHECK: [[REG4:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: fcmp ogt half [[REG4]], 0xH4200
+
+ // CHECK: [[REG5:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: fcmp oeq half [[REG5]], 0xH3C00
+
+ // CHECK: [[REG7:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: fcmp oeq half [[REG7]], 0xH4400
+
+ // CHECK: [[REG8:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: fcmp une half [[REG8]], 0xH3C00
+
+ // CHECK: [[REG9:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: fcmp une half [[REG9]], 0xH4500
+
+ // CHECK: [[REG10:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: fcmp ole half [[REG10]], 0xH3C00
+
+ // CHECK: [[REG11:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: fcmp ole half [[REG11]], 0xH4600
+
+ // CHECK: [[REG12:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: fcmp oge half [[REG12]], 0xH3C00
+
+ // CHECK: [[REG13:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: fcmp oge half [[REG13]], 0xH4700
+ return a < b || a < 2.0f16 || a > b || a > 3.0f16 || a == b || a == 4.0f16 ||
+ a != b || a != 5.0f16 || a <= b || a <= 6.0f16 || a >= b ||
+ a >= 7.0f16;
+}
+
+bool fcmp() {
+ _Float16 a = 0.0f16;
+ _Float16 b = 1.0f16;
+
+ // CHECK-NOT: llvm.convert.to.fp16
+ // CHECK-NOT: llvm.convert.from.fp16
+ // CHECK: [[REG1:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: [[REG2:%.*]] = load half, half* %b, align 2
+ // CHECK-NEXT: fcmp olt half [[REG1]], [[REG2]]
+
+ // CHECK: [[REG3:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: [[REG4:%.*]] = load half, half* %b, align 2
+ // CHECK-NEXT: fcmp ogt half [[REG3]], [[REG4]]
+
+ // CHECK: [[REG5:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: [[REG6:%.*]] = load half, half* %b, align 2
+ // CHECK-NEXT: fcmp oeq half [[REG5]], [[REG6]]
+
+ // CHECK: [[REG7:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: [[REG8:%.*]] = load half, half* %b, align 2
+ // CHECK-NEXT: fcmp une half [[REG7]], [[REG8]]
+
+ // CHECK: [[REG7:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: [[REG8:%.*]] = load half, half* %b, align 2
+ // CHECK-NEXT: fcmp ole half [[REG7]], [[REG8]]
+
+ // CHECK: [[REG7:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: [[REG8:%.*]] = load half, half* %b, align 2
+ // CHECK-NEXT: fcmp oge half [[REG7]], [[REG8]]
+ return a < b || a > b || a == b || a != b || a <= b || a >= b;
+}
+
+_Float16 fadd() {
+ _Float16 a = 1.0f16;
+ const _Float16 b = 2.0f16;
+
+ // CHECK-NOT: llvm.convert.to.fp16
+ // CHECK-NOT: llvm.convert.from.fp16
+
+ // CHECK: [[REG1:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: [[REG2:%.*]] = fadd half [[REG1]], 0xH4000
+ // CHECK-NEXT: [[REG3:%.*]] = fadd half [[REG2]], 0xH4200
+ // CHECK-NEXT: ret half [[REG3]]
+ return a + b + 3.0f16;
+}
+
+_Float16 fsub() {
+ _Float16 a = 1.0f16;
+ const _Float16 b = 2.0f16;
+
+ // CHECK-NOT: llvm.convert.to.fp16
+ // CHECK-NOT: llvm.convert.from.fp16
+
+ // CHECK: [[REG1:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: [[REG2:%.*]] = fsub half [[REG1]], 0xH4000
+ // CHECK-NEXT: [[REG3:%.*]] = fsub half [[REG2]], 0xH4200
+ // CHECK-NEXT: ret half [[REG3]]
+ return a - b - 3.0f16;
+}
+
+_Float16 fmul() {
+ _Float16 a = 1.0f16;
+ const _Float16 b = 2.0f16;
+
+ // CHECK-NOT: llvm.convert.to.fp16
+ // CHECK-NOT: llvm.convert.from.fp16
+
+ // CHECK: [[REG1:%.*]] = load half, half* %a, align 2
+ // CHECK-NEXT: [[REG2:%.*]] = fmul half [[REG1]], 0xH4000
+ // CHECK-NEXT: [[REG3:%.*]] = fmul half [[REG2]], 0xH4200
+ // CHECK-NEXT: ret half [[REG3]]
+ return a * b * 3.0f16;
+}
+
+_Float16 fdiv() {
+ _Float16 a = 1.0f16;
+ const _Float16 b = 2.0f16;
+
+ // CHECK-NOT:
[PATCH] D48188: [SPIR] Prevent SPIR targets from using half conversion intrinsics
smcgro added a comment.
Thanks for the feedback @SjoerdMeijer I've updated the commit message and
responded to the comments
Comment at: lib/Basic/Targets/SPIR.h:50
UseAddrSpaceMapMangling = true;
+HasLegalHalfType = true;
// Define available target features
SjoerdMeijer wrote:
> It doesn't hurt to set this, but you're not using it so you could omit it. I
> had to introduce this to deal differently with half types depending on
> architecture extensions, but don't you think have this problem.
Yeah that's true that it doesn't affect the assert being hit, I included it as
it is true for the SPIR target as the SPIR targets //do// have a legal half
type which is brings the SPIR target file in line with what the specification
says. If this is problematic or it needs a different set of tests I can remove
it
Comment at: lib/Basic/Targets/SPIR.h:65
+ // memcpy as per section 3 of the SPIR spec.
+ bool useFP16ConversionIntrinsics() const override { return false; }
+
SjoerdMeijer wrote:
> just a note: this is the only functional change, but you're testing a lot
> more in test/CodeGen/spir-half-type.cpp
That's true that the fix is only for the assert being hit when attempting to
emit comparison operators. The additional tests I added are mainly to prevent
regressions in the future since the cross section of the _Float16 C/C++ type
and the SPIR target doesn't have any tests at the moment so we'd like to be
sure that it will always be compiled correctly. In addition it helps make sure
that this change doesn't have any unintended consequences with any of the other
operators.
Comment at: test/CodeGen/spir-half-type.cpp:3
+// RUN: %clang_cc1 -O0 -triple spir64 -emit-llvm %s -o - | FileCheck %s
+
+// This file tests that using the _Float16 type with the spir target will not
use the llvm intrinsics but instead will use the half arithmetic instructions
directly.
SjoerdMeijer wrote:
> I think you need one reproducer to test:
>
> // CHECK-NOT: llvm.convert.from.fp16
>
> The other tests, like all the compares are valid tests, but not related to
> this change, and also not specific to SPIR. I put my _Float16 "smoke tests"
> in test/CodeGenCXX/float16-declarations.cpp, perhaps you can move some of
> these generic tests there because I for example see I didn't add any compares
> there.
My thought process was that we should have a test just for this _Float16/SPIR
cross section as that is where this issue emerged from. These are specific to
SPIR insofar that the comparison operators previously wouldn't compile (on
Debug or Release with asserts builds) so their inclusion is necessary to check
that this now compiles. As I mentioned above the others are there to prevent
future regressions on this target and to check that this change doesn't have
any unintended side effects with the other operators. I agree that the smoke
tests should have the operator tests but we do need these operations to be
tested on the SPIR target as well to make sure they compile. Perhaps including
these operations in those float16 specific regression tests should be a
separate issue however, as it unrelated to this SPIR change?
Comment at: test/CodeGen/spir-half-type.cpp:4
+
+// This file tests that using the _Float16 type with the spir target will not
use the llvm intrinsics but instead will use the half arithmetic instructions
directly.
+
SjoerdMeijer wrote:
> nit: this comment exceeds 80 columns, same for the other comment below.
Fixed now, thanks
Repository:
rC Clang
https://reviews.llvm.org/D48188
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r334829 - [clang-tidy] This patch is a fix for D45405 where spaces were mistakenly considered as a part of a type name. So length("int *") was 5 instead of 3 with RemoveStars=0 or
Author: zinovy.nis
Date: Fri Jun 15 06:35:40 2018
New Revision: 334829
URL: http://llvm.org/viewvc/llvm-project?rev=334829=rev
Log:
[clang-tidy] This patch is a fix for D45405 where spaces were mistakenly
considered as a part of a type name. So length("int *") was 5 instead of 3 with
RemoveStars=0 or 4 with RemoveStars=1
Differential Revision: https://reviews.llvm.org/D45927
Modified:
clang-tools-extra/trunk/clang-tidy/modernize/UseAutoCheck.cpp
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-auto.rst
clang-tools-extra/trunk/test/clang-tidy/modernize-use-auto-min-type-name-length.cpp
Modified: clang-tools-extra/trunk/clang-tidy/modernize/UseAutoCheck.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/UseAutoCheck.cpp?rev=334829=334828=334829=diff
==
--- clang-tools-extra/trunk/clang-tidy/modernize/UseAutoCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseAutoCheck.cpp Fri Jun 15
06:35:40 2018
@@ -11,6 +11,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/CharInfo.h"
#include "clang/Tooling/FixIt.h"
using namespace clang;
@@ -27,6 +28,34 @@ const char DeclWithNewId[] = "decl_new";
const char DeclWithCastId[] = "decl_cast";
const char DeclWithTemplateCastId[] = "decl_template";
+size_t GetTypeNameLength(bool RemoveStars, StringRef Text) {
+ enum CharType { Space, Alpha, Punctuation };
+ CharType LastChar = Space, BeforeSpace = Punctuation;
+ size_t NumChars = 0;
+ int TemplateTypenameCntr = 0;
+ for (const unsigned char C : Text) {
+if (C == '<')
+ ++TemplateTypenameCntr;
+else if (C == '>')
+ --TemplateTypenameCntr;
+const CharType NextChar =
+isAlphanumeric(C)
+? Alpha
+: (isWhitespace(C) ||
+ (!RemoveStars && TemplateTypenameCntr == 0 && C == '*'))
+ ? Space
+ : Punctuation;
+if (NextChar != Space) {
+ ++NumChars; // Count the non-space character.
+ if (LastChar == Space && NextChar == Alpha && BeforeSpace == Alpha)
+++NumChars; // Count a single space character between two words.
+ BeforeSpace = NextChar;
+}
+LastChar = NextChar;
+ }
+ return NumChars;
+}
+
/// \brief Matches variable declarations that have explicit initializers that
/// are not initializer lists.
///
@@ -419,8 +448,10 @@ void UseAutoCheck::replaceExpr(
SourceRange Range(Loc.getSourceRange());
if (MinTypeNameLength != 0 &&
- tooling::fixit::getText(Loc.getSourceRange(), FirstDecl->getASTContext())
- .size() < MinTypeNameLength)
+ GetTypeNameLength(RemoveStars,
+tooling::fixit::getText(Loc.getSourceRange(),
+FirstDecl->getASTContext())) <
+ MinTypeNameLength)
return;
auto Diag = diag(Range.getBegin(), Message);
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-auto.rst
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-auto.rst?rev=334829=334828=334829=diff
==
--- clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-auto.rst
(original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-auto.rst Fri
Jun 15 06:35:40 2018
@@ -182,20 +182,37 @@ Options
If the option is set to non-zero (default `5`), the check will ignore type
names having a length less than the option value. The option affects
expressions only, not iterators.
+ Spaces between multi-lexeme type names (``long int``) are considered as one.
+ If ``RemoveStars`` option (see below) is set to non-zero, then ``*s`` in
+ the type are also counted as a part of the type name.
.. code-block:: c++
- // MinTypeNameLength = 0
+ // MinTypeNameLength = 0, RemoveStars=0
int a = static_cast(foo());// ---> auto a = ...
- bool b = new bool; // ---> auto b = ...
+ // length(bool *) = 4
+ bool *b = new bool; // ---> auto *b = ...
unsigned c = static_cast(foo()); // ---> auto c = ...
- // MinTypeNameLength = 8
+ // MinTypeNameLength = 5, RemoveStars=0
- int a = static_cast(foo());// ---> int a = ...
- bool b = new bool; // ---> bool b = ...
- unsigned c = static_cast(foo()); // ---> auto c = ...
+ int a = static_cast(foo()); // ---> int a = ...
+ bool b = static_cast(foo()); // ---> bool b = ...
+ bool *pb = static_cast(foo());// ---> bool *pb = ...
+ unsigned c = static_cast(foo()); // ---> auto c = ...
+ // length(long int) = 8
+ long int d = static_cast(foo()); // ---> auto d =
[PATCH] D45927: [clang-tidy] [modernize-use-auto] Correct way to calculate a type name length for multi-token types
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE334829: [clang-tidy] This patch is a fix for D45405 where
spaces were mistakenly… (authored by zinovy.nis, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D45927?vs=146677=151500#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D45927
Files:
clang-tidy/modernize/UseAutoCheck.cpp
docs/clang-tidy/checks/modernize-use-auto.rst
test/clang-tidy/modernize-use-auto-min-type-name-length.cpp
Index: test/clang-tidy/modernize-use-auto-min-type-name-length.cpp
===
--- test/clang-tidy/modernize-use-auto-min-type-name-length.cpp
+++ test/clang-tidy/modernize-use-auto-min-type-name-length.cpp
@@ -1,30 +1,85 @@
-// RUN: %check_clang_tidy %s modernize-use-auto %t -- \
-// RUN: -config="{CheckOptions: [{key: modernize-use-auto.MinTypeNameLength, value: '5'}]}" \
-// RUN: -- -std=c++11 -frtti
-
-extern int foo();
-
-using VeryVeryVeryLongTypeName = int;
+// RUN: %check_clang_tidy -check-suffix=0-0 %s modernize-use-auto %t -- -config="{CheckOptions: [{key: modernize-use-auto.RemoveStars, value: 0}, {key: modernize-use-auto.MinTypeNameLength, value: 0}]}" -- --std=c++11 -frtti
+// RUN: %check_clang_tidy -check-suffix=0-8 %s modernize-use-auto %t -- -config="{CheckOptions: [{key: modernize-use-auto.RemoveStars, value: 0}, {key: modernize-use-auto.MinTypeNameLength, value: 8}]}" -- --std=c++11 -frtti
+// RUN: %check_clang_tidy -check-suffix=1-0 %s modernize-use-auto %t -- -config="{CheckOptions: [{key: modernize-use-auto.RemoveStars, value: 1}, {key: modernize-use-auto.MinTypeNameLength, value: 0}]}" -- --std=c++11 -frtti
+// RUN: %check_clang_tidy -check-suffix=1-8 %s modernize-use-auto %t -- -config="{CheckOptions: [{key: modernize-use-auto.RemoveStars, value: 1}, {key: modernize-use-auto.MinTypeNameLength, value: 8}]}" -- --std=c++11 -frtti
+
+template extern T foo();
+template struct P { explicit P(T t) : t_(t) {} T t_;};
+template P *foo_ptr();
+template P _ref();
int bar() {
- int a = static_cast(foo());
- // strlen('int') = 4 < 5, so skip it,
- // even strlen('VeryVeryVeryLongTypeName') > 5.
-
- unsigned b = static_cast(foo());
- // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
- // CHECK-FIXES: auto b = static_cast(foo());
-
- bool c = static_cast(foo());
- // strlen('bool') = 4 < 5, so skip it.
-
- const bool c1 = static_cast(foo());
- // strlen('bool') = 4 < 5, so skip it, even there's a 'const'.
-
- unsigned long long ull = static_cast(foo());
- // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: use auto when initializing with a cast to avoid duplicating the type name [modernize-use-auto]
- // CHECK-FIXES: auto ull = static_cast(foo());
+ {
+// Lenth(long) = 4
+long i = static_cast(foo());
+// CHECK-FIXES-0-0: auto i = {{.*}}
+// CHECK-FIXES-0-8: long i = {{.*}}
+// CHECK-FIXES-1-0: auto i = {{.*}}
+// CHECK-FIXES-1-8: long i = {{.*}}
+const long ci = static_cast(foo());
+// CHECK-FIXES-0-0: auto ci = {{.*}}
+// CHECK-FIXES-0-8: long ci = {{.*}}
+// CHECK-FIXES-1-0: auto ci = {{.*}}
+// CHECK-FIXES-1-8: long ci = {{.*}}
+long *pi = static_cast(foo());
+// CHECK-FIXES-0-0: auto *pi = {{.*}}
+// CHECK-FIXES-0-8: long *pi = {{.*}}
+// CHECK-FIXES-1-0: auto pi = {{.*}}
+// CHECK-FIXES-1-8: long *pi = {{.*}}
+
+// Length(long *) is still 5
+long * pi2 = static_cast(foo());
+// CHECK-FIXES-0-0: auto * pi2 = {{.*}}
+// CHECK-FIXES-0-8: long * pi2 = {{.*}}
+// CHECK-FIXES-1-0: auto pi2 = {{.*}}
+// CHECK-FIXES-1-8: long * pi2 = {{.*}}
+
+// Length(long **) = 6
+long **ppi = static_cast(foo());
+// CHECK-FIXES-0-0: auto **ppi = {{.*}}
+// CHECK-FIXES-0-8: long **ppi = {{.*}}
+// CHECK-FIXES-1-0: auto ppi = {{.*}}
+// CHECK-FIXES-1-8: long **ppi = {{.*}}
+ }
+
+ {
+// Lenth(long int) = 4 + 1 + 3 = 8
+// Lenth(longint) is still 8
+long int i = static_cast(foo());
+// CHECK-FIXES-0-0: auto i = {{.*}}
+// CHECK-FIXES-0-8: auto i = {{.*}}
+// CHECK-FIXES-1-0: auto i = {{.*}}
+// CHECK-FIXES-1-8: auto i = {{.*}}
+
+long int *pi = static_cast(foo());
+// CHECK-FIXES-0-0: auto *pi = {{.*}}
+// CHECK-FIXES-0-8: auto *pi = {{.*}}
+// CHECK-FIXES-1-0: auto pi = {{.*}}
+// CHECK-FIXES-1-8: auto pi = {{.*}}
+ }
+
+ // Templates
+ {
+// Length(P) = 7
+P& i = static_cast&>(foo_ref());
+// CHECK-FIXES-0-0: auto& i = {{.*}}
+// CHECK-FIXES-0-8: P& i = {{.*}}
+// CHECK-FIXES-1-0: auto & i = {{.*}}
+// CHECK-FIXES-1-8: P& i = {{.*}}
+
+// Length(P) = 8
+P& pi = static_cast &>(foo_ref());
+// CHECK-FIXES-0-0: auto& pi = {{.*}}
+// CHECK-FIXES-0-8: auto& pi = {{.*}}
+
[PATCH] D48163: [clangd] UI for completion items that would trigger include insertion.
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE334828: [clangd] UI for completion items that would
trigger include insertion. (authored by ioeric, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D48163?vs=151498=151499#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48163
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/Headers.cpp
clangd/Headers.h
test/clangd/completion-snippets.test
test/clangd/completion.test
test/clangd/protocol.test
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/HeadersTests.cpp
Index: unittests/clangd/HeadersTests.cpp
===
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -78,10 +78,10 @@
return Inclusions;
}
- // Calculates the include path, or returns "" on error.
+ // Calculates the include path, or returns "" on error or header should not be
+ // inserted.
std::string calculate(PathRef Original, PathRef Preferred = "",
-const std::vector = {},
-bool ExpectError = false) {
+const std::vector = {}) {
auto Clang = setupClang();
PreprocessOnlyAction Action;
EXPECT_TRUE(
@@ -94,36 +94,30 @@
/*Verbatim=*/!llvm::sys::path::is_absolute(Header)};
};
-auto Path = calculateIncludePath(
-MainFile, CDB.getCompileCommand(MainFile)->Directory,
-Clang->getPreprocessor().getHeaderSearchInfo(), Inclusions,
-ToHeaderFile(Original), ToHeaderFile(Preferred));
+IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
+ CDB.getCompileCommand(MainFile)->Directory,
+ Clang->getPreprocessor().getHeaderSearchInfo());
+for (const auto : Inclusions)
+ Inserter.addExisting(Inc);
+auto Declaring = ToHeaderFile(Original);
+auto Inserted = ToHeaderFile(Preferred);
+if (!Inserter.shouldInsertInclude(Declaring, Inserted))
+ return "";
+std::string Path = Inserter.calculateIncludePath(Declaring, Inserted);
Action.EndSourceFile();
-if (!Path) {
- llvm::consumeError(Path.takeError());
- EXPECT_TRUE(ExpectError);
- return std::string();
-} else {
- EXPECT_FALSE(ExpectError);
-}
-return std::move(*Path);
+return Path;
}
- Expected>
- insert(const HeaderFile , const HeaderFile ,
- const std::vector = {}) {
+ Optional insert(StringRef VerbatimHeader) {
auto Clang = setupClang();
PreprocessOnlyAction Action;
EXPECT_TRUE(
Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
CDB.getCompileCommand(MainFile)->Directory,
Clang->getPreprocessor().getHeaderSearchInfo());
-for (const auto : ExistingInclusions)
- Inserter.addExisting(Inc);
-
-auto Edit = Inserter.insert(DeclaringHeader, InsertedHeader);
+auto Edit = Inserter.insert(VerbatimHeader);
Action.EndSourceFile();
return Edit;
}
@@ -220,31 +214,10 @@
EXPECT_EQ(calculate(BarHeader, "\"BAR.h\"", Inclusions), "");
}
-HeaderFile literal(StringRef Include) {
- HeaderFile H{Include, /*Verbatim=*/true};
- assert(H.valid());
- return H;
-}
-
TEST_F(HeadersTest, PreferInserted) {
- auto Edit = insert(literal(""), literal(""));
- EXPECT_TRUE(static_cast(Edit));
- EXPECT_TRUE(llvm::StringRef((*Edit)->newText).contains(""));
-}
-
-TEST_F(HeadersTest, ExistingInclusion) {
- Inclusion Existing{Range(), /*Written=*/"", /*Resolved=*/""};
- auto Edit = insert(literal(""), literal(""), {Existing});
- EXPECT_TRUE(static_cast(Edit));
- EXPECT_FALSE(static_cast(*Edit));
-}
-
-TEST_F(HeadersTest, ValidateHeaders) {
- HeaderFile InvalidHeader{"a.h", /*Verbatim=*/true};
- assert(!InvalidHeader.valid());
- auto Edit = insert(InvalidHeader, literal("\"c.h\""));
- EXPECT_FALSE(static_cast(Edit));
- llvm::consumeError(Edit.takeError());
+ auto Edit = insert("");
+ EXPECT_TRUE(Edit.hasValue());
+ EXPECT_TRUE(llvm::StringRef(Edit->newText).contains(""));
}
} // namespace
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -44,7 +44,19 @@
// GMock helpers for matching completion items.
MATCHER_P(Named, Name, "") { return arg.insertText == Name; }
-MATCHER_P(Labeled, Label, "") { return arg.label == Label; }
+MATCHER_P(Labeled, Label, "") {
+ std::string Indented;
+ if (!StringRef(Label).startswith(
+ CodeCompleteOptions().IncludeIndicator.Insert) &&
+ !StringRef(Label).startswith(
+ CodeCompleteOptions().IncludeIndicator.NoInsert))
+Indented
[clang-tools-extra] r334828 - [clangd] UI for completion items that would trigger include insertion.
Author: ioeric
Date: Fri Jun 15 06:34:18 2018
New Revision: 334828
URL: http://llvm.org/viewvc/llvm-project?rev=334828=rev
Log:
[clangd] UI for completion items that would trigger include insertion.
Summary:
For completion items that would trigger include insertions (i.e. index symbols
that are not #included yet), add a visual indicator "+" before the completion
label. The inserted headers will appear in the completion detail.
Open to suggestions for better visual indicators; "+" was picked because it
seems cleaner than a few other candidates I've tried (*, #, @ ...).
The displayed header would be like a/b/c.h (without quote) or for
system
headers. I didn't add quotation or "#include" because they can take up limited
space and do not provide additional information after users know what the
headers are. I think a header alone should be obvious for users to infer that
this is an include header..
To align indentation, also prepend ' ' to labels of candidates that would not
trigger include insertions (only for completions where index results are
possible).
Vim:
{F6357587}
vscode:
{F6357589}
{F6357591}
Reviewers: sammccall, ilya-biryukov, hokein
Reviewed By: sammccall
Subscribers: MaskRay, jkorous, cfe-commits
Differential Revision: https://reviews.llvm.org/D48163
Modified:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/CodeComplete.h
clang-tools-extra/trunk/clangd/Headers.cpp
clang-tools-extra/trunk/clangd/Headers.h
clang-tools-extra/trunk/test/clangd/completion-snippets.test
clang-tools-extra/trunk/test/clangd/completion.test
clang-tools-extra/trunk/test/clangd/protocol.test
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
clang-tools-extra/trunk/unittests/clangd/HeadersTests.cpp
Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=334828=334827=334828=diff
==
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Fri Jun 15 06:34:18 2018
@@ -240,10 +240,11 @@ struct CompletionCandidate {
CompletionItem build(StringRef FileName, const CompletionItemScores ,
const CodeCompleteOptions ,
CodeCompletionString *SemaCCS,
- const IncludeInserter *Includes,
+ const IncludeInserter ,
llvm::StringRef SemaDocComment) const {
assert(bool(SemaResult) == bool(SemaCCS));
CompletionItem I;
+bool InsertingInclude = false; // Whether a new #include will be added.
if (SemaResult) {
I.kind = toCompletionItemKind(SemaResult->Kind, SemaResult->Declaration);
getLabelAndInsertText(*SemaCCS, , ,
@@ -273,7 +274,7 @@ struct CompletionCandidate {
if (I.detail.empty())
I.detail = D->CompletionDetail;
if (auto Inserted = headerToInsertIfNotPresent()) {
- auto Edit = [&]() -> Expected> {
+ auto IncludePath = [&]() -> Expected {
auto ResolvedDeclaring = toHeaderFile(
IndexResult->CanonicalDeclaration.FileURI, FileName);
if (!ResolvedDeclaring)
@@ -281,9 +282,13 @@ struct CompletionCandidate {
auto ResolvedInserted = toHeaderFile(*Inserted, FileName);
if (!ResolvedInserted)
return ResolvedInserted.takeError();
-return Includes->insert(*ResolvedDeclaring, *ResolvedInserted);
+if (!Includes.shouldInsertInclude(*ResolvedDeclaring,
+ *ResolvedInserted))
+ return "";
+return Includes.calculateIncludePath(*ResolvedDeclaring,
+ *ResolvedInserted);
}();
- if (!Edit) {
+ if (!IncludePath) {
std::string ErrMsg =
("Failed to generate include insertion edits for adding header
"
"(FileURI=\"" +
@@ -291,13 +296,22 @@ struct CompletionCandidate {
"\", IncludeHeader=\"" + D->IncludeHeader + "\") into " +
FileName)
.str();
-log(ErrMsg + ":" + llvm::toString(Edit.takeError()));
- } else if (*Edit) {
-I.additionalTextEdits = {std::move(**Edit)};
+log(ErrMsg + ":" + llvm::toString(IncludePath.takeError()));
+ } else if (!IncludePath->empty()) {
+// FIXME: consider what to show when there is no #include
insertion,
+// and for sema results, for consistency.
+if (auto Edit = Includes.insert(*IncludePath)) {
+ I.detail += ("\n" + StringRef(*IncludePath).trim('"')).str();
+ I.additionalTextEdits = {std::move(*Edit)};
+ InsertingInclude = true;
+}
[PATCH] D48163: [clangd] UI for completion items that would trigger include insertion.
ioeric updated this revision to Diff 151498.
ioeric added a comment.
- clang-format again...
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48163
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/Headers.cpp
clangd/Headers.h
test/clangd/completion-snippets.test
test/clangd/completion.test
test/clangd/protocol.test
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/HeadersTests.cpp
Index: unittests/clangd/HeadersTests.cpp
===
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -78,10 +78,10 @@
return Inclusions;
}
- // Calculates the include path, or returns "" on error.
+ // Calculates the include path, or returns "" on error or header should not be
+ // inserted.
std::string calculate(PathRef Original, PathRef Preferred = "",
-const std::vector = {},
-bool ExpectError = false) {
+const std::vector = {}) {
auto Clang = setupClang();
PreprocessOnlyAction Action;
EXPECT_TRUE(
@@ -94,36 +94,30 @@
/*Verbatim=*/!llvm::sys::path::is_absolute(Header)};
};
-auto Path = calculateIncludePath(
-MainFile, CDB.getCompileCommand(MainFile)->Directory,
-Clang->getPreprocessor().getHeaderSearchInfo(), Inclusions,
-ToHeaderFile(Original), ToHeaderFile(Preferred));
+IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
+ CDB.getCompileCommand(MainFile)->Directory,
+ Clang->getPreprocessor().getHeaderSearchInfo());
+for (const auto : Inclusions)
+ Inserter.addExisting(Inc);
+auto Declaring = ToHeaderFile(Original);
+auto Inserted = ToHeaderFile(Preferred);
+if (!Inserter.shouldInsertInclude(Declaring, Inserted))
+ return "";
+std::string Path = Inserter.calculateIncludePath(Declaring, Inserted);
Action.EndSourceFile();
-if (!Path) {
- llvm::consumeError(Path.takeError());
- EXPECT_TRUE(ExpectError);
- return std::string();
-} else {
- EXPECT_FALSE(ExpectError);
-}
-return std::move(*Path);
+return Path;
}
- Expected>
- insert(const HeaderFile , const HeaderFile ,
- const std::vector = {}) {
+ Optional insert(StringRef VerbatimHeader) {
auto Clang = setupClang();
PreprocessOnlyAction Action;
EXPECT_TRUE(
Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
CDB.getCompileCommand(MainFile)->Directory,
Clang->getPreprocessor().getHeaderSearchInfo());
-for (const auto : ExistingInclusions)
- Inserter.addExisting(Inc);
-
-auto Edit = Inserter.insert(DeclaringHeader, InsertedHeader);
+auto Edit = Inserter.insert(VerbatimHeader);
Action.EndSourceFile();
return Edit;
}
@@ -220,31 +214,10 @@
EXPECT_EQ(calculate(BarHeader, "\"BAR.h\"", Inclusions), "");
}
-HeaderFile literal(StringRef Include) {
- HeaderFile H{Include, /*Verbatim=*/true};
- assert(H.valid());
- return H;
-}
-
TEST_F(HeadersTest, PreferInserted) {
- auto Edit = insert(literal(""), literal(""));
- EXPECT_TRUE(static_cast(Edit));
- EXPECT_TRUE(llvm::StringRef((*Edit)->newText).contains(""));
-}
-
-TEST_F(HeadersTest, ExistingInclusion) {
- Inclusion Existing{Range(), /*Written=*/"", /*Resolved=*/""};
- auto Edit = insert(literal(""), literal(""), {Existing});
- EXPECT_TRUE(static_cast(Edit));
- EXPECT_FALSE(static_cast(*Edit));
-}
-
-TEST_F(HeadersTest, ValidateHeaders) {
- HeaderFile InvalidHeader{"a.h", /*Verbatim=*/true};
- assert(!InvalidHeader.valid());
- auto Edit = insert(InvalidHeader, literal("\"c.h\""));
- EXPECT_FALSE(static_cast(Edit));
- llvm::consumeError(Edit.takeError());
+ auto Edit = insert("");
+ EXPECT_TRUE(Edit.hasValue());
+ EXPECT_TRUE(llvm::StringRef(Edit->newText).contains(""));
}
} // namespace
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -44,7 +44,19 @@
// GMock helpers for matching completion items.
MATCHER_P(Named, Name, "") { return arg.insertText == Name; }
-MATCHER_P(Labeled, Label, "") { return arg.label == Label; }
+MATCHER_P(Labeled, Label, "") {
+ std::string Indented;
+ if (!StringRef(Label).startswith(
+ CodeCompleteOptions().IncludeIndicator.Insert) &&
+ !StringRef(Label).startswith(
+ CodeCompleteOptions().IncludeIndicator.NoInsert))
+Indented =
+(Twine(CodeCompleteOptions().IncludeIndicator.NoInsert) + Label).str();
+ else
+Indented = Label;
+ return arg.label == Indented;
+}
+MATCHER_P(SigHelpLabeled, Label, "") { return
[PATCH] D48169: [mips] Add '-mcrc', '-mno-crc' options to enable/disable CRC ASE
atanasyan accepted this revision. atanasyan added inline comments. This revision is now accepted and ready to land. Comment at: include/clang/Driver/Options.td:2214 HelpText<"Disable SVR4-style position-independent code (Mips only)">; +def mno_crc : Flag<["-"], "mno-crc">, Group, + HelpText<"Disallow use of CRC instructions (Mips only)">; sdardis wrote: > atanasyan wrote: > > Does it make a sense to define this option as an alias to the `mnocrc` and > > allow to use both `-mno-crc` and `-mnocrc` for ARM and MIPS? > I thought about commenting on that. The problem there is that you then > permitting clang to be more lax than binutils which inhibits compatibility > between the two tools. > The problem there is that you then permitting clang to be more lax than > binutils which inhibits compatibility between the two tools. Good point. Agreed. Repository: rC Clang https://reviews.llvm.org/D48169 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48163: [clangd] UI for completion items that would trigger include insertion.
ioeric added inline comments. Comment at: clangd/CodeComplete.h:63 - // Populated internally by clangd, do not set. + /// Populated internally by clangd, do not set. /// If `Index` is set, it is used to augment the code completion sammccall wrote: > if you want this to be part of the doc comment, move it to the bottom? > > I think the intent was that this comment apply to all following members. So > keeping the line as `//` and adding the new member above it would also make > sense Ohh, didn't know this was intentional. Done. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48163 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48163: [clangd] UI for completion items that would trigger include insertion.
ioeric updated this revision to Diff 151497.
ioeric added a comment.
- fix typo and clang-format
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48163
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/Headers.cpp
clangd/Headers.h
test/clangd/completion-snippets.test
test/clangd/completion.test
test/clangd/protocol.test
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/HeadersTests.cpp
Index: unittests/clangd/HeadersTests.cpp
===
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -78,10 +78,10 @@
return Inclusions;
}
- // Calculates the include path, or returns "" on error.
+ // Calculates the include path, or returns "" on error or header should not be
+ // inserted.
std::string calculate(PathRef Original, PathRef Preferred = "",
-const std::vector = {},
-bool ExpectError = false) {
+const std::vector = {}) {
auto Clang = setupClang();
PreprocessOnlyAction Action;
EXPECT_TRUE(
@@ -94,36 +94,30 @@
/*Verbatim=*/!llvm::sys::path::is_absolute(Header)};
};
-auto Path = calculateIncludePath(
-MainFile, CDB.getCompileCommand(MainFile)->Directory,
-Clang->getPreprocessor().getHeaderSearchInfo(), Inclusions,
-ToHeaderFile(Original), ToHeaderFile(Preferred));
+IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
+ CDB.getCompileCommand(MainFile)->Directory,
+ Clang->getPreprocessor().getHeaderSearchInfo());
+for (const auto : Inclusions)
+ Inserter.addExisting(Inc);
+auto Declaring = ToHeaderFile(Original);
+auto Inserted = ToHeaderFile(Preferred);
+if (!Inserter.shouldInsertInclude(Declaring, Inserted))
+ return "";
+std::string Path = Inserter.calculateIncludePath(Declaring, Inserted);
Action.EndSourceFile();
-if (!Path) {
- llvm::consumeError(Path.takeError());
- EXPECT_TRUE(ExpectError);
- return std::string();
-} else {
- EXPECT_FALSE(ExpectError);
-}
-return std::move(*Path);
+return Path;
}
- Expected>
- insert(const HeaderFile , const HeaderFile ,
- const std::vector = {}) {
+ Optional insert(StringRef VerbatimHeader) {
auto Clang = setupClang();
PreprocessOnlyAction Action;
EXPECT_TRUE(
Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
CDB.getCompileCommand(MainFile)->Directory,
Clang->getPreprocessor().getHeaderSearchInfo());
-for (const auto : ExistingInclusions)
- Inserter.addExisting(Inc);
-
-auto Edit = Inserter.insert(DeclaringHeader, InsertedHeader);
+auto Edit = Inserter.insert(VerbatimHeader);
Action.EndSourceFile();
return Edit;
}
@@ -220,31 +214,10 @@
EXPECT_EQ(calculate(BarHeader, "\"BAR.h\"", Inclusions), "");
}
-HeaderFile literal(StringRef Include) {
- HeaderFile H{Include, /*Verbatim=*/true};
- assert(H.valid());
- return H;
-}
-
TEST_F(HeadersTest, PreferInserted) {
- auto Edit = insert(literal(""), literal(""));
- EXPECT_TRUE(static_cast(Edit));
- EXPECT_TRUE(llvm::StringRef((*Edit)->newText).contains(""));
-}
-
-TEST_F(HeadersTest, ExistingInclusion) {
- Inclusion Existing{Range(), /*Written=*/"", /*Resolved=*/""};
- auto Edit = insert(literal(""), literal(""), {Existing});
- EXPECT_TRUE(static_cast(Edit));
- EXPECT_FALSE(static_cast(*Edit));
-}
-
-TEST_F(HeadersTest, ValidateHeaders) {
- HeaderFile InvalidHeader{"a.h", /*Verbatim=*/true};
- assert(!InvalidHeader.valid());
- auto Edit = insert(InvalidHeader, literal("\"c.h\""));
- EXPECT_FALSE(static_cast(Edit));
- llvm::consumeError(Edit.takeError());
+ auto Edit = insert("");
+ EXPECT_TRUE(Edit.hasValue());
+ EXPECT_TRUE(llvm::StringRef(Edit->newText).contains(""));
}
} // namespace
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -44,7 +44,19 @@
// GMock helpers for matching completion items.
MATCHER_P(Named, Name, "") { return arg.insertText == Name; }
-MATCHER_P(Labeled, Label, "") { return arg.label == Label; }
+MATCHER_P(Labeled, Label, "") {
+ std::string Indented;
+ if (!StringRef(Label).startswith(
+ CodeCompleteOptions().IncludeIndicator.Insert) &&
+ !StringRef(Label).startswith(
+ CodeCompleteOptions().IncludeIndicator.NoInsert))
+Indented =
+(Twine(CodeCompleteOptions().IncludeIndicator.NoInsert) + Label).str();
+ else
+Indented = Label;
+ return arg.label == Indented;
+}
+MATCHER_P(SigHelpLabeled, Label, "") {
[PATCH] D48163: [clangd] UI for completion items that would trigger include insertion.
ioeric updated this revision to Diff 151496.
ioeric marked 6 inline comments as done.
ioeric added a comment.
- addressed review comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48163
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/Headers.cpp
clangd/Headers.h
test/clangd/completion-snippets.test
test/clangd/completion.test
test/clangd/protocol.test
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/HeadersTests.cpp
Index: unittests/clangd/HeadersTests.cpp
===
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -78,10 +78,10 @@
return Inclusions;
}
- // Calculates the include path, or returns "" on error.
+ // Calculates the include path, or returns "" on error or header should not be
+ // inserted.
std::string calculate(PathRef Original, PathRef Preferred = "",
-const std::vector = {},
-bool ExpectError = false) {
+const std::vector = {}) {
auto Clang = setupClang();
PreprocessOnlyAction Action;
EXPECT_TRUE(
@@ -94,36 +94,30 @@
/*Verbatim=*/!llvm::sys::path::is_absolute(Header)};
};
-auto Path = calculateIncludePath(
-MainFile, CDB.getCompileCommand(MainFile)->Directory,
-Clang->getPreprocessor().getHeaderSearchInfo(), Inclusions,
-ToHeaderFile(Original), ToHeaderFile(Preferred));
+IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
+ CDB.getCompileCommand(MainFile)->Directory,
+ Clang->getPreprocessor().getHeaderSearchInfo());
+for (const auto : Inclusions)
+ Inserter.addExisting(Inc);
+auto Declaring = ToHeaderFile(Original);
+auto Inserted = ToHeaderFile(Preferred);
+if (!Inserter.shouldInsertInclude(Declaring, Inserted))
+ return "";
+std::string Path = Inserter.calculateIncludePath(Declaring, Inserted);
Action.EndSourceFile();
-if (!Path) {
- llvm::consumeError(Path.takeError());
- EXPECT_TRUE(ExpectError);
- return std::string();
-} else {
- EXPECT_FALSE(ExpectError);
-}
-return std::move(*Path);
+return Path;
}
- Expected>
- insert(const HeaderFile , const HeaderFile ,
- const std::vector = {}) {
+ Optional insert(StringRef VerbatimHeader) {
auto Clang = setupClang();
PreprocessOnlyAction Action;
EXPECT_TRUE(
Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
CDB.getCompileCommand(MainFile)->Directory,
Clang->getPreprocessor().getHeaderSearchInfo());
-for (const auto : ExistingInclusions)
- Inserter.addExisting(Inc);
-
-auto Edit = Inserter.insert(DeclaringHeader, InsertedHeader);
+auto Edit = Inserter.insert(VerbatimHeader);
Action.EndSourceFile();
return Edit;
}
@@ -220,31 +214,10 @@
EXPECT_EQ(calculate(BarHeader, "\"BAR.h\"", Inclusions), "");
}
-HeaderFile literal(StringRef Include) {
- HeaderFile H{Include, /*Verbatim=*/true};
- assert(H.valid());
- return H;
-}
-
TEST_F(HeadersTest, PreferInserted) {
- auto Edit = insert(literal(""), literal(""));
- EXPECT_TRUE(static_cast(Edit));
- EXPECT_TRUE(llvm::StringRef((*Edit)->newText).contains(""));
-}
-
-TEST_F(HeadersTest, ExistingInclusion) {
- Inclusion Existing{Range(), /*Written=*/"", /*Resolved=*/""};
- auto Edit = insert(literal(""), literal(""), {Existing});
- EXPECT_TRUE(static_cast(Edit));
- EXPECT_FALSE(static_cast(*Edit));
-}
-
-TEST_F(HeadersTest, ValidateHeaders) {
- HeaderFile InvalidHeader{"a.h", /*Verbatim=*/true};
- assert(!InvalidHeader.valid());
- auto Edit = insert(InvalidHeader, literal("\"c.h\""));
- EXPECT_FALSE(static_cast(Edit));
- llvm::consumeError(Edit.takeError());
+ auto Edit = insert("");
+ EXPECT_TRUE(Edit.hasValue());
+ EXPECT_TRUE(llvm::StringRef(Edit->newText).contains(""));
}
} // namespace
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -44,7 +44,19 @@
// GMock helpers for matching completion items.
MATCHER_P(Named, Name, "") { return arg.insertText == Name; }
-MATCHER_P(Labeled, Label, "") { return arg.label == Label; }
+MATCHER_P(Labeled, Label, "") {
+ std::string Indented;
+ if (!StringRef(Label).startswith(
+ CodeCompleteOptions().IncludeIndicator.Insert) &&
+ !StringRef(Label).startswith(
+ CodeCompleteOptions().IncludeIndicator.NoInsert))
+Indented =
+(Twine(CodeCompleteOptions().IncludeIndicator.NoInsert) + Label).str();
+ else
+Indented = Label;
+ return arg.label == Indented;
+}
[PATCH] D48169: [mips] Add '-mcrc', '-mno-crc' options to enable/disable CRC ASE
sdardis added inline comments. Comment at: include/clang/Driver/Options.td:2214 HelpText<"Disable SVR4-style position-independent code (Mips only)">; +def mno_crc : Flag<["-"], "mno-crc">, Group, + HelpText<"Disallow use of CRC instructions (Mips only)">; atanasyan wrote: > Does it make a sense to define this option as an alias to the `mnocrc` and > allow to use both `-mno-crc` and `-mnocrc` for ARM and MIPS? I thought about commenting on that. The problem there is that you then permitting clang to be more lax than binutils which inhibits compatibility between the two tools. Repository: rC Clang https://reviews.llvm.org/D48169 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48163: [clangd] UI for completion items that would trigger include insertion.
sammccall added inline comments. Comment at: clangd/CodeComplete.cpp:313 +I.label = +(InsertingInclude ? Opts.IncludeInsertionIndicator : " ") + I.label; I.scoreInfo = Scores; sammccall wrote: > string(IncludeInsertionIndicator.size(), ' ')? oops, I made the same mistake - #bytes in the indicator != width of the indicator. maybe making this customizable is too much hassle Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48163 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48169: [mips] Add '-mcrc', '-mno-crc' options to enable/disable CRC ASE
atanasyan added inline comments. Comment at: include/clang/Driver/Options.td:2214 HelpText<"Disable SVR4-style position-independent code (Mips only)">; +def mno_crc : Flag<["-"], "mno-crc">, Group, + HelpText<"Disallow use of CRC instructions (Mips only)">; Does it make a sense to define this option as an alias to the `mnocrc` and allow to use both `-mno-crc` and `-mnocrc` for ARM and MIPS? Repository: rC Clang https://reviews.llvm.org/D48169 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48163: [clangd] UI for completion items that would trigger include insertion.
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clangd/CodeComplete.cpp:301
+ } else if (!IncludePath->empty()) {
+if (auto Edit = Includes.insert(*IncludePath)) {
+ I.detail += ((I.detail.empty() ? "" : "\n") +
This is fine for now, but can you add a FIXME to consider what to show if
there's no include to be inserted, and for sema results? There's some value in
showing the header consistently I think.
Comment at: clangd/CodeComplete.cpp:302
+if (auto Edit = Includes.insert(*IncludePath)) {
+ I.detail += ((I.detail.empty() ? "" : "\n") +
+ StringRef(*IncludePath).trim('"'))
can we add the \n for now even if detail is empty?
That way the editors that "inline" the detail into the same line as the label
won't show it (if we patch YCM to truncate like VSCode does).
The inconsistency (sometimes showing a return type and sometimes a header)
seems surprising.
Comment at: clangd/CodeComplete.cpp:313
+I.label =
+(InsertingInclude ? Opts.IncludeInsertionIndicator : " ") + I.label;
I.scoreInfo = Scores;
string(IncludeInsertionIndicator.size(), ' ')?
Comment at: clangd/CodeComplete.cpp:334
Opts.EnableSnippets ? (Name + "(${0})").str() : Name.str();
-First.label = (Name + "(…)").str();
+// Keep the first character of the original label which is an visual
+// indicator for include insertion or an indentation.
again, need to account for the length of the indicator
Comment at: clangd/CodeComplete.h:63
- // Populated internally by clangd, do not set.
+ /// Populated internally by clangd, do not set.
/// If `Index` is set, it is used to augment the code completion
if you want this to be part of the doc comment, move it to the bottom?
I think the intent was that this comment apply to all following members. So
keeping the line as `//` and adding the new member above it would also make
sense
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48163
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47474: Implement cpu_dispatch/cpu_specific Multiversioning
erichkeane added a comment. Just a quick bump, I know most of you are still recovering from the Switzerland trip, but hopefully the others will have some time. https://reviews.llvm.org/D47474 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r334824 - [clangd] context key constructor is constexpr. NFC
Author: sammccall Date: Fri Jun 15 05:39:21 2018 New Revision: 334824 URL: http://llvm.org/viewvc/llvm-project?rev=334824=rev Log: [clangd] context key constructor is constexpr. NFC Modified: clang-tools-extra/trunk/clangd/Context.h Modified: clang-tools-extra/trunk/clangd/Context.h URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Context.h?rev=334824=334823=334824=diff == --- clang-tools-extra/trunk/clangd/Context.h (original) +++ clang-tools-extra/trunk/clangd/Context.h Fri Jun 15 05:39:21 2018 @@ -43,7 +43,7 @@ public: static_assert(!std::is_reference::value, "Reference arguments to Key<> are not allowed"); - Key() = default; + constexpr Key() = default; Key(Key const &) = delete; Key =(Key const &) = delete; ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48163: [clangd] UI for completion items that would trigger include insertion.
ioeric added a comment. Thanks for the review! Comment at: clangd/CodeComplete.cpp:187 + // Whether or not this candidate is in a completion where index is allowed. + // This can affect how items are built (e.g. indent label to align with visual + // indicator in index results). sammccall wrote: > Hmm, I'm not sure this should be conditional. > Especially in editors that render [icon]label, inconsistently having a space > or not for no obvious reason seems disorienting to users. > > And it adds some complexity (especially in conjunction with bundling...) Sure. I wasn't sure about this and wanted get your opinion before fixing tests. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48163 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48163: [clangd] UI for completion items that would trigger include insertion.
ioeric updated this revision to Diff 151492.
ioeric marked 6 inline comments as done.
ioeric added a comment.
- Addressed review comments.
- Merged with orgin/master
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48163
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/Headers.cpp
clangd/Headers.h
test/clangd/completion-snippets.test
test/clangd/completion.test
test/clangd/protocol.test
unittests/clangd/CodeCompleteTests.cpp
unittests/clangd/HeadersTests.cpp
Index: unittests/clangd/HeadersTests.cpp
===
--- unittests/clangd/HeadersTests.cpp
+++ unittests/clangd/HeadersTests.cpp
@@ -78,10 +78,10 @@
return Inclusions;
}
- // Calculates the include path, or returns "" on error.
+ // Calculates the include path, or returns "" on error or header should not be
+ // inserted.
std::string calculate(PathRef Original, PathRef Preferred = "",
-const std::vector = {},
-bool ExpectError = false) {
+const std::vector = {}) {
auto Clang = setupClang();
PreprocessOnlyAction Action;
EXPECT_TRUE(
@@ -94,36 +94,30 @@
/*Verbatim=*/!llvm::sys::path::is_absolute(Header)};
};
-auto Path = calculateIncludePath(
-MainFile, CDB.getCompileCommand(MainFile)->Directory,
-Clang->getPreprocessor().getHeaderSearchInfo(), Inclusions,
-ToHeaderFile(Original), ToHeaderFile(Preferred));
+IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
+ CDB.getCompileCommand(MainFile)->Directory,
+ Clang->getPreprocessor().getHeaderSearchInfo());
+for (const auto : Inclusions)
+ Inserter.addExisting(Inc);
+auto Declaring = ToHeaderFile(Original);
+auto Inserted = ToHeaderFile(Preferred);
+if (!Inserter.shouldInsertInclude(Declaring, Inserted))
+ return "";
+std::string Path = Inserter.calculateIncludePath(Declaring, Inserted);
Action.EndSourceFile();
-if (!Path) {
- llvm::consumeError(Path.takeError());
- EXPECT_TRUE(ExpectError);
- return std::string();
-} else {
- EXPECT_FALSE(ExpectError);
-}
-return std::move(*Path);
+return Path;
}
- Expected>
- insert(const HeaderFile , const HeaderFile ,
- const std::vector = {}) {
+ Optional insert(StringRef VerbatimHeader) {
auto Clang = setupClang();
PreprocessOnlyAction Action;
EXPECT_TRUE(
Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0]));
IncludeInserter Inserter(MainFile, /*Code=*/"", format::getLLVMStyle(),
CDB.getCompileCommand(MainFile)->Directory,
Clang->getPreprocessor().getHeaderSearchInfo());
-for (const auto : ExistingInclusions)
- Inserter.addExisting(Inc);
-
-auto Edit = Inserter.insert(DeclaringHeader, InsertedHeader);
+auto Edit = Inserter.insert(VerbatimHeader);
Action.EndSourceFile();
return Edit;
}
@@ -220,31 +214,10 @@
EXPECT_EQ(calculate(BarHeader, "\"BAR.h\"", Inclusions), "");
}
-HeaderFile literal(StringRef Include) {
- HeaderFile H{Include, /*Verbatim=*/true};
- assert(H.valid());
- return H;
-}
-
TEST_F(HeadersTest, PreferInserted) {
- auto Edit = insert(literal(""), literal(""));
- EXPECT_TRUE(static_cast(Edit));
- EXPECT_TRUE(llvm::StringRef((*Edit)->newText).contains(""));
-}
-
-TEST_F(HeadersTest, ExistingInclusion) {
- Inclusion Existing{Range(), /*Written=*/"", /*Resolved=*/""};
- auto Edit = insert(literal(""), literal(""), {Existing});
- EXPECT_TRUE(static_cast(Edit));
- EXPECT_FALSE(static_cast(*Edit));
-}
-
-TEST_F(HeadersTest, ValidateHeaders) {
- HeaderFile InvalidHeader{"a.h", /*Verbatim=*/true};
- assert(!InvalidHeader.valid());
- auto Edit = insert(InvalidHeader, literal("\"c.h\""));
- EXPECT_FALSE(static_cast(Edit));
- llvm::consumeError(Edit.takeError());
+ auto Edit = insert("");
+ EXPECT_TRUE(Edit.hasValue());
+ EXPECT_TRUE(llvm::StringRef(Edit->newText).contains(""));
}
} // namespace
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -44,7 +44,17 @@
// GMock helpers for matching completion items.
MATCHER_P(Named, Name, "") { return arg.insertText == Name; }
-MATCHER_P(Labeled, Label, "") { return arg.label == Label; }
+MATCHER_P(Labeled, Label, "") {
+ std::string Indented;
+ if (!StringRef(Label).startswith(
+ CodeCompleteOptions().IncludeInsertionIndicator) &&
+ !StringRef(Label).startswith(" "))
+Indented = (Twine(" ") + Label).str();
+ else
+Indented = Label;
+ return arg.label == Indented;
+}
+MATCHER_P(SigHelpLabeled, Label, "") { return arg.label == Label; }
[PATCH] D47957: [clangd] Add option to fold overloads into a single completion item.
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.
Closed by commit rCTE334822: [clangd] Add option to fold overloads into a
single completion item. (authored by sammccall, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D47957?vs=151476=151486#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47957
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/tool/ClangdMain.cpp
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -32,6 +32,7 @@
using ::testing::Each;
using ::testing::ElementsAre;
using ::testing::Field;
+using ::testing::HasSubstr;
using ::testing::IsEmpty;
using ::testing::Not;
using ::testing::UnorderedElementsAre;
@@ -1060,6 +1061,57 @@
}
}
+TEST(CompletionTest, OverloadBundling) {
+ clangd::CodeCompleteOptions Opts;
+ Opts.BundleOverloads = true;
+
+ std::string Context = R"cpp(
+struct X {
+ // Overload with int
+ int a(int);
+ // Overload with bool
+ int a(bool);
+ int b(float);
+};
+int GFuncC(int);
+int GFuncD(int);
+ )cpp";
+
+ // Member completions are bundled.
+ EXPECT_THAT(completions(Context + "int y = X().^", {}, Opts).items,
+ UnorderedElementsAre(Labeled("a(…)"), Labeled("b(float)")));
+
+ // Non-member completions are bundled, including index+sema.
+ Symbol NoArgsGFunc = func("GFuncC");
+ EXPECT_THAT(
+ completions(Context + "int y = GFunc^", {NoArgsGFunc}, Opts).items,
+ UnorderedElementsAre(Labeled("GFuncC(…)"), Labeled("GFuncD(int)")));
+
+ // Differences in header-to-insert suppress bundling.
+ Symbol::Details Detail;
+ std::string DeclFile = URI::createFile(testPath("foo")).toString();
+ NoArgsGFunc.CanonicalDeclaration.FileURI = DeclFile;
+ Detail.IncludeHeader = "";
+ NoArgsGFunc.Detail =
+ EXPECT_THAT(
+ completions(Context + "int y = GFunc^", {NoArgsGFunc}, Opts).items,
+ UnorderedElementsAre(AllOf(Named("GFuncC"), InsertInclude("")),
+ Labeled("GFuncC(int)"), Labeled("GFuncD(int)")));
+
+ // Examine a bundled completion in detail.
+ auto A = completions(Context + "int y = X().a^", {}, Opts).items.front();
+ EXPECT_EQ(A.label, "a(…)");
+ EXPECT_EQ(A.detail, "[2 overloads]");
+ EXPECT_EQ(A.insertText, "a");
+ EXPECT_EQ(A.kind, CompletionItemKind::Method);
+ // For now we just return one of the doc strings arbitrarily.
+ EXPECT_THAT(A.documentation, AnyOf(HasSubstr("Overload with int"),
+ HasSubstr("Overload with bool")));
+ Opts.EnableSnippets = true;
+ A = completions(Context + "int y = X().a^", {}, Opts).items.front();
+ EXPECT_EQ(A.insertText, "a(${0})");
+}
+
TEST(CompletionTest, DocumentationFromChangedFileCrash) {
MockFSProvider FS;
auto FooH = testPath("foo.h");
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -7,10 +7,14 @@
//
//===-===//
//
-// AST-based completions are provided using the completion hooks in Sema.
+// Code completion has several moving parts:
+// - AST-based completions are provided using the completion hooks in Sema.
+// - external completions are retrieved from the index (using hints from Sema)
+// - the two sources overlap, and must be merged and overloads bundled
+// - results must be scored and ranked (see Quality.h) before rendering
//
-// Signature help works in a similar way as code completion, but it is simpler
-// as there are typically fewer candidates.
+// Signature help works in a similar way as code completion, but it is simpler:
+// it's purely AST-based, and there are few candidates.
//
//===-===//
@@ -184,36 +188,69 @@
const CodeCompletionResult *SemaResult = nullptr;
const Symbol *IndexResult = nullptr;
+ // Returns a token identifying the overload set this is part of.
+ // 0 indicates it's not part of any overload set.
+ size_t overloadSet() const {
+SmallString<256> Scratch;
+if (IndexResult) {
+ switch (IndexResult->SymInfo.Kind) {
+ case index::SymbolKind::ClassMethod:
+ case index::SymbolKind::InstanceMethod:
+ case index::SymbolKind::StaticMethod:
+assert(false && "Don't expect members from index in code completion");
+// fall through
+ case index::SymbolKind::Function:
+// We can't group overloads together that need different #includes.
+// This could break #include insertion.
+return hash_combine(
+(IndexResult->Scope + IndexResult->Name).toStringRef(Scratch),
+
[clang-tools-extra] r334822 - [clangd] Add option to fold overloads into a single completion item.
Author: sammccall
Date: Fri Jun 15 04:06:29 2018
New Revision: 334822
URL: http://llvm.org/viewvc/llvm-project?rev=334822=rev
Log:
[clangd] Add option to fold overloads into a single completion item.
Summary:
Adds a CodeCompleteOption to folds together compatible function/method overloads
into a single item. This feels pretty good (for editors with signatureHelp
support), but has limitations.
This happens in the code completion merge step, so there may be inconsistencies
(e.g. if only one overload made it into the index result list, no folding).
We don't want to bundle together completions that have different side-effects
(include insertion), because we can't constructo a coherent CompletionItem.
This may be confusing for users, as the reason for non-bundling may not
be immediately obvious. (Also, the implementation seems a little fragile)
Subscribers: ilya-biryukov, ioeric, MaskRay, jkorous, cfe-commits
Differential Revision: https://reviews.llvm.org/D47957
Modified:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/CodeComplete.h
clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=334822=334821=334822=diff
==
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Fri Jun 15 04:06:29 2018
@@ -7,10 +7,14 @@
//
//===-===//
//
-// AST-based completions are provided using the completion hooks in Sema.
+// Code completion has several moving parts:
+// - AST-based completions are provided using the completion hooks in Sema.
+// - external completions are retrieved from the index (using hints from Sema)
+// - the two sources overlap, and must be merged and overloads bundled
+// - results must be scored and ranked (see Quality.h) before rendering
//
-// Signature help works in a similar way as code completion, but it is simpler
-// as there are typically fewer candidates.
+// Signature help works in a similar way as code completion, but it is simpler:
+// it's purely AST-based, and there are few candidates.
//
//===-===//
@@ -184,6 +188,54 @@ struct CompletionCandidate {
const CodeCompletionResult *SemaResult = nullptr;
const Symbol *IndexResult = nullptr;
+ // Returns a token identifying the overload set this is part of.
+ // 0 indicates it's not part of any overload set.
+ size_t overloadSet() const {
+SmallString<256> Scratch;
+if (IndexResult) {
+ switch (IndexResult->SymInfo.Kind) {
+ case index::SymbolKind::ClassMethod:
+ case index::SymbolKind::InstanceMethod:
+ case index::SymbolKind::StaticMethod:
+assert(false && "Don't expect members from index in code completion");
+// fall through
+ case index::SymbolKind::Function:
+// We can't group overloads together that need different #includes.
+// This could break #include insertion.
+return hash_combine(
+(IndexResult->Scope + IndexResult->Name).toStringRef(Scratch),
+headerToInsertIfNotPresent().getValueOr(""));
+ default:
+return 0;
+ }
+}
+assert(SemaResult);
+// We need to make sure we're consistent with the IndexResult case!
+const NamedDecl *D = SemaResult->Declaration;
+if (!D || !D->isFunctionOrFunctionTemplate())
+ return 0;
+{
+ llvm::raw_svector_ostream OS(Scratch);
+ D->printQualifiedName(OS);
+}
+return hash_combine(Scratch, headerToInsertIfNotPresent().getValueOr(""));
+ }
+
+ llvm::Optional headerToInsertIfNotPresent() const {
+if (!IndexResult || !IndexResult->Detail ||
+IndexResult->Detail->IncludeHeader.empty())
+ return llvm::None;
+if (SemaResult && SemaResult->Declaration) {
+ // Avoid inserting new #include if the declaration is found in the
current
+ // file e.g. the symbol is forward declared.
+ auto = SemaResult->Declaration->getASTContext().getSourceManager();
+ for (const Decl *RD : SemaResult->Declaration->redecls())
+if (SM.isInMainFile(SM.getExpansionLoc(RD->getLocStart(
+ return llvm::None;
+}
+return IndexResult->Detail->IncludeHeader;
+ }
+
// Builds an LSP completion item.
CompletionItem build(StringRef FileName, const CompletionItemScores ,
const CodeCompleteOptions ,
@@ -192,7 +244,6 @@ struct CompletionCandidate {
llvm::StringRef SemaDocComment) const {
assert(bool(SemaResult) == bool(SemaCCS));
CompletionItem I;
-bool ShouldInsertInclude = true;
if (SemaResult) {
[PATCH] D47957: [clangd] Add option to fold overloads into a single completion item.
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM. (and a question on why we want to special-case the members)
Comment at: clangd/CodeComplete.cpp:245
+// Methods are simply grouped by name.
+return hash_combine('M', IndexResult->Name);
+ case index::SymbolKind::Function:
sammccall wrote:
> ilya-biryukov wrote:
> > Members never come from the index for completion, right? Maybe add an
> > assert here instead?
> Done.
> I'd keep the actual return here as it's simple and explains the value. We
> likely want to use it in future to augment members with signals from the
> index.
LG, thanks!
Comment at: clangd/CodeComplete.cpp:263
+if (D->isCXXClassMember())
+ return hash_combine('M', D->getDeclName().isIdentifier()
+ ? D->getName()
Using the normal-function scheme (fully-qualified name as grouping key) should
also work for members and will make the code simpler.
Why do we choose to special-case the members?
This might be addressed after landing the patch, just wanted to get the
rationale behind special-casing members.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47957
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r334820 - [AArch64] Reverted rC334696 with Clang VCVTA test fix
Author: lukegeeson
Date: Fri Jun 15 03:10:45 2018
New Revision: 334820
URL: http://llvm.org/viewvc/llvm-project?rev=334820=rev
Log:
[AArch64] Reverted rC334696 with Clang VCVTA test fix
Modified:
cfe/trunk/lib/CodeGen/CGBuiltin.cpp
cfe/trunk/test/CodeGen/aarch64-v8.2a-neon-intrinsics.c
cfe/trunk/test/CodeGen/arm-v8.2a-neon-intrinsics.c
Modified: cfe/trunk/lib/CodeGen/CGBuiltin.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGBuiltin.cpp?rev=334820=334819=334820=diff
==
--- cfe/trunk/lib/CodeGen/CGBuiltin.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGBuiltin.cpp Fri Jun 15 03:10:45 2018
@@ -4000,6 +4000,7 @@ static const NeonIntrinsicInfo ARMSIMDIn
NEONMAP1(vcvta_s16_v, arm_neon_vcvtas, 0),
NEONMAP1(vcvta_s32_v, arm_neon_vcvtas, 0),
NEONMAP1(vcvta_s64_v, arm_neon_vcvtas, 0),
+ NEONMAP1(vcvta_u16_v, arm_neon_vcvtau, 0),
NEONMAP1(vcvta_u32_v, arm_neon_vcvtau, 0),
NEONMAP1(vcvta_u64_v, arm_neon_vcvtau, 0),
NEONMAP1(vcvtaq_s16_v, arm_neon_vcvtas, 0),
@@ -4884,6 +4885,7 @@ Value *CodeGenFunction::EmitCommonNeonBu
case NEON::BI__builtin_neon_vcvta_s16_v:
case NEON::BI__builtin_neon_vcvta_s32_v:
case NEON::BI__builtin_neon_vcvta_s64_v:
+ case NEON::BI__builtin_neon_vcvta_u16_v:
case NEON::BI__builtin_neon_vcvta_u32_v:
case NEON::BI__builtin_neon_vcvta_u64_v:
case NEON::BI__builtin_neon_vcvtaq_s16_v:
@@ -7623,6 +7625,7 @@ Value *CodeGenFunction::EmitAArch64Built
return Builder.CreateFPToSI(Ops[0], Ty);
}
case NEON::BI__builtin_neon_vcvta_s16_v:
+ case NEON::BI__builtin_neon_vcvta_u16_v:
case NEON::BI__builtin_neon_vcvta_s32_v:
case NEON::BI__builtin_neon_vcvtaq_s16_v:
case NEON::BI__builtin_neon_vcvtaq_s32_v:
Modified: cfe/trunk/test/CodeGen/aarch64-v8.2a-neon-intrinsics.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/aarch64-v8.2a-neon-intrinsics.c?rev=334820=334819=334820=diff
==
--- cfe/trunk/test/CodeGen/aarch64-v8.2a-neon-intrinsics.c (original)
+++ cfe/trunk/test/CodeGen/aarch64-v8.2a-neon-intrinsics.c Fri Jun 15 03:10:45
2018
@@ -164,6 +164,13 @@ int16x4_t test_vcvta_s16_f16 (float16x4_
return vcvta_s16_f16(a);
}
+// CHECK-LABEL: test_vcvta_u16_f16
+// CHECK: [[VCVT:%.*]] = call <4 x i16>
@llvm.aarch64.neon.fcvtau.v4i16.v4f16(<4 x half> %a)
+// CHECK: ret <4 x i16> [[VCVT]]
+int16x4_t test_vcvta_u16_f16 (float16x4_t a) {
+ return vcvta_u16_f16(a);
+}
+
// CHECK-LABEL: test_vcvtaq_s16_f16
// CHECK: [[VCVT:%.*]] = call <8 x i16>
@llvm.aarch64.neon.fcvtas.v8i16.v8f16(<8 x half> %a)
// CHECK: ret <8 x i16> [[VCVT]]
Modified: cfe/trunk/test/CodeGen/arm-v8.2a-neon-intrinsics.c
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/arm-v8.2a-neon-intrinsics.c?rev=334820=334819=334820=diff
==
--- cfe/trunk/test/CodeGen/arm-v8.2a-neon-intrinsics.c (original)
+++ cfe/trunk/test/CodeGen/arm-v8.2a-neon-intrinsics.c Fri Jun 15 03:10:45 2018
@@ -164,6 +164,13 @@ int16x4_t test_vcvta_s16_f16 (float16x4_
return vcvta_s16_f16(a);
}
+// CHECK-LABEL: test_vcvta_u16_f16
+// CHECK: [[VCVT:%.*]] = call <4 x i16> @llvm.arm.neon.vcvtau.v4i16.v4f16(<4
x half> %a)
+// CHECK: ret <4 x i16> [[VCVT]]
+int16x4_t test_vcvta_u16_f16 (float16x4_t a) {
+ return vcvta_u16_f16(a);
+}
+
// CHECK-LABEL: test_vcvtaq_s16_f16
// CHECK: [[VCVT:%.*]] = call <8 x i16> @llvm.arm.neon.vcvtas.v8i16.v8f16(<8
x half> %a)
// CHECK: ret <8 x i16> [[VCVT]]
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48211: [clangd] Do not show namespace comments.
ilya-biryukov created this revision.
ilya-biryukov added a reviewer: sammccall.
Herald added subscribers: jkorous, MaskRay, ioeric.
Comments from namespaces that clangd produces are too noisy and often
not useful.
Namespaces have too many redecls and we don't have a good way of
determining which of the comments are relevant and which should be
ignored (e.g. because they come from code generators like the protobuf
compiler).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48211
Files:
clangd/CodeCompletionStrings.cpp
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1110,6 +1110,10 @@
ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
Annotations Source(R"cpp(
+// We ignore namespace comments, for rationale see CodeCompletionStrings.h.
+namespace comments_ns {
+}
+
// --
int comments_foo();
@@ -1151,6 +1155,7 @@
UnorderedElementsAre(AllOf(Not(IsDocumented()), Named("comments_foo")),
AllOf(IsDocumented(), Named("comments_baz")),
AllOf(IsDocumented(), Named("comments_quux")),
+ AllOf(Not(IsDocumented()), Named("comments_ns")),
// FIXME(ibiryukov): the following items should have
// empty documentation, since they are separated
from
// a comment with an empty line. Unfortunately, I
Index: clangd/CodeCompletionStrings.cpp
===
--- clangd/CodeCompletionStrings.cpp
+++ clangd/CodeCompletionStrings.cpp
@@ -174,6 +174,14 @@
auto *Decl = Result.getDeclaration();
if (!Decl || !canRequestComment(Ctx, *Decl, CommentsFromHeaders))
return "";
+ if (llvm::isa(Decl)) {
+// Namespaces often have too many redecls for any particular redecl comment
+// to be useful. Moreover, we often confuse file headers or generated
+// comments with namespace comments. Therefore we choose to just ignore
+// the comments for namespaces.
+return "";
+ }
+
const RawComment *RC = getCompletionComment(Ctx, Decl);
if (!RC)
return "";
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -1110,6 +1110,10 @@
ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
Annotations Source(R"cpp(
+// We ignore namespace comments, for rationale see CodeCompletionStrings.h.
+namespace comments_ns {
+}
+
// --
int comments_foo();
@@ -1151,6 +1155,7 @@
UnorderedElementsAre(AllOf(Not(IsDocumented()), Named("comments_foo")),
AllOf(IsDocumented(), Named("comments_baz")),
AllOf(IsDocumented(), Named("comments_quux")),
+ AllOf(Not(IsDocumented()), Named("comments_ns")),
// FIXME(ibiryukov): the following items should have
// empty documentation, since they are separated from
// a comment with an empty line. Unfortunately, I
Index: clangd/CodeCompletionStrings.cpp
===
--- clangd/CodeCompletionStrings.cpp
+++ clangd/CodeCompletionStrings.cpp
@@ -174,6 +174,14 @@
auto *Decl = Result.getDeclaration();
if (!Decl || !canRequestComment(Ctx, *Decl, CommentsFromHeaders))
return "";
+ if (llvm::isa(Decl)) {
+// Namespaces often have too many redecls for any particular redecl comment
+// to be useful. Moreover, we often confuse file headers or generated
+// comments with namespace comments. Therefore we choose to just ignore
+// the comments for namespaces.
+return "";
+ }
+
const RawComment *RC = getCompletionComment(Ctx, Decl);
if (!RC)
return "";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47957: [clangd] Add option to fold overloads into a single completion item.
sammccall updated this revision to Diff 151476.
sammccall marked an inline comment as done.
sammccall added a comment.
Review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47957
Files:
clangd/CodeComplete.cpp
clangd/CodeComplete.h
clangd/tool/ClangdMain.cpp
unittests/clangd/CodeCompleteTests.cpp
Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -32,6 +32,7 @@
using ::testing::Each;
using ::testing::ElementsAre;
using ::testing::Field;
+using ::testing::HasSubstr;
using ::testing::IsEmpty;
using ::testing::Not;
using ::testing::UnorderedElementsAre;
@@ -1065,6 +1066,57 @@
}
}
+TEST(CompletionTest, OverloadBundling) {
+ clangd::CodeCompleteOptions Opts;
+ Opts.BundleOverloads = true;
+
+ std::string Context = R"cpp(
+struct X {
+ // Overload with int
+ int a(int);
+ // Overload with bool
+ int a(bool);
+ int b(float);
+};
+int GFuncC(int);
+int GFuncD(int);
+ )cpp";
+
+ // Member completions are bundled.
+ EXPECT_THAT(completions(Context + "int y = X().^", {}, Opts).items,
+ UnorderedElementsAre(Labeled("a(…)"), Labeled("b(float)")));
+
+ // Non-member completions are bundled, including index+sema.
+ Symbol NoArgsGFunc = func("GFuncC");
+ EXPECT_THAT(
+ completions(Context + "int y = GFunc^", {NoArgsGFunc}, Opts).items,
+ UnorderedElementsAre(Labeled("GFuncC(…)"), Labeled("GFuncD(int)")));
+
+ // Differences in header-to-insert suppress bundling.
+ Symbol::Details Detail;
+ std::string DeclFile = URI::createFile(testPath("foo")).toString();
+ NoArgsGFunc.CanonicalDeclaration.FileURI = DeclFile;
+ Detail.IncludeHeader = "";
+ NoArgsGFunc.Detail =
+ EXPECT_THAT(
+ completions(Context + "int y = GFunc^", {NoArgsGFunc}, Opts).items,
+ UnorderedElementsAre(AllOf(Named("GFuncC"), InsertInclude("")),
+ Labeled("GFuncC(int)"), Labeled("GFuncD(int)")));
+
+ // Examine a bundled completion in detail.
+ auto A = completions(Context + "int y = X().a^", {}, Opts).items.front();
+ EXPECT_EQ(A.label, "a(…)");
+ EXPECT_EQ(A.detail, "[2 overloads]");
+ EXPECT_EQ(A.insertText, "a");
+ EXPECT_EQ(A.kind, CompletionItemKind::Method);
+ // For now we just return one of the doc strings arbitrarily.
+ EXPECT_THAT(A.documentation, AnyOf(HasSubstr("Overload with int"),
+ HasSubstr("Overload with bool")));
+ Opts.EnableSnippets = true;
+ A = completions(Context + "int y = X().a^", {}, Opts).items.front();
+ EXPECT_EQ(A.insertText, "a(${0})");
+}
+
TEST(CompletionTest, DocumentationFromChangedFileCrash) {
MockFSProvider FS;
auto FooH = testPath("foo.h");
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -59,6 +59,23 @@
llvm::cl::desc("Number of async workers used by clangd"),
llvm::cl::init(getDefaultAsyncThreadsCount()));
+// FIXME: also support "plain" style where signatures are always omitted.
+enum CompletionStyleFlag {
+ Detailed,
+ Bundled,
+};
+static llvm::cl::opt CompletionStyle(
+"completion-style",
+llvm::cl::desc("Granularity of code completion suggestions"),
+llvm::cl::values(
+clEnumValN(Detailed, "detailed",
+ "One completion item for each semantically distinct "
+ "completion, with full type information."),
+clEnumValN(Bundled, "bundled",
+ "Similar completion items (e.g. function overloads) are "
+ "combined. Type information shown where possible.")),
+llvm::cl::init(Detailed));
+
// FIXME: Flags are the wrong mechanism for user preferences.
// We should probably read a dotfile or similar.
static llvm::cl::opt IncludeIneligibleResults(
@@ -233,6 +250,7 @@
clangd::CodeCompleteOptions CCOpts;
CCOpts.IncludeIneligibleResults = IncludeIneligibleResults;
CCOpts.Limit = LimitResults;
+ CCOpts.BundleOverloads = CompletionStyle != Detailed;
// Initialize and run ClangdLSPServer.
ClangdLSPServer LSPServer(Out, CCOpts, CompileCommandsDirPath, Opts);
Index: clangd/CodeComplete.h
===
--- clangd/CodeComplete.h
+++ clangd/CodeComplete.h
@@ -53,6 +53,9 @@
/// For example, private members are usually inaccessible.
bool IncludeIneligibleResults = false;
+ /// Combine overloads into a single completion item where possible.
+ bool BundleOverloads = false;
+
/// Limit the number of results returned (0 means no limit).
/// If more results are available, we set CompletionList.isIncomplete.
size_t Limit = 0;
Index: clangd/CodeComplete.cpp
[PATCH] D47957: [clangd] Add option to fold overloads into a single completion item.
sammccall marked 4 inline comments as done.
sammccall added inline comments.
Comment at: clangd/CodeComplete.cpp:245
+// Methods are simply grouped by name.
+return hash_combine('M', IndexResult->Name);
+ case index::SymbolKind::Function:
ilya-biryukov wrote:
> Members never come from the index for completion, right? Maybe add an assert
> here instead?
Done.
I'd keep the actual return here as it's simple and explains the value. We
likely want to use it in future to augment members with signals from the index.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47957
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48188: [SPIR] Prevent SPIR targets from using half conversion intrinsics
SjoerdMeijer added a comment.
I know very little about SPIR, and Initially didn't understand this:
> The SPIR target currently allows for half precision floating point types to
> use the LLVM intrinsic functions to convert to floats and doubles. This is
> illegal in SPIR as the only intrinsic allowed by SPIR is memcpy ...
until I looked at the implementation what you're trying to achieve here.
Perhaps you can make the commit message a bit more descriptive and specific.
Comment at: lib/Basic/Targets/SPIR.h:50
UseAddrSpaceMapMangling = true;
+HasLegalHalfType = true;
// Define available target features
It doesn't hurt to set this, but you're not using it so you could omit it. I
had to introduce this to deal differently with half types depending on
architecture extensions, but don't you think have this problem.
Comment at: lib/Basic/Targets/SPIR.h:65
+ // memcpy as per section 3 of the SPIR spec.
+ bool useFP16ConversionIntrinsics() const override { return false; }
+
just a note: this is the only functional change, but you're testing a lot more
in test/CodeGen/spir-half-type.cpp
Comment at: test/CodeGen/spir-half-type.cpp:3
+// RUN: %clang_cc1 -O0 -triple spir64 -emit-llvm %s -o - | FileCheck %s
+
+// This file tests that using the _Float16 type with the spir target will not
use the llvm intrinsics but instead will use the half arithmetic instructions
directly.
I think you need one reproducer to test:
// CHECK-NOT: llvm.convert.from.fp16
The other tests, like all the compares are valid tests, but not related to this
change, and also not specific to SPIR. I put my _Float16 "smoke tests" in
test/CodeGenCXX/float16-declarations.cpp, perhaps you can move some of these
generic tests there because I for example see I didn't add any compares there.
Comment at: test/CodeGen/spir-half-type.cpp:4
+
+// This file tests that using the _Float16 type with the spir target will not
use the llvm intrinsics but instead will use the half arithmetic instructions
directly.
+
nit: this comment exceeds 80 columns, same for the other comment below.
Repository:
rC Clang
https://reviews.llvm.org/D48188
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r334812 - [clangd] Fix buildbot error.
Author: hokein
Date: Fri Jun 15 02:32:36 2018
New Revision: 334812
URL: http://llvm.org/viewvc/llvm-project?rev=334812=rev
Log:
[clangd] Fix buildbot error.
Modified:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=334812=334811=334812=diff
==
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Fri Jun 15 02:32:36 2018
@@ -858,7 +858,7 @@ public:
// calculate the file proximity, which would capture include/ and src/
// project setup where headers and implementations are not in the same
// directory.
-FileProximityMatch({FileName}) {}
+FileProximityMatch(ArrayRef({FileName})) {}
CompletionList run(const SemaCompleteInput ) && {
trace::Span Tracer("CodeCompleteFlow");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47935: [clangd] Boost completion score according to file proximity.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL334810: [clangd] Boost completion score according to file
proximity. (authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D47935
Files:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/Quality.cpp
clang-tools-extra/trunk/clangd/Quality.h
clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp
@@ -18,12 +18,18 @@
//===--===//
#include "Quality.h"
+#include "TestFS.h"
#include "TestTU.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
namespace clang {
namespace clangd {
+
+// Force the unittest URI scheme to be linked,
+static int LLVM_ATTRIBUTE_UNUSED UnittestSchemeAnchorDest =
+UnittestSchemeAnchorSource;
+
namespace {
TEST(QualityTests, SymbolQualitySignalExtraction) {
@@ -91,13 +97,14 @@
Relevance = {};
Relevance.merge(CodeCompletionResult((AST, "main"), 42));
- EXPECT_FLOAT_EQ(Relevance.ProximityScore, 1.0) << "Decl in current file";
+ EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0) << "Decl in current file";
Relevance = {};
Relevance.merge(CodeCompletionResult((AST, "header"), 42));
- EXPECT_FLOAT_EQ(Relevance.ProximityScore, 0.0) << "Decl from header";
+ EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 0.6) << "Decl from header";
Relevance = {};
Relevance.merge(CodeCompletionResult((AST, "header_main"), 42));
- EXPECT_FLOAT_EQ(Relevance.ProximityScore, 1.0) << "Current file and header";
+ EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0)
+ << "Current file and header";
Relevance = {};
Relevance.merge(CodeCompletionResult((AST, "X"), 42));
@@ -151,7 +158,7 @@
EXPECT_LT(PoorNameMatch.evaluate(), Default.evaluate());
SymbolRelevanceSignals WithProximity;
- WithProximity.ProximityScore = 0.2f;
+ WithProximity.SemaProximityScore = 0.2f;
EXPECT_GT(WithProximity.evaluate(), Default.evaluate());
SymbolRelevanceSignals Scoped;
@@ -173,6 +180,59 @@
EXPECT_LT(sortText(0, "a"), sortText(0, "z"));
}
+// {a,b,c} becomes /clangd-test/a/b/c
+std::string joinPaths(llvm::ArrayRef Parts) {
+ return testPath(
+ llvm::join(Parts.begin(), Parts.end(), llvm::sys::path::get_separator()));
+}
+
+static constexpr float ProximityBase = 0.7;
+
+// Calculates a proximity score for an index symbol with declaration file
+// SymPath with the given URI scheme.
+float URIProximity(const FileProximityMatcher , StringRef SymPath,
+ StringRef Scheme = "file") {
+ auto U = URI::create(SymPath, Scheme);
+ EXPECT_TRUE(static_cast(U)) << llvm::toString(U.takeError());
+ return Matcher.uriProximity(U->toString());
+}
+
+TEST(QualityTests, URIProximityScores) {
+ FileProximityMatcher Matcher(
+ /*ProximityPaths=*/{joinPaths({"a", "b", "c", "d", "x"})});
+
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"a", "b", "c", "d", "x"})),
+ 1);
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"a", "b", "c", "d", "y"})),
+ ProximityBase);
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"a", "y", "z"})),
+ std::pow(ProximityBase, 5));
+ EXPECT_FLOAT_EQ(
+ URIProximity(Matcher, joinPaths({"a", "b", "c", "d", "e", "y"})),
+ std::pow(ProximityBase, 2));
+ EXPECT_FLOAT_EQ(
+ URIProximity(Matcher, joinPaths({"a", "b", "m", "n", "o", "y"})),
+ std::pow(ProximityBase, 5));
+ EXPECT_FLOAT_EQ(
+ URIProximity(Matcher, joinPaths({"a", "t", "m", "n", "o", "y"})),
+ std::pow(ProximityBase, 6));
+ // Note the common directory is /clang-test/
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"m", "n", "o", "p", "y"})),
+ std::pow(ProximityBase, 7));
+}
+
+TEST(QualityTests, URIProximityScoresWithTestURI) {
+ FileProximityMatcher Matcher(
+ /*ProximityPaths=*/{joinPaths({"b", "c", "x"})});
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"b", "c", "x"}), "unittest"),
+ 1);
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"b", "y"}), "unittest"),
+ std::pow(ProximityBase, 2));
+ // unittest:///b/c/x vs unittest:///m/n/y. No common directory.
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"m", "n", "y"}), "unittest"),
+ std::pow(ProximityBase, 4));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/trunk/clangd/Quality.h
===
--- clang-tools-extra/trunk/clangd/Quality.h
+++ clang-tools-extra/trunk/clangd/Quality.h
@@ -26,6 +26,7 @@
[clang-tools-extra] r334810 - [clangd] Boost completion score according to file proximity.
Author: ioeric
Date: Fri Jun 15 01:58:12 2018
New Revision: 334810
URL: http://llvm.org/viewvc/llvm-project?rev=334810=rev
Log:
[clangd] Boost completion score according to file proximity.
Summary:
Also move unittest: URI scheme to TestFS so that it can be shared by
different tests.
Reviewers: sammccall
Reviewed By: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, cfe-commits
Differential Revision: https://reviews.llvm.org/D47935
Modified:
clang-tools-extra/trunk/clangd/CodeComplete.cpp
clang-tools-extra/trunk/clangd/Quality.cpp
clang-tools-extra/trunk/clangd/Quality.h
clang-tools-extra/trunk/unittests/clangd/QualityTests.cpp
Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=334810=334809=334810=diff
==
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Fri Jun 15 01:58:12 2018
@@ -848,11 +848,17 @@ class CodeCompleteFlow {
bool Incomplete = false; // Would more be available with a higher limit?
llvm::Optional Filter; // Initialized once Sema runs.
std::unique_ptr Includes; // Initialized once compiler runs.
+ FileProximityMatcher FileProximityMatch;
public:
// A CodeCompleteFlow object is only useful for calling run() exactly once.
CodeCompleteFlow(PathRef FileName, const CodeCompleteOptions )
- : FileName(FileName), Opts(Opts) {}
+ : FileName(FileName), Opts(Opts),
+// FIXME: also use path of the main header corresponding to FileName to
+// calculate the file proximity, which would capture include/ and src/
+// project setup where headers and implementations are not in the same
+// directory.
+FileProximityMatch({FileName}) {}
CompletionList run(const SemaCompleteInput ) && {
trace::Span Tracer("CodeCompleteFlow");
@@ -993,6 +999,7 @@ private:
SymbolQualitySignals Quality;
SymbolRelevanceSignals Relevance;
Relevance.Query = SymbolRelevanceSignals::CodeComplete;
+Relevance.FileProximityMatch =
if (auto FuzzyScore = fuzzyScore(C))
Relevance.NameMatch = *FuzzyScore;
else
Modified: clang-tools-extra/trunk/clangd/Quality.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Quality.cpp?rev=334810=334809=334810=diff
==
--- clang-tools-extra/trunk/clangd/Quality.cpp (original)
+++ clang-tools-extra/trunk/clangd/Quality.cpp Fri Jun 15 01:58:12 2018
@@ -7,6 +7,7 @@
//
//===-===//
#include "Quality.h"
+#include "URI.h"
#include "index/Index.h"
#include "clang/AST/ASTContext.h"
#include "clang/Basic/CharInfo.h"
@@ -185,6 +186,60 @@ raw_ostream <<(raw_ostream ,
return OS;
}
+/// Calculates a proximity score from \p From and \p To, which are URI strings
+/// that have the same scheme. This does not parse URI. A URI (sans
":")
+/// is split into chunks by '/' and each chunk is considered a file/directory.
+/// For example, "uri:///a/b/c" will be treated as /a/b/c
+static float uriProximity(StringRef From, StringRef To) {
+ auto SchemeSplitFrom = From.split(':');
+ auto SchemeSplitTo = To.split(':');
+ assert((SchemeSplitFrom.first == SchemeSplitTo.first) &&
+ "URIs must have the same scheme in order to compute proximity.");
+ auto Split = [](StringRef URIWithoutScheme) {
+SmallVector Split;
+URIWithoutScheme.split(Split, '/', /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+return Split;
+ };
+ SmallVector Fs = Split(SchemeSplitFrom.second);
+ SmallVector Ts = Split(SchemeSplitTo.second);
+ auto F = Fs.begin(), T = Ts.begin(), FE = Fs.end(), TE = Ts.end();
+ for (; F != FE && T != TE && *F == *T; ++F, ++T) {
+ }
+ // We penalize for traversing up and down from \p From to \p To but penalize
+ // less for traversing down because subprojects are more closely related than
+ // superprojects.
+ int UpDist = FE - F;
+ int DownDist = TE - T;
+ return std::pow(0.7, UpDist + DownDist/2);
+}
+
+FileProximityMatcher::FileProximityMatcher(ArrayRef ProximityPaths)
+: ProximityPaths(ProximityPaths.begin(), ProximityPaths.end()) {}
+
+float FileProximityMatcher::uriProximity(StringRef SymbolURI) const {
+ float Score = 0;
+ if (!ProximityPaths.empty() && !SymbolURI.empty()) {
+for (const auto : ProximityPaths)
+ // Only calculate proximity score for two URIs with the same scheme so
+ // that the computation can be purely text-based and thus avoid expensive
+ // URI encoding/decoding.
+ if (auto U = URI::create(Path, SymbolURI.split(':').first)) {
+Score = std::max(Score, clangd::uriProximity(U->toString(),
SymbolURI));
+ } else {
+llvm::consumeError(U.takeError());
+ }
+ }
+ return
[PATCH] D47935: [clangd] Boost completion score according to file proximity.
ioeric added a comment. Thanks for the review! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D47935 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47935: [clangd] Boost completion score according to file proximity.
ioeric updated this revision to Diff 151469.
ioeric marked 6 inline comments as done.
ioeric added a comment.
- addressed review comments and rebase.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47935
Files:
clangd/CodeComplete.cpp
clangd/Quality.cpp
clangd/Quality.h
unittests/clangd/QualityTests.cpp
Index: unittests/clangd/QualityTests.cpp
===
--- unittests/clangd/QualityTests.cpp
+++ unittests/clangd/QualityTests.cpp
@@ -18,12 +18,18 @@
//===--===//
#include "Quality.h"
+#include "TestFS.h"
#include "TestTU.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
namespace clang {
namespace clangd {
+
+// Force the unittest URI scheme to be linked,
+static int LLVM_ATTRIBUTE_UNUSED UnittestSchemeAnchorDest =
+UnittestSchemeAnchorSource;
+
namespace {
TEST(QualityTests, SymbolQualitySignalExtraction) {
@@ -91,13 +97,14 @@
Relevance = {};
Relevance.merge(CodeCompletionResult((AST, "main"), 42));
- EXPECT_FLOAT_EQ(Relevance.ProximityScore, 1.0) << "Decl in current file";
+ EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0) << "Decl in current file";
Relevance = {};
Relevance.merge(CodeCompletionResult((AST, "header"), 42));
- EXPECT_FLOAT_EQ(Relevance.ProximityScore, 0.0) << "Decl from header";
+ EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 0.6) << "Decl from header";
Relevance = {};
Relevance.merge(CodeCompletionResult((AST, "header_main"), 42));
- EXPECT_FLOAT_EQ(Relevance.ProximityScore, 1.0) << "Current file and header";
+ EXPECT_FLOAT_EQ(Relevance.SemaProximityScore, 1.0)
+ << "Current file and header";
Relevance = {};
Relevance.merge(CodeCompletionResult((AST, "X"), 42));
@@ -151,7 +158,7 @@
EXPECT_LT(PoorNameMatch.evaluate(), Default.evaluate());
SymbolRelevanceSignals WithProximity;
- WithProximity.ProximityScore = 0.2f;
+ WithProximity.SemaProximityScore = 0.2f;
EXPECT_GT(WithProximity.evaluate(), Default.evaluate());
SymbolRelevanceSignals Scoped;
@@ -173,6 +180,59 @@
EXPECT_LT(sortText(0, "a"), sortText(0, "z"));
}
+// {a,b,c} becomes /clangd-test/a/b/c
+std::string joinPaths(llvm::ArrayRef Parts) {
+ return testPath(
+ llvm::join(Parts.begin(), Parts.end(), llvm::sys::path::get_separator()));
+}
+
+static constexpr float ProximityBase = 0.7;
+
+// Calculates a proximity score for an index symbol with declaration file
+// SymPath with the given URI scheme.
+float URIProximity(const FileProximityMatcher , StringRef SymPath,
+ StringRef Scheme = "file") {
+ auto U = URI::create(SymPath, Scheme);
+ EXPECT_TRUE(static_cast(U)) << llvm::toString(U.takeError());
+ return Matcher.uriProximity(U->toString());
+}
+
+TEST(QualityTests, URIProximityScores) {
+ FileProximityMatcher Matcher(
+ /*ProximityPaths=*/{joinPaths({"a", "b", "c", "d", "x"})});
+
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"a", "b", "c", "d", "x"})),
+ 1);
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"a", "b", "c", "d", "y"})),
+ ProximityBase);
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"a", "y", "z"})),
+ std::pow(ProximityBase, 5));
+ EXPECT_FLOAT_EQ(
+ URIProximity(Matcher, joinPaths({"a", "b", "c", "d", "e", "y"})),
+ std::pow(ProximityBase, 2));
+ EXPECT_FLOAT_EQ(
+ URIProximity(Matcher, joinPaths({"a", "b", "m", "n", "o", "y"})),
+ std::pow(ProximityBase, 5));
+ EXPECT_FLOAT_EQ(
+ URIProximity(Matcher, joinPaths({"a", "t", "m", "n", "o", "y"})),
+ std::pow(ProximityBase, 6));
+ // Note the common directory is /clang-test/
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"m", "n", "o", "p", "y"})),
+ std::pow(ProximityBase, 7));
+}
+
+TEST(QualityTests, URIProximityScoresWithTestURI) {
+ FileProximityMatcher Matcher(
+ /*ProximityPaths=*/{joinPaths({"b", "c", "x"})});
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"b", "c", "x"}), "unittest"),
+ 1);
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"b", "y"}), "unittest"),
+ std::pow(ProximityBase, 2));
+ // unittest:///b/c/x vs unittest:///m/n/y. No common directory.
+ EXPECT_FLOAT_EQ(URIProximity(Matcher, joinPaths({"m", "n", "y"}), "unittest"),
+ std::pow(ProximityBase, 4));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clangd/Quality.h
===
--- clangd/Quality.h
+++ clangd/Quality.h
@@ -26,6 +26,7 @@
//===-===//
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_QUALITY_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_QUALITY_H
+#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/StringRef.h"
#include
#include
@@ -68,13 +69,22 @@
llvm::raw_ostream <<(llvm::raw_ostream &,
[PATCH] D47931: [clangd] Customizable URI schemes for dynamic index.
ioeric added inline comments.
Comment at: unittests/clangd/FileIndexTests.cpp:99
+class TestURIScheme : public URIScheme {
+public:
sammccall wrote:
> This can use the shared unittest: scheme from the followup patch, right?
>
> Add a fixme or lift that into this patch?
Done. Lifted the TestFS change into this patch.
Repository:
rL LLVM
https://reviews.llvm.org/D47931
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47931: [clangd] Customizable URI schemes for dynamic index.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL334809: [clangd] Customizable URI schemes for dynamic index.
(authored by ioeric, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D47931
Files:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/index/FileIndex.cpp
clang-tools-extra/trunk/clangd/index/FileIndex.h
clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
clang-tools-extra/trunk/unittests/clangd/TestFS.cpp
clang-tools-extra/trunk/unittests/clangd/TestFS.h
clang-tools-extra/trunk/unittests/clangd/URITests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
@@ -96,6 +96,20 @@
M.update(File.Filename, (), AST.getPreprocessorPtr());
}
+TEST(FileIndexTest, CustomizedURIScheme) {
+ FileIndex M({"unittest"});
+ update(M, "f", "class string {};");
+
+ FuzzyFindRequest Req;
+ Req.Query = "";
+ bool SeenSymbol = false;
+ M.fuzzyFind(Req, [&](const Symbol ) {
+EXPECT_EQ(Sym.CanonicalDeclaration.FileURI, "unittest:///f.h");
+SeenSymbol = true;
+ });
+ EXPECT_TRUE(SeenSymbol);
+}
+
TEST(FileIndexTest, IndexAST) {
FileIndex M;
update(M, "f1", "namespace ns { void f() {} class X {}; }");
Index: clang-tools-extra/trunk/unittests/clangd/TestFS.h
===
--- clang-tools-extra/trunk/unittests/clangd/TestFS.h
+++ clang-tools-extra/trunk/unittests/clangd/TestFS.h
@@ -56,6 +56,11 @@
// Returns a suitable absolute path for this OS.
std::string testPath(PathRef File);
+// unittest: is a scheme that refers to files relative to testRoot()
+// This anchor is used to force the linker to link in the generated object file
+// and thus register unittest: URI scheme plugin.
+extern volatile int UnittestSchemeAnchorSource;
+
} // namespace clangd
} // namespace clang
#endif
Index: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
@@ -152,28 +152,6 @@
}
};
-constexpr const char* ClangdTestScheme = "ClangdTests";
-class TestURIScheme : public URIScheme {
-public:
- llvm::Expected
- getAbsolutePath(llvm::StringRef /*Authority*/, llvm::StringRef Body,
- llvm::StringRef /*HintPath*/) const override {
-llvm_unreachable("ClangdTests never makes absolute path.");
- }
-
- llvm::Expected
- uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
-llvm_unreachable("ClangdTest never creates a test URI.");
- }
-
- llvm::Expected getIncludeSpelling(const URI ) const override {
-return ("\"" + U.body().trim("/") + "\"").str();
- }
-};
-
-static URISchemeRegistry::Add
-X(ClangdTestScheme, "Test scheme for ClangdTests.");
-
TEST_F(ClangdVFSTest, Parse) {
// FIXME: figure out a stable format for AST dumps, so that we can check the
// output of the dump itself is equal to the expected one, not just that it's
Index: clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
@@ -372,17 +372,15 @@
UnorderedElementsAre(AllOf(QName("Foo"), DeclURI(TestHeaderURI;
}
-#ifndef _WIN32
TEST_F(SymbolCollectorTest, CustomURIScheme) {
// Use test URI scheme from URITests.cpp
CollectorOpts.URISchemes.insert(CollectorOpts.URISchemes.begin(), "unittest");
- TestHeaderName = testPath("test-root/x.h");
- TestFileName = testPath("test-root/x.cpp");
+ TestHeaderName = testPath("x.h");
+ TestFileName = testPath("x.cpp");
runSymbolCollector("class Foo {};", /*Main=*/"");
- EXPECT_THAT(Symbols,
- UnorderedElementsAre(AllOf(QName("Foo"), DeclURI("unittest:x.h";
+ EXPECT_THAT(Symbols, UnorderedElementsAre(
+ AllOf(QName("Foo"), DeclURI("unittest:///x.h";
}
-#endif
TEST_F(SymbolCollectorTest, InvalidURIScheme) {
// Use test URI scheme from URITests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/URITests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/URITests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/URITests.cpp
@@ -14,46 +14,19 @@
namespace clang {
namespace clangd {
+
+// Force the unittest URI scheme to
[clang-tools-extra] r334809 - [clangd] Customizable URI schemes for dynamic index.
Author: ioeric
Date: Fri Jun 15 01:55:00 2018
New Revision: 334809
URL: http://llvm.org/viewvc/llvm-project?rev=334809=rev
Log:
[clangd] Customizable URI schemes for dynamic index.
Summary:
This allows dynamic index to have consistent URI schemes with the
static index which can have customized URI schemes, which would make file
proximity scoring based on URIs easier.
Reviewers: sammccall
Reviewed By: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, cfe-commits
Differential Revision: https://reviews.llvm.org/D47931
Modified:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/index/FileIndex.cpp
clang-tools-extra/trunk/clangd/index/FileIndex.h
clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
clang-tools-extra/trunk/unittests/clangd/FileIndexTests.cpp
clang-tools-extra/trunk/unittests/clangd/SymbolCollectorTests.cpp
clang-tools-extra/trunk/unittests/clangd/TestFS.cpp
clang-tools-extra/trunk/unittests/clangd/TestFS.h
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=334809=334808=334809=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.cpp (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.cpp Fri Jun 15 01:55:00 2018
@@ -87,7 +87,8 @@ ClangdServer::ClangdServer(GlobalCompila
: CDB(CDB), DiagConsumer(DiagConsumer), FSProvider(FSProvider),
ResourceDir(Opts.ResourceDir ? Opts.ResourceDir->str()
: getStandardResourceDir()),
- FileIdx(Opts.BuildDynamicSymbolIndex ? new FileIndex() : nullptr),
+ FileIdx(Opts.BuildDynamicSymbolIndex ? new FileIndex(Opts.URISchemes)
+ : nullptr),
PCHs(std::make_shared()),
// Pass a callback into `WorkScheduler` to extract symbols from a newly
// parsed file and rebuild the file index synchronously each time an AST
Modified: clang-tools-extra/trunk/clangd/ClangdServer.h
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/ClangdServer.h?rev=334809=334808=334809=diff
==
--- clang-tools-extra/trunk/clangd/ClangdServer.h (original)
+++ clang-tools-extra/trunk/clangd/ClangdServer.h Fri Jun 15 01:55:00 2018
@@ -79,6 +79,10 @@ public:
/// opened files and uses the index to augment code completion results.
bool BuildDynamicSymbolIndex = false;
+/// URI schemes to use when building the dynamic index.
+/// If empty, the default schemes in SymbolCollector will be used.
+std::vector URISchemes;
+
/// If set, use this index to augment code completion results.
SymbolIndex *StaticIndex = nullptr;
Modified: clang-tools-extra/trunk/clangd/index/FileIndex.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/FileIndex.cpp?rev=334809=334808=334809=diff
==
--- clang-tools-extra/trunk/clangd/index/FileIndex.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/FileIndex.cpp Fri Jun 15 01:55:00 2018
@@ -15,7 +15,8 @@
namespace clang {
namespace clangd {
-SymbolSlab indexAST(ASTContext , std::shared_ptr PP) {
+SymbolSlab indexAST(ASTContext , std::shared_ptr PP,
+llvm::ArrayRef URISchemes) {
SymbolCollector::Options CollectorOpts;
// FIXME(ioeric): we might also want to collect include headers. We would
need
// to make sure all includes are canonicalized (with CanonicalIncludes),
which
@@ -24,6 +25,8 @@ SymbolSlab indexAST(ASTContext , std
// CommentHandler for IWYU pragma) to canonicalize includes.
CollectorOpts.CollectIncludePath = false;
CollectorOpts.CountReferences = false;
+ if (!URISchemes.empty())
+CollectorOpts.URISchemes = URISchemes;
SymbolCollector Collector(std::move(CollectorOpts));
Collector.setPreprocessor(PP);
@@ -41,6 +44,9 @@ SymbolSlab indexAST(ASTContext , std
return Collector.takeSymbols();
}
+FileIndex::FileIndex(std::vector URISchemes)
+: URISchemes(std::move(URISchemes)) {}
+
void FileSymbols::update(PathRef Path, std::unique_ptr Slab) {
std::lock_guard Lock(Mutex);
if (!Slab)
@@ -79,7 +85,7 @@ void FileIndex::update(PathRef Path, AST
} else {
assert(PP);
auto Slab = llvm::make_unique();
-*Slab = indexAST(*AST, PP);
+*Slab = indexAST(*AST, PP, URISchemes);
FSymbols.update(Path, std::move(Slab));
}
auto Symbols = FSymbols.allSymbols();
Modified: clang-tools-extra/trunk/clangd/index/FileIndex.h
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/FileIndex.h?rev=334809=334808=334809=diff
[PATCH] D47931: [clangd] Customizable URI schemes for dynamic index.
ioeric updated this revision to Diff 151467.
ioeric marked an inline comment as done.
ioeric added a comment.
- Pulled unittest URI refactoring from https://reviews.llvm.org/D47935
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47931
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/index/FileIndex.cpp
clangd/index/FileIndex.h
unittests/clangd/ClangdTests.cpp
unittests/clangd/FileIndexTests.cpp
unittests/clangd/SymbolCollectorTests.cpp
unittests/clangd/TestFS.cpp
unittests/clangd/TestFS.h
unittests/clangd/URITests.cpp
Index: unittests/clangd/URITests.cpp
===
--- unittests/clangd/URITests.cpp
+++ unittests/clangd/URITests.cpp
@@ -14,46 +14,19 @@
namespace clang {
namespace clangd {
+
+// Force the unittest URI scheme to be linked,
+static int LLVM_ATTRIBUTE_UNUSED UnittestSchemeAnchorDest =
+UnittestSchemeAnchorSource;
+
namespace {
using ::testing::AllOf;
MATCHER_P(Scheme, S, "") { return arg.scheme() == S; }
MATCHER_P(Authority, A, "") { return arg.authority() == A; }
MATCHER_P(Body, B, "") { return arg.body() == B; }
-// Assume all files in the schema have a "test-root/" root directory, and the
-// schema path is the relative path to the root directory.
-// So the schema of "/some-dir/test-root/x/y/z" is "test:x/y/z".
-class TestScheme : public URIScheme {
-public:
- static const char *Scheme;
-
- static const char *TestRoot;
-
- llvm::Expected
- getAbsolutePath(llvm::StringRef /*Authority*/, llvm::StringRef Body,
- llvm::StringRef HintPath) const override {
-auto Pos = HintPath.find(TestRoot);
-assert(Pos != llvm::StringRef::npos);
-return (HintPath.substr(0, Pos + llvm::StringRef(TestRoot).size()) + Body)
-.str();
- }
-
- llvm::Expected
- uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
-auto Pos = AbsolutePath.find(TestRoot);
-assert(Pos != llvm::StringRef::npos);
-return URI(Scheme, /*Authority=*/"",
- AbsolutePath.substr(Pos + llvm::StringRef(TestRoot).size()));
- }
-};
-
-const char *TestScheme::Scheme = "unittest";
-const char *TestScheme::TestRoot = "/test-root/";
-
-static URISchemeRegistry::Add X(TestScheme::Scheme, "Test schema");
-
std::string createOrDie(llvm::StringRef AbsolutePath,
llvm::StringRef Scheme = "file") {
auto Uri = URI::create(AbsolutePath, Scheme);
@@ -167,12 +140,12 @@
#else
EXPECT_EQ(resolveOrDie(parseOrDie("file:/a/b/c")), "/a/b/c");
EXPECT_EQ(resolveOrDie(parseOrDie("file://auth/a/b/c")), "/a/b/c");
- EXPECT_EQ(resolveOrDie(parseOrDie("unittest:a/b/c"), "/dir/test-root/x/y/z"),
-"/dir/test-root/a/b/c");
EXPECT_THAT(resolveOrDie(parseOrDie("file://au%3dth/%28x%29/y/%20z")),
"/(x)/y/ z");
EXPECT_THAT(resolveOrDie(parseOrDie("file:///c:/x/y/z")), "c:/x/y/z");
#endif
+ EXPECT_EQ(resolveOrDie(parseOrDie("unittest:a"), testPath("x")),
+testPath("a"));
}
TEST(URITest, Platform) {
Index: unittests/clangd/TestFS.h
===
--- unittests/clangd/TestFS.h
+++ unittests/clangd/TestFS.h
@@ -56,6 +56,11 @@
// Returns a suitable absolute path for this OS.
std::string testPath(PathRef File);
+// unittest: is a scheme that refers to files relative to testRoot()
+// This anchor is used to force the linker to link in the generated object file
+// and thus register unittest: URI scheme plugin.
+extern volatile int UnittestSchemeAnchorSource;
+
} // namespace clangd
} // namespace clang
#endif
Index: unittests/clangd/TestFS.cpp
===
--- unittests/clangd/TestFS.cpp
+++ unittests/clangd/TestFS.cpp
@@ -7,7 +7,11 @@
//
//===--===//
#include "TestFS.h"
+#include "URI.h"
+#include "clang/AST/DeclCXX.h"
+#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Errc.h"
+#include "llvm/Support/Path.h"
#include "gtest/gtest.h"
namespace clang {
@@ -62,5 +66,38 @@
return Path.str();
}
+/// unittest: is a scheme that refers to files relative to testRoot()
+class TestScheme : public URIScheme {
+public:
+ static const char *Scheme;
+
+ llvm::Expected
+ getAbsolutePath(llvm::StringRef /*Authority*/, llvm::StringRef Body,
+ llvm::StringRef HintPath) const override {
+assert(HintPath.startswith(testRoot()));
+llvm::SmallString<16> Path(Body.begin(), Body.end());
+llvm::sys::path::native(Path);
+return testPath(Path);
+ }
+
+ llvm::Expected
+ uriFromAbsolutePath(llvm::StringRef AbsolutePath) const override {
+llvm::StringRef Body = AbsolutePath;
+if (!Body.consume_front(testRoot()))
+ return llvm::make_error(
+ AbsolutePath + "does not start with " + testRoot(),
+ llvm::inconvertibleErrorCode());
+
+return URI(Scheme,
[PATCH] D47931: [clangd] Customizable URI schemes for dynamic index.
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: unittests/clangd/FileIndexTests.cpp:99
+class TestURIScheme : public URIScheme {
+public:
This can use the shared unittest: scheme from the followup patch, right?
Add a fixme or lift that into this patch?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47931
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48171: [clangd] Do not report comments that only have special chars.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL334807: [clangd] Do not report comments that only have
special chars. (authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D48171
Files:
clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -1100,6 +1100,65 @@
Contains(AllOf(Not(IsDocumented()), Named("func";
}
+TEST(CompletionTest, NonDocComments) {
+ MockFSProvider FS;
+ auto FooCpp = testPath("foo.cpp");
+ FS.Files[FooCpp] = "";
+
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+ Annotations Source(R"cpp(
+// --
+int comments_foo();
+
+// A comment and a decl are separated by newlines.
+// Therefore, the comment shouldn't show up as doc comment.
+
+int comments_bar();
+
+// this comment should be in the results.
+int comments_baz();
+
+
+template
+struct Struct {
+ int comments_qux();
+ int comments_quux();
+};
+
+
+// This comment should not be there.
+
+template
+int Struct::comments_qux() {
+}
+
+// This comment **should** be in results.
+template
+int Struct::comments_quux() {
+ int a = comments^;
+}
+ )cpp");
+ Server.addDocument(FooCpp, Source.code(), WantDiagnostics::Yes);
+ CompletionList Completions = cantFail(runCodeComplete(
+ Server, FooCpp, Source.point(), clangd::CodeCompleteOptions()));
+
+ // We should not get any of those comments in completion.
+ EXPECT_THAT(
+ Completions.items,
+ UnorderedElementsAre(AllOf(Not(IsDocumented()), Named("comments_foo")),
+ AllOf(IsDocumented(), Named("comments_baz")),
+ AllOf(IsDocumented(), Named("comments_quux")),
+ // FIXME(ibiryukov): the following items should have
+ // empty documentation, since they are separated from
+ // a comment with an empty line. Unfortunately, I
+ // couldn't make Sema tests pass if we ignore those.
+ AllOf(IsDocumented(), Named("comments_bar")),
+ AllOf(IsDocumented(), Named("comments_qux";
+}
+
TEST(CompletionTest, CompleteOnInvalidLine) {
auto FooCpp = testPath("foo.cpp");
Index: clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp
===
--- clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp
+++ clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp
@@ -151,6 +151,16 @@
const ObjCPropertyDecl *PDecl = M ? M->findPropertyDecl() : nullptr;
return !PDecl || canRequestForDecl(*PDecl);
}
+
+bool LooksLikeDocComment(llvm::StringRef CommentText) {
+ // We don't report comments that only contain "special" chars.
+ // This avoids reporting various delimiters, like:
+ // =
+ // -
+ // *
+ return CommentText.find_first_not_of("/*-= \t\r\n") != llvm::StringRef::npos;
+}
+
} // namespace
std::string getDocComment(const ASTContext ,
@@ -167,7 +177,10 @@
const RawComment *RC = getCompletionComment(Ctx, Decl);
if (!RC)
return "";
- return RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
+ std::string Doc = RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
+ if (!LooksLikeDocComment(Doc))
+return "";
+ return Doc;
}
std::string
@@ -180,7 +193,10 @@
const RawComment *RC = getParameterComment(Ctx, Result, ArgIndex);
if (!RC)
return "";
- return RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
+ std::string Doc = RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
+ if (!LooksLikeDocComment(Doc))
+return "";
+ return Doc;
}
void getLabelAndInsertText(const CodeCompletionString , std::string *Label,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] r334807 - [clangd] Do not report comments that only have special chars.
Author: ibiryukov
Date: Fri Jun 15 01:31:17 2018
New Revision: 334807
URL: http://llvm.org/viewvc/llvm-project?rev=334807=rev
Log:
[clangd] Do not report comments that only have special chars.
Summary:
Like the following:
// ---
// ===
// ***
It does not cover all the cases, but those are definitely not very
useful.
Reviewers: sammccall, ioeric, hokein
Reviewed By: sammccall
Subscribers: MaskRay, jkorous, cfe-commits
Differential Revision: https://reviews.llvm.org/D48171
Modified:
clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp
clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
Modified: clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp?rev=334807=334806=334807=diff
==
--- clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp (original)
+++ clang-tools-extra/trunk/clangd/CodeCompletionStrings.cpp Fri Jun 15
01:31:17 2018
@@ -151,6 +151,16 @@ bool canRequestComment(const ASTContext
const ObjCPropertyDecl *PDecl = M ? M->findPropertyDecl() : nullptr;
return !PDecl || canRequestForDecl(*PDecl);
}
+
+bool LooksLikeDocComment(llvm::StringRef CommentText) {
+ // We don't report comments that only contain "special" chars.
+ // This avoids reporting various delimiters, like:
+ // =
+ // -
+ // *
+ return CommentText.find_first_not_of("/*-= \t\r\n") != llvm::StringRef::npos;
+}
+
} // namespace
std::string getDocComment(const ASTContext ,
@@ -167,7 +177,10 @@ std::string getDocComment(const ASTConte
const RawComment *RC = getCompletionComment(Ctx, Decl);
if (!RC)
return "";
- return RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
+ std::string Doc = RC->getFormattedText(Ctx.getSourceManager(),
Ctx.getDiagnostics());
+ if (!LooksLikeDocComment(Doc))
+return "";
+ return Doc;
}
std::string
@@ -180,7 +193,10 @@ getParameterDocComment(const ASTContext
const RawComment *RC = getParameterComment(Ctx, Result, ArgIndex);
if (!RC)
return "";
- return RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
+ std::string Doc = RC->getFormattedText(Ctx.getSourceManager(),
Ctx.getDiagnostics());
+ if (!LooksLikeDocComment(Doc))
+return "";
+ return Doc;
}
void getLabelAndInsertText(const CodeCompletionString , std::string *Label,
Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
URL:
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=334807=334806=334807=diff
==
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Fri Jun 15
01:31:17 2018
@@ -1100,6 +1100,65 @@ TEST(CompletionTest, DocumentationFromCh
Contains(AllOf(Not(IsDocumented()), Named("func";
}
+TEST(CompletionTest, NonDocComments) {
+ MockFSProvider FS;
+ auto FooCpp = testPath("foo.cpp");
+ FS.Files[FooCpp] = "";
+
+ MockCompilationDatabase CDB;
+ IgnoreDiagnostics DiagConsumer;
+ ClangdServer Server(CDB, FS, DiagConsumer, ClangdServer::optsForTest());
+
+ Annotations Source(R"cpp(
+// --
+int comments_foo();
+
+// A comment and a decl are separated by newlines.
+// Therefore, the comment shouldn't show up as doc comment.
+
+int comments_bar();
+
+// this comment should be in the results.
+int comments_baz();
+
+
+template
+struct Struct {
+ int comments_qux();
+ int comments_quux();
+};
+
+
+// This comment should not be there.
+
+template
+int Struct::comments_qux() {
+}
+
+// This comment **should** be in results.
+template
+int Struct::comments_quux() {
+ int a = comments^;
+}
+ )cpp");
+ Server.addDocument(FooCpp, Source.code(), WantDiagnostics::Yes);
+ CompletionList Completions = cantFail(runCodeComplete(
+ Server, FooCpp, Source.point(), clangd::CodeCompleteOptions()));
+
+ // We should not get any of those comments in completion.
+ EXPECT_THAT(
+ Completions.items,
+ UnorderedElementsAre(AllOf(Not(IsDocumented()), Named("comments_foo")),
+ AllOf(IsDocumented(), Named("comments_baz")),
+ AllOf(IsDocumented(), Named("comments_quux")),
+ // FIXME(ibiryukov): the following items should have
+ // empty documentation, since they are separated
from
+ // a comment with an empty line. Unfortunately, I
+ // couldn't make Sema tests pass if we ignore those.
+ AllOf(IsDocumented(), Named("comments_bar")),
+
[PATCH] D47957: [clangd] Add option to fold overloads into a single completion item.
ilya-biryukov added a comment.
Overall LG. It would be nice to mention somewhere that we since we bundle
overloads client-side we might run into weird side-effects (e.g. return less
completion items than the specified limit) in case the index returns too many
non-bundled.
Comment at: clangd/tool/ClangdMain.cpp:62
+// TODO: also support "plain" style where signatures are always omitted.
+enum CompletionStyleFlag {
NIT: maybe use FIXME to be consistent with the rest of clangd?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47957
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47957: [clangd] Add option to fold overloads into a single completion item.
ilya-biryukov added a comment.
Oh, sorry, I forgot to submit the comments yesterday :-(
Comment at: clangd/CodeComplete.cpp:245
+// Methods are simply grouped by name.
+return hash_combine('M', IndexResult->Name);
+ case index::SymbolKind::Function:
Members never come from the index for completion, right? Maybe add an assert
here instead?
Comment at: clangd/CodeComplete.cpp:265
+ :
StringRef(D->getDeclName().getAsString()));
+return hash_combine('F', StringRef(D->getQualifiedNameAsString()),
+headerToInsertIfNotPresent().getValueOr(""));
`getQualifiedNameAsString` has a fixme, suggesting it's deprecated. Maybe we
should call `printName` directly instead? And we have `Scratch` for storage
anyway, so it might be a little faster too.
Comment at: clangd/CodeComplete.cpp:366
+Opts.EnableSnippets ? (Name + "(${0})").str() : Name.str();
+First.label = (Name + "(...)").str();
+First.detail = llvm::formatv("[{0} overloads]", Bundle.size());
Maybe use a unicode ellipsis char (…) here? Or even come up with something
else. `...` is a valid syntax in C++, might be little confusing. Especially in
VSCode, where detail is not shown.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47957
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45202: [X86] Replacing X86-specific floor and ceil vector intrinsics with generic LLVM intrinsics
mike.dvoretsky abandoned this revision. mike.dvoretsky added a comment. Abandoning this due to https://reviews.llvm.org/D48067 being accepted instead. https://reviews.llvm.org/D45202 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47935: [clangd] Boost completion score according to file proximity.
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks! Just nits
Comment at: clangd/Quality.cpp:304
OS << formatv("\tForbidden: {0}\n", S.Forbidden);
- OS << formatv("\tProximity: {0}\n", S.ProximityScore);
+ OS << formatv("\tIndexSymbolURI: {0}\n", S.IndexSymbolURI);
+ if (S.FileProximityMatch) {
No camel case here, just words
Comment at: clangd/Quality.cpp:305
+ OS << formatv("\tIndexSymbolURI: {0}\n", S.IndexSymbolURI);
+ if (S.FileProximityMatch) {
+OS << "\t" << *S.FileProximityMatch << "\n";
Could this just be inlined like the others?
`Index proximity: 0.5 (ProximityRoots{foo/bar.h})`
Comment at: clangd/Quality.h:71
+class FileProximityMatcher {
+ public:
nit: can we forward declare this here and move it down (e.g. above TopN) to
keep the signals at the top?
(I suspect it'll end up in another header eventually)
Comment at: clangd/Quality.h:99
+ /// query.
+ llvm::StringRef IndexSymbolURI;
/// Proximity between best declaration and the query. [0-1], 1 is closest.
nit: just SymbolURI (signals should be conceptually source-independent, may be
missing)
Comment at: clangd/Quality.h:101
/// Proximity between best declaration and the query. [0-1], 1 is closest.
- float ProximityScore = 0;
+ float SemaProximityScore = 0;
can you add a FIXME to unify with index proximity score? signals should be
source-independent
Comment at: unittests/clangd/TestFS.h:60
+// This anchor is used to force the linker to link in the generated object file
+// and thus register unittest: URI scheme plugin.
+extern volatile int UnittestSchemeAnchorSource;
document the unittest: scheme here?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D47935
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing added a comment. Ping https://reviews.llvm.org/D47687 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing updated this revision to Diff 151460.
Higuoxing added a comment.
I think I am almost there.
Here, In my sight
#define foo(op0, op1, x, y, z) ( (void)(x op0 y op1 z) )
is un-actionable, because `x op0 y op1 z` are from different arguments of
function-like macro, so, we will not check parentheses for op0 or op1 when we
call it by
foo(&&, ||, x, y, z)
but if we call it by
foo(&&, ||, x && y ||z, y, z)
it is actionable, because argument (x && y || z) is from the same arg position
of macro foo;
hence we should check `x && y || z` but shouldn't check parentheses for the
first argument (&&) and second argument (||)
I think this could cover bunch of cases. But I think my code is not so
beautiful... So, is there any suggestion?
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/logical-op-parentheses-in-macros.c
Index: test/Sema/logical-op-parentheses-in-macros.c
===
--- test/Sema/logical-op-parentheses-in-macros.c
+++ test/Sema/logical-op-parentheses-in-macros.c
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -Wparentheses -fsyntax-only -verify=logical_op_parentheses_check %s
+
+// operator of this case is expanded from macro function argument
+#define macro_op_parentheses_check(x) ( \
+ ( (void)(x) ) \
+)
+
+// operator of this case is expanded from macro function body
+#define macro_op_parentheses_check_ops_args(op0, op1, x, y, z) ( \
+ ( (void) (x op0 y op1 z ) ) \
+)
+
+// operator of this case is expanded from macro function body
+#define macro_op_parentheses_check_ops_args2(op0, op1, op2, w, x, y, z) ( \
+ ( (void) (w op0 x op1 y op2 z) ) \
+)
+
+// operator of this case is expanded from marco body
+#define macro_op_parentheses_check_ops_body(x, y, z) ( \
+ ( (void) (x && y || z) ) \
+)
+
+void logical_op_from_macro_arguments(unsigned i) {
+ macro_op_parentheses_check(i || i && i); // logical_op_parentheses_check-warning {{'&&' within '||'}} \
+ // logical_op_parentheses_check-note {{place parentheses around the '&&' expression to silence this warning}}
+ macro_op_parentheses_check(i || i && "I love LLVM"); // no warning.
+ macro_op_parentheses_check("I love LLVM" && i || i); // no warning.
+
+ macro_op_parentheses_check(i || i && "I love LLVM" || i); // logical_op_parentheses_check-warning {{'&&' within '||'}} \
+// logical_op_parentheses_check-note {{place parentheses around the '&&' expression to silence this warning}}
+ macro_op_parentheses_check(i || "I love LLVM" && i || i); // logical_op_parentheses_check-warning {{'&&' within '||'}} \
+// logical_op_parentheses_check-note {{place parentheses around the '&&' expression to silence this warning}}
+ macro_op_parentheses_check(i && i || 0); // no warning.
+ macro_op_parentheses_check(0 || i && i); // no warning.
+}
+
+void logical_op_from_macro_arguments2(unsigned i) {
+ macro_op_parentheses_check_ops_args(||, &&, i, i, i); // no warning.
+ macro_op_parentheses_check_ops_args(||, &&, i, i, "I love LLVM"); // no warning.
+ macro_op_parentheses_check_ops_args(&&, ||, "I love LLVM", i, i); // no warning.
+
+ macro_op_parentheses_check_ops_args2(||, &&, ||, i, i, "I love LLVM", i); // no warning.
+ macro_op_parentheses_check_ops_args2(||, &&, ||, i, "I love LLVM", i, i); // no warning.
+
+ macro_op_parentheses_check_ops_args(&&, ||, i, i, 0); // no warning.
+ macro_op_parentheses_check_ops_args(||, &&, 0, i, i); // no warning.
+}
+
+void logical_op_from_macro_body(unsigned i) {
+ macro_op_parentheses_check_ops_body(i, i, i); // no warning.
+}
\ No newline at end of file
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -12172,7 +12172,7 @@
EmitDiagnosticForLogicalAndInLogicalOr(Sema , SourceLocation OpLoc,
BinaryOperator *Bop) {
assert(Bop->getOpcode() == BO_LAnd);
- Self.Diag(Bop->getOperatorLoc(), diag::warn_logical_and_in_logical_or)
+ Self.Diag(Bop->getOperatorLoc(), diag::warn_logical_and_in_logical_or)
<< Bop->getSourceRange() << OpLoc;
SuggestParentheses(Self, Bop->getOperatorLoc(),
Self.PDiag(diag::note_precedence_silence)
@@ -12205,6 +12205,7 @@
if (EvaluatesAsFalse(S, RHSExpr))
return;
// If it's "1 && a || b" don't warn since the precedence doesn't matter.
+ // And 'assert("some message" && a || b)' don't warn as well.
if (!EvaluatesAsTrue(S, Bop->getLHS()))
return EmitDiagnosticForLogicalAndInLogicalOr(S, OpLoc, Bop);
} else if (Bop->getOpcode() == BO_LOr) {
@@ -12227,6 +12228,7 @@
if (EvaluatesAsFalse(S, LHSExpr))
return;
// If it's "a || b && 1" don't warn since the precedence doesn't matter.
+ // And 'assert(a || b && "some
[PATCH] D47445: [ASTImporter] Corrected diagnostic client handling in tests.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC334804: [ASTImporter] Corrected diagnostic client handling
in tests. (authored by baloghadamsoftware, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D47445?vs=149412=151459#toc
Repository:
rC Clang
https://reviews.llvm.org/D47445
Files:
include/clang/Frontend/ASTUnit.h
lib/Frontend/ASTUnit.cpp
unittests/AST/ASTImporterTest.cpp
Index: lib/Frontend/ASTUnit.cpp
===
--- lib/Frontend/ASTUnit.cpp
+++ lib/Frontend/ASTUnit.cpp
@@ -275,6 +275,12 @@
this->PP = std::move(PP);
}
+void ASTUnit::enableSourceFileDiagnostics() {
+ assert(getDiagnostics().getClient() && Ctx &&
+ "Bad context for source file");
+ getDiagnostics().getClient()->BeginSourceFile(Ctx->getLangOpts(), PP.get());
+}
+
/// Determine the set of code-completion contexts in which this
/// declaration should be shown.
static unsigned getDeclShowContexts(const NamedDecl *ND,
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -98,6 +98,9 @@
ASTContext = FromAST->getASTContext(),
= ToAST->getASTContext();
+ FromAST->enableSourceFileDiagnostics();
+ ToAST->enableSourceFileDiagnostics();
+
ASTImporter Importer(ToCtx, ToAST->getFileManager(),
FromCtx, FromAST->getFileManager(), false);
@@ -172,7 +175,9 @@
: Code(Code), FileName(FileName),
Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
this->FileName)),
- TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {}
+ TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {
+ Unit->enableSourceFileDiagnostics();
+}
};
// We may have several From contexts and related translation units. In each
@@ -214,6 +219,7 @@
ToCode = ToSrcCode;
assert(!ToAST);
ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
+ToAST->enableSourceFileDiagnostics();
ASTContext = FromTU.Unit->getASTContext(),
= ToAST->getASTContext();
@@ -261,6 +267,7 @@
ToCode = ToSrcCode;
assert(!ToAST);
ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
+ToAST->enableSourceFileDiagnostics();
return ToAST->getASTContext().getTranslationUnitDecl();
}
@@ -274,6 +281,7 @@
// Build the AST from an empty file.
ToAST =
tooling::buildASTFromCodeWithArgs(/*Code=*/"", ToArgs, "empty.cc");
+ ToAST->enableSourceFileDiagnostics();
}
// Create a virtual file in the To Ctx which corresponds to the file from
Index: include/clang/Frontend/ASTUnit.h
===
--- include/clang/Frontend/ASTUnit.h
+++ include/clang/Frontend/ASTUnit.h
@@ -438,6 +438,15 @@
void setASTContext(ASTContext *ctx) { Ctx = ctx; }
void setPreprocessor(std::shared_ptr pp);
+ /// Enable source-range based diagnostic messages.
+ ///
+ /// If diagnostic messages with source-range information are to be expected
+ /// and AST comes not from file (e.g. after LoadFromCompilerInvocation) this
+ /// function has to be called.
+ /// The function is to be called only once and the AST should be associated
+ /// with the same source file afterwards.
+ void enableSourceFileDiagnostics();
+
bool hasSema() const { return (bool)TheSema; }
Sema () const {
Index: lib/Frontend/ASTUnit.cpp
===
--- lib/Frontend/ASTUnit.cpp
+++ lib/Frontend/ASTUnit.cpp
@@ -275,6 +275,12 @@
this->PP = std::move(PP);
}
+void ASTUnit::enableSourceFileDiagnostics() {
+ assert(getDiagnostics().getClient() && Ctx &&
+ "Bad context for source file");
+ getDiagnostics().getClient()->BeginSourceFile(Ctx->getLangOpts(), PP.get());
+}
+
/// Determine the set of code-completion contexts in which this
/// declaration should be shown.
static unsigned getDeclShowContexts(const NamedDecl *ND,
Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -98,6 +98,9 @@
ASTContext = FromAST->getASTContext(),
= ToAST->getASTContext();
+ FromAST->enableSourceFileDiagnostics();
+ ToAST->enableSourceFileDiagnostics();
+
ASTImporter Importer(ToCtx, ToAST->getFileManager(),
FromCtx, FromAST->getFileManager(), false);
@@ -172,7 +175,9 @@
: Code(Code), FileName(FileName),
Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
this->FileName)),
- TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {}
+
