Re: [PATCH] D24010: [ReachableCode] Skip over ExprWithCleanups in isConfigurationValue

[David Blaikie via cfe-commits]

Ah, right - thanks for reminding/explaining!

On Mon, Oct 24, 2016 at 2:42 PM Tim Shen  wrote:

> On Mon, Oct 24, 2016 at 2:38 PM David Blaikie  wrote:
>
> Simplify it further by replacing A() with just a function instead of a
> class? Or does that break the repro?
>
> bool Foo();
> void Bar();
> void Baz() {
>   if (False && Foo())
> Bar();
> }
>
>
> This actually breaks the repro. My test introduces a non-trivially
> destructible temporary object in the condition expression, which creates a
> ExprWithCleanups node at the full expression entry.
>
>
>
> On Mon, Oct 24, 2016 at 1:38 PM Tim Shen  wrote:
>
> On Mon, Oct 24, 2016 at 10:33 AM David Blaikie  wrote:
>
> On Mon, Aug 29, 2016 at 3:45 PM Tim Shen via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
> timshen created this revision.
> timshen added reviewers: rsmith, pirama.
> timshen added a subscriber: cfe-commits.
>
> https://reviews.llvm.org/D24010
>
> Files:
>   clang/include/clang/AST/Stmt.h
>   clang/lib/Analysis/ReachableCode.cpp
>   clang/test/SemaCXX/PR29152.cpp
>
> Index: clang/test/SemaCXX/PR29152.cpp
> ===
> --- /dev/null
> +++ clang/test/SemaCXX/PR29152.cpp
> @@ -0,0 +1,19 @@
> +// RUN: %clang_cc1 -fsyntax-only -Wunreachable-code -verify %s
> +
> +static const bool False = false;
> +
> +struct Vector {
> +  struct iterator {
> +bool operator==(const iterator &) const;
> +  };
> +  iterator end();
> +};
> +
> +void Bar();
> +Vector::iterator Find(Vector &a);
> +
> +void Foo(Vector &a) {
> +  if (False && Find(a) == a.end()) {
> +Bar(); // expected-no-diagnostics
> +  }
> +}
>
>
> What are the relevant parts of this test for this change?
>
> Is it the static const bool False that's interesting?
>
> Is it the op==/other interesting side effects on the RHS that are
> interesting?
>
> I'd be surprised if it were both (& if it isn't both, I'm guessing it's
> the former rather than the latter - in which case the latter can probably
> be reduced to just an arbitrary function call to, say: bool Baz())
>
>
> It's the static const bool False. I actually managed to simplify the code
> to remove the operator==() call.
>
>
>
>
> Index: clang/lib/Analysis/ReachableCode.cpp
> ===
> --- clang/lib/Analysis/ReachableCode.cpp
> +++ clang/lib/Analysis/ReachableCode.cpp
> @@ -164,6 +164,8 @@
>if (!S)
>  return false;
>
> +  S = S->IgnoreImplicit();
> +
>if (const Expr *Ex = dyn_cast(S))
>  S = Ex->IgnoreCasts();
>
> Index: clang/include/clang/AST/Stmt.h
> ===
> --- clang/include/clang/AST/Stmt.h
> +++ clang/include/clang/AST/Stmt.h
> @@ -387,6 +387,9 @@
>/// Skip past any implicit AST nodes which might surround this
>/// statement, such as ExprWithCleanups or ImplicitCastExpr nodes.
>Stmt *IgnoreImplicit();
> +  const Stmt *IgnoreImplicit() const {
> +return const_cast(this)->IgnoreImplicit();
> +  }
>
>/// \brief Skip no-op (attributed, compound) container stmts and skip
> captured
>/// stmt at the top, if \a IgnoreCaptured is true.
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24010: [ReachableCode] Skip over ExprWithCleanups in isConfigurationValue

[Tim Shen via cfe-commits]

On Mon, Oct 24, 2016 at 2:38 PM David Blaikie  wrote:

> Simplify it further by replacing A() with just a function instead of a
> class? Or does that break the repro?
>
> bool Foo();
> void Bar();
> void Baz() {
>   if (False && Foo())
> Bar();
> }
>

