================
Comment at: include/clang/Sema/Sema.h:3626
@@ -3624,1 +3625,3 @@
+
+
/// ActOnConditionalOp - Parse a ?: operation. Note that 'LHS' may be null
----------------
Reid Kleckner wrote:
> micro nit: extra blank line
Randomly selected one of the two lines and removed it.
================
Comment at: lib/Sema/SemaExpr.cpp:8610
@@ +8609,3 @@
+
+ // Allow assignemtn and compound assignments. Also recurse on other
+ // comma operators.
----------------
Reid Kleckner wrote:
> typo assignemtn
Unscrambled the letters.
================
Comment at: lib/Sema/SemaExpr.cpp:8615
@@ +8614,3 @@
+ case BO_Comma:
+ return IgnoreCommaOperand(BO->getRHS());
+ case BO_Assign:
----------------
Reid Kleckner wrote:
> Any reason to only look at the RHS?
`-BinaryOperator : outer
|-BinaryOperator : inner
| |-LHS
| `-RHS
`-RHS : outer
Both BinaryOperators are comma operators.
Currently, this is processing the the outer BinaryOperator and encounters the
inner BinaryOperator. We recurse down the RHS to find which expression will be
used. The LHS will be checked when processing the inner BinaryOperator.
================
Comment at: lib/Sema/SemaExpr.cpp:8683
@@ +8682,3 @@
+void Sema::DiagnoseCommaOperator(const Expr *LHS, SourceLocation Loc) {
+ // No warnings in macros
+ if (Loc.isMacroID())
----------------
Reid Kleckner wrote:
> Can you explain why a bit? I'm assuming the answer is that people use the
> comma in macros to simulate multiple statements, and they end up passing the
> result as a function argument.
It is common practice to ignore warnings in macros. One common case is macros
which need to return a value, but also perform a logging or checking operation
at the same time.
#define CHECK_AND_RETURN_VALUE(x) \
(log(x), x)
void bar(int num) {
foo(CHECK_AND_RETURN_VALUE(num));
}
================
Comment at: lib/Sema/SemaExpr.cpp:8687
@@ +8686,3 @@
+
+ // Don't warn in template instantiations
+ if (!ActiveTemplateInstantiations.empty())
----------------
Reid Kleckner wrote:
> Reid Kleckner wrote:
> > Micro nit: end the sentence with a period.
> Can you elaborate a tiny bit? I'm guessing the idea here is to diagnose once
> prior to instantiation, even if there are some things that we can't diagnose
> without instantiating:
>
> template <typename T>
> struct A {
> void foo(int x) {}
> void bar() {
> foo((T::baz(), T::qux()));
> }
> };
Period added.
================
Comment at: lib/Sema/SemaExpr.cpp:8687
@@ +8686,3 @@
+
+ // Don't warn in template instantiations
+ if (!ActiveTemplateInstantiations.empty())
----------------
Richard Trieu wrote:
> Reid Kleckner wrote:
> > Reid Kleckner wrote:
> > > Micro nit: end the sentence with a period.
> > Can you elaborate a tiny bit? I'm guessing the idea here is to diagnose
> > once prior to instantiation, even if there are some things that we can't
> > diagnose without instantiating:
> >
> > template <typename T>
> > struct A {
> > void foo(int x) {}
> > void bar() {
> > foo((T::baz(), T::qux()));
> > }
> > };
> Period added.
This prevents one warning in each template instantiation. Instead, only one
warning will be given on the template definition. For cases that are
ambiguous, the warning assumes that type-dependent comma operators are bad and
warns on them.
http://reviews.llvm.org/D3976
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
