Higuoxing marked 3 inline comments as done.
Higuoxing added a comment.
In https://reviews.llvm.org/D47687#1126607, @dexonsmith wrote:
> In https://reviews.llvm.org/D47687#1126074, @lebedev.ri wrote:
>
> > In https://reviews.llvm.org/D47687#1126032, @Higuoxing wrote:
> >
> > > In https://reviews.l
dexonsmith added a comment.
In https://reviews.llvm.org/D47687#1126074, @lebedev.ri wrote:
> In https://reviews.llvm.org/D47687#1126032, @Higuoxing wrote:
>
> > In https://reviews.llvm.org/D47687#1125926, @rnk wrote:
> >
> > > @dexonsmith is there someone from Apple who can comment on rdar://8678
Higuoxing added a comment.
This diff version pass the both `parentheses.c` and `logical-op-parentheses.c`
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm
Higuoxing updated this revision to Diff 150517.
https://reviews.llvm.org/D47687
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Sema/SemaExpr.cpp
test/Sema/logical-op-parentheses-in-macros.c
Index: test/Sema/logical-op-parentheses-in-macros.c
Higuoxing marked 2 inline comments as done.
Higuoxing added a comment.
Sorry, I will check it and update the test case
Besides, shall I add the macro-parentheses checking test cases to the original
'parentheses.c'?
https://reviews.llvm.org/D47687
lebedev.ri added inline comments.
Comment at: test/Sema/logical-op-parentheses-in-macros.c:3-4
+// RUN:-verify=logical-op-parentheses %s
+// RUN: %clang_cc1 -fsyntax-only %s
+// RUN: %clang_cc1 -Wparentheses -fsyntax-only %s
+
The comment got lost, b
lebedev.ri added inline comments.
Comment at: include/clang/Basic/DiagnosticGroups.td:264-265
def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">;
+def LogicalOpParenthesesInMacros:
DiagGroup<"logical-op-parentheses-in-macros">;
def LogicalNotParentheses: DiagGroup<"
Higuoxing updated this revision to Diff 150507.
Higuoxing marked 4 inline comments as done.
Higuoxing added a comment.
Hope this not disturb you too much : ) thanks
https://reviews.llvm.org/D47687
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
li
lebedev.ri added inline comments.
Comment at: include/clang/Basic/DiagnosticGroups.td:264-265
def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">;
+def LogicalOpParenthesesInMacros:
DiagGroup<"logical-op-parentheses-in-macros">;
def LogicalNotParentheses: DiagGroup<"
Higuoxing updated this revision to Diff 150495.
Higuoxing added a comment.
Update without conflict with test case `parentheses.c`... I add a separate
option [-Wlogical-op-parentheses-in-macros] and will be silence by default
thanks
https://reviews.llvm.org/D47687
Files:
include/clang/Basic/
Higuoxing added a comment.
Thanks, let me have a try
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
lebedev.ri added a comment.
+ there will need to be a new standalone test for
`-Wlogical-op-parentheses-in-macros`,
what it does, how it relates to `-Wlogical-op-parentheses`,`-Wparentheses`, etc.
Comment at: include/clang/Basic/DiagnosticGroups.td:264-265
def LogicalOpParent
Higuoxing updated this revision to Diff 150465.
Higuoxing added a comment.
I step out a little further... I made an attempt to add a new warning
`[-Wlogical-op-parentheses-in-macros]`. Comparing to the previous one, which do
you prefer? I am new to llvm community, and as you see, this is my firs
Higuoxing added a comment.
In https://reviews.llvm.org/D47687#1126074, @lebedev.ri wrote:
> At worst, we can issue this warning in a new `-Wparentheses-in-macros`
> subgroup, which, if apple so insists, could be off-by-default.
> That would less worse than just completely silencing it for the e
lebedev.ri added a comment.
In https://reviews.llvm.org/D47687#1126032, @Higuoxing wrote:
> In https://reviews.llvm.org/D47687#1125926, @rnk wrote:
>
> > @dexonsmith is there someone from Apple who can comment on rdar://8678458
> > and the merits of disabling this warning in macros? I strongly s
Higuoxing added a comment.
In https://reviews.llvm.org/D47687#1125926, @rnk wrote:
> @dexonsmith is there someone from Apple who can comment on rdar://8678458 and
> the merits of disabling this warning in macros? I strongly suspect the
> original report was dealing with code like `assert(x || y
dexonsmith added a comment.
In https://reviews.llvm.org/D47687#1125926, @rnk wrote:
> @dexonsmith is there someone from Apple who can comment on rdar://8678458 and
> the merits of disabling this warning in macros? I strongly suspect the
> original report was dealing with code like `assert(x ||
rnk added subscribers: dexonsmith, chandlerc.
rnk added a comment.
@dexonsmith is there someone from Apple who can comment on rdar://8678458 and
the merits of disabling this warning in macros? I strongly suspect the original
report was dealing with code like `assert(x || y && "str");`, if so we
Higuoxing updated this revision to Diff 150253.
Higuoxing added a reviewer: echristo.
Higuoxing added a comment.
update with comments & remove some useless blank lines.
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
Higuoxing added a comment.
Thanks for reviewing! I think enabling parentheses-checking in macros could be
helpful, and `assert(something)` is widely used : )
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ht
rnk added a comment.
I'm in favor of this direction. The `var || var && truth_constant` pattern
match seems much more robust than the macro pattern match. It's also consistent
with what we do outside of macros, so it's less special and surprising.
https://reviews.llvm.org/D47687
___
Higuoxing added a comment.
Thanks for reviewing!
I think the logic that *do not emit warning in macros* is not so proper ...
Best Regards,
Xing
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.ll
dblaikie added a subscriber: Higuoxing.
dblaikie added a comment.
Probably CC someone from apple here & ask about rdar://8678458 - they can
look it up & provide the missing context.
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-co
Probably CC someone from apple here & ask about rdar://8678458 - they can
look it up & provide the missing context.
On Mon, Jun 4, 2018 at 8:17 AM Roman Lebedev via Phabricator <
revi...@reviews.llvm.org> wrote:
> lebedev.ri added reviewers: rjmccall, akyrtzi.
> lebedev.ri added a comment.
>
> It
lebedev.ri added reviewers: rjmccall, akyrtzi.
lebedev.ri added a comment.
It seems it was https://reviews.llvm.org/rL119537 that regressed it.
Nice, such a change, no review, no test :)
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
c
lebedev.ri added subscribers: dblaikie, rnk.
lebedev.ri added a comment.
In https://reviews.llvm.org/D47687#1120971, @Higuoxing wrote:
> Update with test cases :)
Thanks.
I do believe we are being overly forgiving for macros.
Sometimes that makes sense, sometimes not, especially there is no pr
Higuoxing updated this revision to Diff 149766.
Higuoxing edited the summary of this revision.
Higuoxing added a comment.
Update with test cases :)
https://reviews.llvm.org/D47687
Files:
lib/Sema/SemaExpr.cpp
test/Sema/parentheses.c
Index: test/Sema/parentheses.c
=
Higuoxing added a comment.
In https://reviews.llvm.org/D47687#1120226, @lebedev.ri wrote:
> In https://reviews.llvm.org/D47687#1120213, @Higuoxing wrote:
>
> > As for some test cases,
>
>
> The tests need to be within the patch itself, in this case
> i guess that should go into `clang/test/Sema/
lebedev.ri added a comment.
In https://reviews.llvm.org/D47687#1120213, @Higuoxing wrote:
> As for some test cases,
The tests need to be within the patch itself, in this case
i guess that should go into `clang/test/Sema/`.
See other tests in there on how to write them.
And they will be run via
lebedev.ri added reviewers: rsmith, aaron.ballman.
lebedev.ri added a comment.
Tests?
Repository:
rC Clang
https://reviews.llvm.org/D47687
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-
Higuoxing added a comment.
As for some test cases,
$ cat a.cc
#include
#include
#define bar(x) \
( \
( std::cout << x ) \
)
bool x;
int val;
void foo(bool b) {
std::cout << b << std::endl;
}
int main () {
foo(x && val == 4 || (!x && v
Higuoxing created this revision.
Herald added a subscriber: cfe-commits.
Higuoxing edited the summary of this revision.
Hi,
As you see, to avoid some expression like
assert(x || y && "some messages");
clang will skip all the parentheses check if the expression is in `macro`;
and this rule is
32 matches
Mail list logo