This actually breaks the repro. My test introduces a non-trivially
destructible temporary object in the condition expression, which creates a
ExprWithCleanups node at the full expression entry.


>
> On Mon, Oct 24, 2016 at 1:38 PM Tim Shen  wrote:
>
> On Mon, Oct 24, 2016 at 10:33 AM David Blaikie  wrote:
>
> On Mon, Aug 29, 2016 at 3:45 PM Tim Shen via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
> timshen created this revision.
> timshen added reviewers: rsmith, pirama.
> timshen added a subscriber: cfe-commits.
>
> https://reviews.llvm.org/D24010
>
> Files:
>   clang/include/clang/AST/Stmt.h
>   clang/lib/Analysis/ReachableCode.cpp
>   clang/test/SemaCXX/PR29152.cpp
>
> Index: clang/test/SemaCXX/PR29152.cpp
> ===
> --- /dev/null
> +++ clang/test/SemaCXX/PR29152.cpp
> @@ -0,0 +1,19 @@
> +// RUN: %clang_cc1 -fsyntax-only -Wunreachable-code -verify %s
> +
> +static const bool False = false;
> +
> +struct Vector {
> +  struct iterator {
> +bool operator==(const iterator &) const;
> +  };
> +  iterator end();
> +};
> +
> +void Bar();
> +Vector::iterator Find(Vector &a);
> +
> +void Foo(Vector &a) {
> +  if (False && Find(a) == a.end()) {
> +Bar(); // expected-no-diagnostics
> +  }
> +}
>
>
> What are the relevant parts of this test for this change?
>
> Is it the static const bool False that's interesting?
>
> Is it the op==/other interesting side effects on the RHS that are
> interesting?
>
> I'd be surprised if it were both (& if it isn't both, I'm guessing it's
> the former rather than the latter - in which case the latter can probably
> be reduced to just an arbitrary function call to, say: bool Baz())
>
>
> It's the static const bool False. I actually managed to simplify the code
> to remove the operator==() call.
>
>
>
>
> Index: clang/lib/Analysis/ReachableCode.cpp
> ===
> --- clang/lib/Analysis/ReachableCode.cpp
> +++ clang/lib/Analysis/ReachableCode.cpp
> @@ -164,6 +164,8 @@
>if (!S)
>  return false;
>
> +  S = S->IgnoreImplicit();
> +
>if (const Expr *Ex = dyn_cast(S))
>  S = Ex->IgnoreCasts();
>
> Index: clang/include/clang/AST/Stmt.h
> ===
> --- clang/include/clang/AST/Stmt.h
> +++ clang/include/clang/AST/Stmt.h
> @@ -387,6 +387,9 @@
>/// Skip past any implicit AST nodes which might surround this
>/// statement, such as ExprWithCleanups or ImplicitCastExpr nodes.
>Stmt *IgnoreImplicit();
> +  const Stmt *IgnoreImplicit() const {
> +return const_cast(this)->IgnoreImplicit();
> +  }
>
>/// \brief Skip no-op (attributed, compound) container stmts and skip
> captured
>/// stmt at the top, if \a IgnoreCaptured is true.
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24010: [ReachableCode] Skip over ExprWithCleanups in isConfigurationValue

[David Blaikie via cfe-commits]

Simplify it further by replacing A() with just a function instead of a
class? Or does that break the repro?

bool Foo();
void Bar();
void Baz() {
  if (False && Foo())
Bar();
}

On Mon, Oct 24, 2016 at 1:38 PM Tim Shen  wrote:

