r334886 - Add RUN line for amdgcn to lit test conditional-temporaries.cpp
Author: yaxunl
Date: Sat Jun 16 05:28:51 2018
New Revision: 334886
URL: http://llvm.org/viewvc/llvm-project?rev=334886&view=rev
Log:
Add RUN line for amdgcn to lit test conditional-temporaries.cpp
This is partial re-commit of r332982.
Modified:
cfe/trunk/test/CodeGenCXX/conditional-temporaries.cpp
Modified: cfe/trunk/test/CodeGenCXX/conditional-temporaries.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/conditional-temporaries.cpp?rev=334886&r1=334885&r2=334886&view=diff
==
--- cfe/trunk/test/CodeGenCXX/conditional-temporaries.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/conditional-temporaries.cpp Sat Jun 16 05:28:51
2018
@@ -1,4 +1,6 @@
+// REQUIRES: amdgpu-registered-target
// RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-apple-darwin9 -O3 |
FileCheck %s
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple=amdgcn-amd-amdhsa -O3 |
FileCheck %s
namespace {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48221: [analyzer] Add method to the generic SMT API to dump the SMT formula
This revision was automatically updated to reflect the committed changes.
Closed by commit rL334891: [analyzer] Add method to the generic SMT API to dump
the SMT formula (authored by mramalho, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D48221?vs=151531&id=151618#toc
Repository:
rL LLVM
https://reviews.llvm.org/D48221
Files:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
Index:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
===
---
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
+++
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
@@ -35,6 +35,8 @@
/// Checks if the added constraints are satisfiable
virtual clang::ento::ConditionTruthVal isModelFeasible() = 0;
+ /// Dumps SMT formula
+ LLVM_DUMP_METHOD virtual void dump() const = 0;
}; // end class SMTConstraintManager
} // namespace ento
Index: cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
@@ -911,6 +911,8 @@
ConditionTruthVal isModelFeasible() override;
+ LLVM_DUMP_METHOD void dump() const override;
+
//===--===//
// Implementation for interface from ConstraintManager.
//===--===//
@@ -1299,6 +1301,11 @@
return ConditionTruthVal();
}
+LLVM_DUMP_METHOD void Z3ConstraintManager::dump() const
+{
+ Solver.dump();
+}
+
//===--===//
// Internal implementation.
//===--===//
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
@@ -35,6 +35,8 @@
/// Checks if the added constraints are satisfiable
virtual clang::ento::ConditionTruthVal isModelFeasible() = 0;
+ /// Dumps SMT formula
+ LLVM_DUMP_METHOD virtual void dump() const = 0;
}; // end class SMTConstraintManager
} // namespace ento
Index: cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
@@ -911,6 +911,8 @@
ConditionTruthVal isModelFeasible() override;
+ LLVM_DUMP_METHOD void dump() const override;
+
//===--===//
// Implementation for interface from ConstraintManager.
//===--===//
@@ -1299,6 +1301,11 @@
return ConditionTruthVal();
}
+LLVM_DUMP_METHOD void Z3ConstraintManager::dump() const
+{
+ Solver.dump();
+}
+
//===--===//
// Internal implementation.
//===--===//
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48221: [analyzer] Add method to the generic SMT API to dump the SMT formula
This revision was automatically updated to reflect the committed changes.
Closed by commit rC334891: [analyzer] Add method to the generic SMT API to dump
the SMT formula (authored by mramalho, committed by ).
Herald added a subscriber: cfe-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D48221
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
@@ -35,6 +35,8 @@
/// Checks if the added constraints are satisfiable
virtual clang::ento::ConditionTruthVal isModelFeasible() = 0;
+ /// Dumps SMT formula
+ LLVM_DUMP_METHOD virtual void dump() const = 0;
}; // end class SMTConstraintManager
} // namespace ento
Index: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
===
--- lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
+++ lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
@@ -911,6 +911,8 @@
ConditionTruthVal isModelFeasible() override;
+ LLVM_DUMP_METHOD void dump() const override;
+
//===--===//
// Implementation for interface from ConstraintManager.
//===--===//
@@ -1299,6 +1301,11 @@
return ConditionTruthVal();
}
+LLVM_DUMP_METHOD void Z3ConstraintManager::dump() const
+{
+ Solver.dump();
+}
+
//===--===//
// Internal implementation.
//===--===//
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
@@ -35,6 +35,8 @@
/// Checks if the added constraints are satisfiable
virtual clang::ento::ConditionTruthVal isModelFeasible() = 0;
+ /// Dumps SMT formula
+ LLVM_DUMP_METHOD virtual void dump() const = 0;
}; // end class SMTConstraintManager
} // namespace ento
Index: lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
===
--- lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
+++ lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
@@ -911,6 +911,8 @@
ConditionTruthVal isModelFeasible() override;
+ LLVM_DUMP_METHOD void dump() const override;
+
//===--===//
// Implementation for interface from ConstraintManager.
//===--===//
@@ -1299,6 +1301,11 @@
return ConditionTruthVal();
}
+LLVM_DUMP_METHOD void Z3ConstraintManager::dump() const
+{
+ Solver.dump();
+}
+
//===--===//
// Internal implementation.
//===--===//
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
r334891 - [analyzer] Add method to the generic SMT API to dump the SMT formula
Author: mramalho
Date: Sat Jun 16 07:36:17 2018
New Revision: 334891
URL: http://llvm.org/viewvc/llvm-project?rev=334891&view=rev
Log:
[analyzer] Add method to the generic SMT API to dump the SMT formula
Summary:
New method dump the SMT formula and the Z3 implementation.
There is no test because I only used it for debugging.
However, if requested, I can add an option to the static analyzer to dump the
formula (whole program? per path?), maybe something like the trimmed graph but
for SMT formulas.
Reviewers: NoQ, george.karpenkov, ddcc
Reviewed By: george.karpenkov
Subscribers: xazax.hun, szepet, a.sidorin
Differential Revision: https://reviews.llvm.org/D48221
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h?rev=334891&r1=334890&r2=334891&view=diff
==
---
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
(original)
+++
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
Sat Jun 16 07:36:17 2018
@@ -35,6 +35,8 @@ public:
/// Checks if the added constraints are satisfiable
virtual clang::ento::ConditionTruthVal isModelFeasible() = 0;
+ /// Dumps SMT formula
+ LLVM_DUMP_METHOD virtual void dump() const = 0;
}; // end class SMTConstraintManager
} // namespace ento
Modified: cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp
URL:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp?rev=334891&r1=334890&r2=334891&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/Z3ConstraintManager.cpp Sat Jun 16
07:36:17 2018
@@ -911,6 +911,8 @@ public:
ConditionTruthVal isModelFeasible() override;
+ LLVM_DUMP_METHOD void dump() const override;
+
//===--===//
// Implementation for interface from ConstraintManager.
//===--===//
@@ -1299,6 +1301,11 @@ clang::ento::ConditionTruthVal Z3Constra
return ConditionTruthVal();
}
+LLVM_DUMP_METHOD void Z3ConstraintManager::dump() const
+{
+ Solver.dump();
+}
+
//===--===//
// Internal implementation.
//===--===//
___
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:1647 + } +} + rjmccall wrote: > 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. AFAIK reinterpreting one class as another is UB if they are not in hierarchy (especially calling virtual function on reinterpreted class), not sure if strict aliasing should allow it anyway (if it would be a hand written custom vptr then it should be ok with strict aliasing turned off, but with vptr I don't think it is legal). @rsmith Can you comment on 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
[libcxx] r334894 - Remove P0771, which was not passed in Rapperswil
Author: marshall Date: Sat Jun 16 11:03:29 2018 New Revision: 334894 URL: http://llvm.org/viewvc/llvm-project?rev=334894&view=rev Log: Remove P0771, which was not passed in Rapperswil Modified: libcxx/trunk/www/cxx2a_status.html Modified: libcxx/trunk/www/cxx2a_status.html URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/www/cxx2a_status.html?rev=334894&r1=334893&r2=334894&view=diff == --- libcxx/trunk/www/cxx2a_status.html (original) +++ libcxx/trunk/www/cxx2a_status.html Sat Jun 16 11:03:29 2018 @@ -96,7 +96,6 @@ https://wg21.link/P0758R1";>P0758R1LWGImplicit conversion traits and utility functionsRapperswil https://wg21.link/P0759R1";>P0759R1LWGfpos RequirementsRapperswil https://wg21.link/P0769R2";>P0769R2LWGAdd shift toRapperswil - https://wg21.link/P0771R0";>P0771R0LWGstd::function move operations should be noexceptRapperswil https://wg21.link/P0788R3";>P0788R3LWGStandard Library Specification in a Concepts and Contracts WorldRapperswil https://wg21.link/P0879R0";>P0879R0LWGConstexpr for swap and swap related functions Also resolves LWG issue 2800.Rapperswil https://wg21.link/P0887R1";>P0887R1LWGThe identity metafunctionRapperswil ___ 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
whisperity accepted this revision. whisperity added a comment. This revision is now accepted and ready to land. Ah, and the function names in the test files have been made more logical. Let's roll. 😉 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] D48259: [clang-format] Fix bug with UT_Always when there is less than one full tab
guigu created this revision.
guigu added reviewers: klimek, djasper.
guigu added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
There's a bug in the `WhitespaceManager` that prevents correct alignment when
using tab.
This patch fix a test condition that was triggering the bug and add missing
unit tests that would have failed.
The issue can be seen by invoking clang-format on this simple snippet with
`-style="{BasedOnStyle: llvm, UseTab: Always, AlignConsecutiveAssignments:
true, AlignConsecutiveDeclarations: true, TabWidth: 4}"`
int a = 42;
int aa = 42;
int aaa = 42;
int = 42;
Displaying the result in a editor with a tabsize of 4, the first = won't be
aligned correctly.
The logic in `WhitespaceManager::appendIndentText` treats the first tab as a
special case, which is what must be done in order to handle tabstop correctly.
However, handling the first tab is done conditionally using the first tab width.
The proposed fix is to handle the first tab independently of its size as the
code that follow this test...
Text.append(Spaces / Style.TabWidth, '\t');
...will only work if the `'\t'` we're adding are positionned on a tabstop (so
`'\t'` is equal to **TabWidth** in the output, otherwise it will be between 1
and **TabWidth** - 1)
Repository:
rC Clang
https://reviews.llvm.org/D48259
Files:
lib/Format/WhitespaceManager.cpp
unittests/Format/FormatTest.cpp
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9367,6 +9367,18 @@
"int oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+ // Test with use of tabs for alignment
+ FormatStyle::UseTabStyle OldTabStyle = Alignment.UseTab;
+ unsigned OldTabWidth = Alignment.TabWidth;
+ Alignment.UseTab = FormatStyle::UT_Always;
+ Alignment.TabWidth = 4;
+ verifyFormat("int a\t = 42;\n"
+ "int aa = 42;\n"
+ "int aaa = 42;\n"
+ "int = 42;",
+ Alignment);
+ Alignment.UseTab = OldTabStyle;
+ Alignment.TabWidth = OldTabWidth;
Alignment.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
verifyFormat("#define A \\\n"
" int = 12; \\\n"
@@ -9542,6 +9554,18 @@
"unsigned oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+ // Test with use of tabs for alignment
+ FormatStyle::UseTabStyle OldTabStyle = Alignment.UseTab;
+ unsigned OldTabWidth = Alignment.TabWidth;
+ Alignment.UseTab = FormatStyle::UT_Always;
+ Alignment.TabWidth = 4;
+ verifyFormat("int\t a = 42;\n"
+ "float b = 42;\n"
+ "char c = 42;\n"
+ "double d = 42;",
+ Alignment);
+ Alignment.UseTab = OldTabStyle;
+ Alignment.TabWidth = OldTabWidth;
// Function prototype alignment
verifyFormat("inta();\n"
"double b();",
Index: lib/Format/WhitespaceManager.cpp
===
--- lib/Format/WhitespaceManager.cpp
+++ lib/Format/WhitespaceManager.cpp
@@ -675,7 +675,7 @@
unsigned FirstTabWidth =
Style.TabWidth - WhitespaceStartColumn % Style.TabWidth;
// Indent with tabs only when there's at least one full tab.
-if (FirstTabWidth + Style.TabWidth <= Spaces) {
+if (Style.TabWidth <= Spaces) {
Spaces -= FirstTabWidth;
Text.append("\t");
}
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9367,6 +9367,18 @@
"int oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+ // Test with use of tabs for alignment
+ FormatStyle::UseTabStyle OldTabStyle = Alignment.UseTab;
+ unsigned OldTabWidth = Alignment.TabWidth;
+ Alignment.UseTab = FormatStyle::UT_Always;
+ Alignment.TabWidth = 4;
+ verifyFormat("int a\t = 42;\n"
+ "int aa = 42;\n"
+ "int aaa = 42;\n"
+ "int = 42;",
+ Alignment);
+ Alignment.UseTab = OldTabStyle;
+ Alignment.TabWidth = OldTabWidth;
Alignment.AlignEscapedNewlines = FormatStyle::ENAS_DontAlign;
verifyFormat("#define A \\\n"
" int = 12; \\\n"
@@ -9542,6 +9554,18 @@
"unsigned oneTwoThree = 123;\n"
"int oneTwo = 12;",
Alignment));
+ // Test with use of tabs for alignment
+ FormatStyle::UseTabStyle OldTabStyle = Alignment.UseTab;
+ unsigned OldTabWidth = Alignment.TabWidth;
+ Alignment.UseTab = FormatStyle::UT_Always;
+ Alignment.TabWidth = 4;
+ verifyFormat("int\t a = 42;\n"
+ "float b = 42;\n"
+ "char
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
dexonsmith requested changes to this revision.
dexonsmith added inline comments.
This revision now requires changes to proceed.
Comment at: lib/Sema/SemaExpr.cpp:12304-12309
// Diagnose "arg1 & arg2 | arg3"
if ((Opc == BO_Or || Opc == BO_Xor) &&
!OpLoc.isMacroID()/* Don't warn in macros. */) {
DiagnoseBitwiseOpInBitwiseOp(Self, Opc, OpLoc, LHSExpr);
DiagnoseBitwiseOpInBitwiseOp(Self, Opc, OpLoc, RHSExpr);
}
It seems unfortunate not to update this one at the same time. Or are you
planning that for a follow-up?
Can you think of a way to share the logic cleanly?
Comment at: lib/Sema/SemaExpr.cpp:12313
// 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()) {
I think the code inside this should be split out into a separate function
(straw man name: "diagnoseLogicalAndInLogicalOr") so that you can use early
returns and reduce nesting.
Comment at: lib/Sema/SemaExpr.cpp:12315
+if (!OpLoc.isMacroID()) {
+ // Operator is not in macros
+ DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
I don't think this comment is valuable. It's just repeating the code in the
condition.
Comment at: lib/Sema/SemaExpr.cpp:12319
+} else {
+ // This Expression is expanded from macro
+ SourceLocation LHSExpansionLoc, OpExpansionLoc, RHSExpansionLoc;
This comment is just repeating what's in the condition. I suggest replacing it
with something describing the logic that follows. Also, it's missing a period
at the end of the sentence.
Comment at: lib/Sema/SemaExpr.cpp:12321
+ SourceLocation LHSExpansionLoc, OpExpansionLoc, RHSExpansionLoc;
+ if ((Self.getSourceManager().isMacroArgExpansion(OpLoc,
+ &OpExpansionLoc) &&
This will be cleaner if you create a local reference to
`Self.getSourceManager()` called `SM`.
Comment at: lib/Sema/SemaExpr.cpp:12325-12328
+ (Self.getSourceManager().isMacroArgExpansion(OpLoc,
+ &OpExpansionLoc) &&
+ Self.getSourceManager().isMacroArgExpansion(RHSExpr->getExprLoc(),
+ &LHSExpansionLoc))) {
It's a little awkward to add this condition in with the previous one, and
you're also repeating a call to `isMacroArgExpansion` unnecessarily. I suggest
something like:
```
SourceLocation OpExpansion;
if (!SM.isMacroArgExpansion(OpLoc, &OpExpansion))
return;
SourceLocation ExprExpansion;
if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), &ExprExpansion) &&
OpExpansion == ExprExpansion)
DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
if (SM.isMacroArgExpansion(RHSExpr->getExprLoc(), &ExprExpansion) &&
OpExpansion == ExprExpansion)
DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
```
Comment at: test/Sema/parentheses.c:133
+ // expected-note {{place
parentheses around the '&&' expression to silence this warning}}
+ NESTING_VOID_WRAPPER(&&, ||, ((i && i) || i), i, i); // no waring.
+
typo: should be "no warning".
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 151633.
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,115 @@
(void)(i && i || 0); // no warning.
(void)(0 || i && i); // no warning.
+
+ //===--
+ // Logical operator in macros
+ //===--
+
+ 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);
+ // 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 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 warning.
+
+ // 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.
+
+ // 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.
+
+ // 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 not be checked because `op ||` and its rhs are not from same arg position
+ // ^ arg position 0 (&&)
+ //^ arg position 3 (i)
+ // ^
[PATCH] D47687: [Sema] Missing -Wlogical-op-parentheses warnings in macros (PR18971)
Higuoxing added inline comments.
Comment at: lib/Sema/SemaExpr.cpp:12304-12309
// Diagnose "arg1 & arg2 | arg3"
if ((Opc == BO_Or || Opc == BO_Xor) &&
!OpLoc.isMacroID()/* Don't warn in macros. */) {
DiagnoseBitwiseOpInBitwiseOp(Self, Opc, OpLoc, LHSExpr);
DiagnoseBitwiseOpInBitwiseOp(Self, Opc, OpLoc, RHSExpr);
}
dexonsmith wrote:
> It seems unfortunate not to update this one at the same time. Or are you
> planning that for a follow-up?
>
> Can you think of a way to share the logic cleanly?
Yes, I would like to update it after this patch being accepted, because I think
this patch is about `logical-op-parentheses` and this one is about
`bitwise-op`, and I think after this patch being accepted I will be more
confident on the code style, API using and various things : ) Of course, I
would like to help!
Comment at: lib/Sema/SemaExpr.cpp:12313
// 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()) {
dexonsmith wrote:
> I think the code inside this should be split out into a separate function
> (straw man name: "diagnoseLogicalAndInLogicalOr") so that you can use early
> returns and reduce nesting.
Yes, I add a function `DiagnoseLogicalAndInLogicalOr`, does the uppercase `D`
matter?
Comment at: lib/Sema/SemaExpr.cpp:12319
+} else {
+ // This Expression is expanded from macro
+ SourceLocation LHSExpansionLoc, OpExpansionLoc, RHSExpansionLoc;
dexonsmith wrote:
> This comment is just repeating what's in the condition. I suggest replacing
> it with something describing the logic that follows. Also, it's missing a
> period at the end of the sentence.
Sorry, what period is missing?
Comment at: lib/Sema/SemaExpr.cpp:12325-12328
+ (Self.getSourceManager().isMacroArgExpansion(OpLoc,
+ &OpExpansionLoc) &&
+ Self.getSourceManager().isMacroArgExpansion(RHSExpr->getExprLoc(),
+ &LHSExpansionLoc))) {
dexonsmith wrote:
> It's a little awkward to add this condition in with the previous one, and
> you're also repeating a call to `isMacroArgExpansion` unnecessarily. I
> suggest something like:
>
> ```
> SourceLocation OpExpansion;
> if (!SM.isMacroArgExpansion(OpLoc, &OpExpansion))
> return;
>
> SourceLocation ExprExpansion;
> if (SM.isMacroArgExpansion(LHSExpr->getExprLoc(), &ExprExpansion) &&
> OpExpansion == ExprExpansion)
> DiagnoseLogicalAndInLogicalOrLHS(Self, OpLoc, LHSExpr, RHSExpr);
>
> if (SM.isMacroArgExpansion(RHSExpr->getExprLoc(), &ExprExpansion) &&
> OpExpansion == ExprExpansion)
> DiagnoseLogicalAndInLogicalOrRHS(Self, OpLoc, LHSExpr, RHSExpr);
> ```
>
Great! The code is more elegant!
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