> On Mon, Oct 24, 2016 at 10:33 AM David Blaikie  wrote:
>
> On Mon, Aug 29, 2016 at 3:45 PM Tim Shen via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
> timshen created this revision.
> timshen added reviewers: rsmith, pirama.
> timshen added a subscriber: cfe-commits.
>
> https://reviews.llvm.org/D24010
>
> Files:
>   clang/include/clang/AST/Stmt.h
>   clang/lib/Analysis/ReachableCode.cpp
>   clang/test/SemaCXX/PR29152.cpp
>
> Index: clang/test/SemaCXX/PR29152.cpp
> ===
> --- /dev/null
> +++ clang/test/SemaCXX/PR29152.cpp
> @@ -0,0 +1,19 @@
> +// RUN: %clang_cc1 -fsyntax-only -Wunreachable-code -verify %s
> +
> +static const bool False = false;
> +
> +struct Vector {
> +  struct iterator {
> +bool operator==(const iterator &) const;
> +  };
> +  iterator end();
> +};
> +
> +void Bar();
> +Vector::iterator Find(Vector &a);
> +
> +void Foo(Vector &a) {
> +  if (False && Find(a) == a.end()) {
> +Bar(); // expected-no-diagnostics
> +  }
> +}
>
>
> What are the relevant parts of this test for this change?
>
> Is it the static const bool False that's interesting?
>
> Is it the op==/other interesting side effects on the RHS that are
> interesting?
>
> I'd be surprised if it were both (& if it isn't both, I'm guessing it's
> the former rather than the latter - in which case the latter can probably
> be reduced to just an arbitrary function call to, say: bool Baz())
>
>
> It's the static const bool False. I actually managed to simplify the code
> to remove the operator==() call.
>
>
>
>
> Index: clang/lib/Analysis/ReachableCode.cpp
> ===
> --- clang/lib/Analysis/ReachableCode.cpp
> +++ clang/lib/Analysis/ReachableCode.cpp
> @@ -164,6 +164,8 @@
>if (!S)
>  return false;
>
> +  S = S->IgnoreImplicit();
> +
>if (const Expr *Ex = dyn_cast(S))
>  S = Ex->IgnoreCasts();
>
> Index: clang/include/clang/AST/Stmt.h
> ===
> --- clang/include/clang/AST/Stmt.h
> +++ clang/include/clang/AST/Stmt.h
> @@ -387,6 +387,9 @@
>/// Skip past any implicit AST nodes which might surround this
>/// statement, such as ExprWithCleanups or ImplicitCastExpr nodes.
>Stmt *IgnoreImplicit();
> +  const Stmt *IgnoreImplicit() const {
> +return const_cast(this)->IgnoreImplicit();
> +  }
>
>/// \brief Skip no-op (attributed, compound) container stmts and skip
> captured
>/// stmt at the top, if \a IgnoreCaptured is true.
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24010: [ReachableCode] Skip over ExprWithCleanups in isConfigurationValue

[Tim Shen via cfe-commits]

On Mon, Oct 24, 2016 at 10:33 AM David Blaikie  wrote:

> On Mon, Aug 29, 2016 at 3:45 PM Tim Shen via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
> timshen created this revision.
> timshen added reviewers: rsmith, pirama.
> timshen added a subscriber: cfe-commits.
>
> https://reviews.llvm.org/D24010
>
> Files:
>   clang/include/clang/AST/Stmt.h
>   clang/lib/Analysis/ReachableCode.cpp
>   clang/test/SemaCXX/PR29152.cpp
>
> Index: clang/test/SemaCXX/PR29152.cpp
> ===
> --- /dev/null
> +++ clang/test/SemaCXX/PR29152.cpp
> @@ -0,0 +1,19 @@
> +// RUN: %clang_cc1 -fsyntax-only -Wunreachable-code -verify %s
> +
> +static const bool False = false;
> +
> +struct Vector {
> +  struct iterator {
> +bool operator==(const iterator &) const;
> +  };
> +  iterator end();
> +};
> +
> +void Bar();
> +Vector::iterator Find(Vector &a);
> +
> +void Foo(Vector &a) {
> +  if (False && Find(a) == a.end()) {
> +Bar(); // expected-no-diagnostics
> +  }
> +}
>
>
> What are the relevant parts of this test for this change?
>
> Is it the static const bool False that's interesting?
>
> Is it the op==/other interesting side effects on the RHS that are
> interesting?
>
> I'd be surprised if it were both (& if it isn't both, I'm guessing it's
> the former rather than the latter - in which case the latter can probably
> be reduced to just an arbitrary function call to, say: bool Baz())
>

It's the static const bool False. I actually managed to simplify the code
to remove the operator==() call.


>
>
> Index: clang/lib/Analysis/ReachableCode.cpp
> ===
> --- clang/lib/Analysis/ReachableCode.cpp
> +++ clang/lib/Analysis/ReachableCode.cpp
> @@ -164,6 +164,8 @@
>if (!S)
>  return false;
>
> +  S = S->IgnoreImplicit();
> +
>if (const Expr *Ex = dyn_cast(S))
>  S = Ex->IgnoreCasts();
>
> Index: clang/include/clang/AST/Stmt.h
> ===
> --- clang/include/clang/AST/Stmt.h
> +++ clang/include/clang/AST/Stmt.h
> @@ -387,6 +387,9 @@
>/// Skip past any implicit AST nodes which might surround this
>/// statement, such as ExprWithCleanups or ImplicitCastExpr nodes.
>Stmt *IgnoreImplicit();
> +  const Stmt *IgnoreImplicit() const {
> +return const_cast(this)->IgnoreImplicit();
> +  }
>
>/// \brief Skip no-op (attributed, compound) container stmts and skip
> captured
>/// stmt at the top, if \a IgnoreCaptured is true.
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24010: [ReachableCode] Skip over ExprWithCleanups in isConfigurationValue

[David Blaikie via cfe-commits]

On Mon, Aug 29, 2016 at 3:45 PM Tim Shen via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> timshen created this revision.
> timshen added reviewers: rsmith, pirama.
> timshen added a subscriber: cfe-commits.
>
> https://reviews.llvm.org/D24010
>
> Files:
>   clang/include/clang/AST/Stmt.h
>   clang/lib/Analysis/ReachableCode.cpp
>   clang/test/SemaCXX/PR29152.cpp
>
> Index: clang/test/SemaCXX/PR29152.cpp
> ===
> --- /dev/null
> +++ clang/test/SemaCXX/PR29152.cpp
> @@ -0,0 +1,19 @@
> +// RUN: %clang_cc1 -fsyntax-only -Wunreachable-code -verify %s
> +
> +static const bool False = false;
> +
> +struct Vector {
> +  struct iterator {
> +bool operator==(const iterator &) const;
> +  };
> +  iterator end();
> +};
> +
> +void Bar();
> +Vector::iterator Find(Vector &a);
> +
> +void Foo(Vector &a) {
> +  if (False && Find(a) == a.end()) {
> +Bar(); // expected-no-diagnostics
> +  }
> +}
>

What are the relevant parts of this test for this change?

Is it the static const bool False that's interesting?

Is it the op==/other interesting side effects on the RHS that are
interesting?

I'd be surprised if it were both (& if it isn't both, I'm guessing it's the
former rather than the latter - in which case the latter can probably be
reduced to just an arbitrary function call to, say: bool Baz())


> Index: clang/lib/Analysis/ReachableCode.cpp
> ===
> --- clang/lib/Analysis/ReachableCode.cpp
> +++ clang/lib/Analysis/ReachableCode.cpp
> @@ -164,6 +164,8 @@
>if (!S)
>  return false;
>
> +  S = S->IgnoreImplicit();
> +
>if (const Expr *Ex = dyn_cast(S))
>  S = Ex->IgnoreCasts();
>
> Index: clang/include/clang/AST/Stmt.h
> ===
> --- clang/include/clang/AST/Stmt.h
> +++ clang/include/clang/AST/Stmt.h
> @@ -387,6 +387,9 @@
>/// Skip past any implicit AST nodes which might surround this
>/// statement, such as ExprWithCleanups or ImplicitCastExpr nodes.
>Stmt *IgnoreImplicit();
> +  const Stmt *IgnoreImplicit() const {
> +return const_cast(this)->IgnoreImplicit();
> +  }
>
>/// \brief Skip no-op (attributed, compound) container stmts and skip
> captured
>/// stmt at the top, if \a IgnoreCaptured is true.
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24010: [ReachableCode] Skip over ExprWithCleanups in isConfigurationValue

[Pirama Arumuga Nainar via cfe-commits]

pirama added a comment.

Ping... I am not the author of this patch, but am interested in seeing the 
issue fixed :)


https://reviews.llvm.org/D24010



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24010: [ReachableCode] Skip over ExprWithCleanups in isConfigurationValue

[Pirama Arumuga Nainar via cfe-commits]

pirama added a subscriber: srhines.
pirama added a comment.

Tim: Thanks for looking into this.  It's definitely a simpler fix than I 
thought it would be :)


https://reviews.llvm.org/D24010



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits