Re: [clang] e4b3fc1 - Get rid of -Wunused warnings in release build, NFC.

2020-06-16 Thread David Blaikie via cfe-commits 
On Tue, Jun 16, 2020 at 8:17 AM Kristóf Umann  wrote:
>
> Apologies for the inconvenience, though I wonder why I haven't gotten 
> builtbot mails :/
>
> On Tue, 16 Jun 2020 at 09:54, Haojian Wu  wrote:
>>
>> On Mon, 15 Jun 2020 at 18:29, David Blaikie  wrote:
>>>
>>> I don't think the comment's adding value here - it should be fairly
>>> clear from the context that the whole loop only exists for some
>>> assertions.
>>>
>>> Also: Could you remove the "(void)" casts now that the whole thing's
>>> wrapped in NDEBUG?
>>
>>
>> oops, I think this is coincident, there were two patches at the same time to 
>> fix the issue.
>> Cleaned it up in  e00dcf61a2f4397c0525db7133503b80d8ecf5ac.
>>
>>>
>>> (an alternative way of phrasing this that doesn't require the #ifdef
>>> would be using nested llvm::all_of calls, but that would mean the
>>> assertion firing wouldn't point to a particular mismatched pair, just
>>> that overall the data structures were mismatched - if you think that
>>> this isn't especially likely/wouldn't be super hard to debug with
>>> that, maybe the assert would be more compact/tidier?
>>>
>>> assert(llvm::all_of(Dependencies, [&](const auto ) {
>>>   return llvm::all_of(WeakDependencies, [&](const auto ) {
>>> return WeakDepPair != DepPair && WeakDepPair.first !=
>>> DepPair.second && WeakDepPair.second != DepPair.second;
>>>   }
>>> }) && "...");
>>
>>
>> I'm not the author of this code, defer the decision to the author Kirstóf, 
>> dkszelet...@gmail.com.
>>
>
>
> That is a great suggestion, but I find the individual asserts (with a 
> message!) a better solution to guide the developer should an error be made in 
> the TableGen file.

Yep, totally fair!

>
>>>
>>> On Fri, Jun 12, 2020 at 6:45 AM Haojian Wu via cfe-commits
>>>  wrote:
>>> >
>>> >
>>> > Author: Haojian Wu
>>> > Date: 2020-06-12T15:42:29+02:00
>>> > New Revision: e4b3fc18d33199e2081d300f14687d81be48b6a0
>>> >
>>> > URL: 
>>> > https://github.com/llvm/llvm-project/commit/e4b3fc18d33199e2081d300f14687d81be48b6a0
>>> > DIFF: 
>>> > https://github.com/llvm/llvm-project/commit/e4b3fc18d33199e2081d300f14687d81be48b6a0.diff
>>> >
>>> > LOG: Get rid of -Wunused warnings in release build, NFC.
>>> >
>>> > Added:
>>> >
>>> >
>>> > Modified:
>>> > clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
>>> >
>>> > Removed:
>>> >
>>> >
>>> >
>>> > 
>>> > diff  --git a/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp 
>>> > b/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
>>> > index c2ca9c12b025..4a7e0d91ea23 100644
>>> > --- a/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
>>> > +++ b/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
>>> > @@ -285,6 +285,7 @@ CheckerRegistry::CheckerRegistry(
>>> >resolveDependencies();
>>> >resolveDependencies();
>>> >
>>> > +#ifndef NDEBUG // avoid -Wunused warnings in release build.
>>> >for (auto  : Dependencies) {
>>> >  for (auto  : WeakDependencies) {
>>> >// Some assertions to enforce that strong dependencies are 
>>> > relations in
>>> > @@ -298,6 +299,7 @@ CheckerRegistry::CheckerRegistry(
>>> >   "A strong dependency mustn't be a weak dependency as 
>>> > well!");
>>> >  }
>>> >}
>>> > +#endif
>>> >
>>> >resolveCheckerAndPackageOptions();
>>> >
>>> >
>>> >
>>> >
>>> > ___
>>> > cfe-commits mailing list
>>> > cfe-commits@lists.llvm.org
>>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang] e4b3fc1 - Get rid of -Wunused warnings in release build, NFC.

2020-06-16 Thread Kristóf Umann via cfe-commits 
Apologies for the inconvenience, though I wonder why I haven't gotten
builtbot mails :/

On Tue, 16 Jun 2020 at 09:54, Haojian Wu  wrote:

> On Mon, 15 Jun 2020 at 18:29, David Blaikie  wrote:
>
>> I don't think the comment's adding value here - it should be fairly
>> clear from the context that the whole loop only exists for some
>> assertions.
>>
>> Also: Could you remove the "(void)" casts now that the whole thing's
>> wrapped in NDEBUG?
>>
>
> oops, I think this is coincident, there were two patches at the same time
> to fix the issue.
> Cleaned it up in  e00dcf61a2f4397c0525db7133503b80d8ecf5ac.
>
>
>> (an alternative way of phrasing this that doesn't require the #ifdef
>> would be using nested llvm::all_of calls, but that would mean the
>> assertion firing wouldn't point to a particular mismatched pair, just
>> that overall the data structures were mismatched - if you think that
>> this isn't especially likely/wouldn't be super hard to debug with
>> that, maybe the assert would be more compact/tidier?
>>
>> assert(llvm::all_of(Dependencies, [&](const auto ) {
>>   return llvm::all_of(WeakDependencies, [&](const auto ) {
>> return WeakDepPair != DepPair && WeakDepPair.first !=
>> DepPair.second && WeakDepPair.second != DepPair.second;
>>   }
>> }) && "...");
>>
>
> I'm not the author of this code, defer the decision to the author Kirstóf,
> dkszelet...@gmail.com.
>
>

That is a great suggestion, but I find the individual asserts (with a
message!) a better solution to guide the developer should an error be made
in the TableGen file.


> On Fri, Jun 12, 2020 at 6:45 AM Haojian Wu via cfe-commits
>>  wrote:
>> >
>> >
>> > Author: Haojian Wu
>> > Date: 2020-06-12T15:42:29+02:00
>> > New Revision: e4b3fc18d33199e2081d300f14687d81be48b6a0
>> >
>> > URL:
>> https://github.com/llvm/llvm-project/commit/e4b3fc18d33199e2081d300f14687d81be48b6a0
>> > DIFF:
>> https://github.com/llvm/llvm-project/commit/e4b3fc18d33199e2081d300f14687d81be48b6a0.diff
>> >
>> > LOG: Get rid of -Wunused warnings in release build, NFC.
>> >
>> > Added:
>> >
>> >
>> > Modified:
>> > clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
>> >
>> > Removed:
>> >
>> >
>> >
>> >
>> 
>> > diff  --git a/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
>> b/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
>> > index c2ca9c12b025..4a7e0d91ea23 100644
>> > --- a/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
>> > +++ b/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
>> > @@ -285,6 +285,7 @@ CheckerRegistry::CheckerRegistry(
>> >resolveDependencies();
>> >resolveDependencies();
>> >
>> > +#ifndef NDEBUG // avoid -Wunused warnings in release build.
>> >for (auto  : Dependencies) {
>> >  for (auto  : WeakDependencies) {
>> >// Some assertions to enforce that strong dependencies are
>> relations in
>> > @@ -298,6 +299,7 @@ CheckerRegistry::CheckerRegistry(
>> >   "A strong dependency mustn't be a weak dependency as
>> well!");
>> >  }
>> >}
>> > +#endif
>> >
>> >resolveCheckerAndPackageOptions();
>> >
>> >
>> >
>> >
>> > ___
>> > cfe-commits mailing list
>> > cfe-commits@lists.llvm.org
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang] e4b3fc1 - Get rid of -Wunused warnings in release build, NFC.

2020-06-16 Thread Haojian Wu via cfe-commits 
On Mon, 15 Jun 2020 at 18:29, David Blaikie  wrote:

> I don't think the comment's adding value here - it should be fairly
> clear from the context that the whole loop only exists for some
> assertions.
>
> Also: Could you remove the "(void)" casts now that the whole thing's
> wrapped in NDEBUG?
>

oops, I think this is coincident, there were two patches at the same time
to fix the issue.
Cleaned it up in  e00dcf61a2f4397c0525db7133503b80d8ecf5ac.


> (an alternative way of phrasing this that doesn't require the #ifdef
> would be using nested llvm::all_of calls, but that would mean the
> assertion firing wouldn't point to a particular mismatched pair, just
> that overall the data structures were mismatched - if you think that
> this isn't especially likely/wouldn't be super hard to debug with
> that, maybe the assert would be more compact/tidier?
>
> assert(llvm::all_of(Dependencies, [&](const auto ) {
>   return llvm::all_of(WeakDependencies, [&](const auto ) {
> return WeakDepPair != DepPair && WeakDepPair.first !=
> DepPair.second && WeakDepPair.second != DepPair.second;
>   }
> }) && "...");
>

I'm not the author of this code, defer the decision to the author Kirstóf,
dkszelet...@gmail.com.


> On Fri, Jun 12, 2020 at 6:45 AM Haojian Wu via cfe-commits
>  wrote:
> >
> >
> > Author: Haojian Wu
> > Date: 2020-06-12T15:42:29+02:00
> > New Revision: e4b3fc18d33199e2081d300f14687d81be48b6a0
> >
> > URL:
> https://github.com/llvm/llvm-project/commit/e4b3fc18d33199e2081d300f14687d81be48b6a0
> > DIFF:
> https://github.com/llvm/llvm-project/commit/e4b3fc18d33199e2081d300f14687d81be48b6a0.diff
> >
> > LOG: Get rid of -Wunused warnings in release build, NFC.
> >
> > Added:
> >
> >
> > Modified:
> > clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
> >
> > Removed:
> >
> >
> >
> >
> 
> > diff  --git a/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
> b/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
> > index c2ca9c12b025..4a7e0d91ea23 100644
> > --- a/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
> > +++ b/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
> > @@ -285,6 +285,7 @@ CheckerRegistry::CheckerRegistry(
> >resolveDependencies();
> >resolveDependencies();
> >
> > +#ifndef NDEBUG // avoid -Wunused warnings in release build.
> >for (auto  : Dependencies) {
> >  for (auto  : WeakDependencies) {
> >// Some assertions to enforce that strong dependencies are
> relations in
> > @@ -298,6 +299,7 @@ CheckerRegistry::CheckerRegistry(
> >   "A strong dependency mustn't be a weak dependency as
> well!");
> >  }
> >}
> > +#endif
> >
> >resolveCheckerAndPackageOptions();
> >
> >
> >
> >
> > ___
> > cfe-commits mailing list
> > cfe-commits@lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [clang] e4b3fc1 - Get rid of -Wunused warnings in release build, NFC.

2020-06-15 Thread David Blaikie via cfe-commits 
I don't think the comment's adding value here - it should be fairly
clear from the context that the whole loop only exists for some
assertions.

Also: Could you remove the "(void)" casts now that the whole thing's
wrapped in NDEBUG?

(an alternative way of phrasing this that doesn't require the #ifdef
would be using nested llvm::all_of calls, but that would mean the
assertion firing wouldn't point to a particular mismatched pair, just
that overall the data structures were mismatched - if you think that
this isn't especially likely/wouldn't be super hard to debug with
that, maybe the assert would be more compact/tidier?

assert(llvm::all_of(Dependencies, [&](const auto ) {
  return llvm::all_of(WeakDependencies, [&](const auto ) {
return WeakDepPair != DepPair && WeakDepPair.first !=
DepPair.second && WeakDepPair.second != DepPair.second;
  }
}) && "...");

On Fri, Jun 12, 2020 at 6:45 AM Haojian Wu via cfe-commits
 wrote:
>
>
> Author: Haojian Wu
> Date: 2020-06-12T15:42:29+02:00
> New Revision: e4b3fc18d33199e2081d300f14687d81be48b6a0
>
> URL: 
> https://github.com/llvm/llvm-project/commit/e4b3fc18d33199e2081d300f14687d81be48b6a0
> DIFF: 
> https://github.com/llvm/llvm-project/commit/e4b3fc18d33199e2081d300f14687d81be48b6a0.diff
>
> LOG: Get rid of -Wunused warnings in release build, NFC.
>
> Added:
>
>
> Modified:
> clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
>
> Removed:
>
>
>
> 
> diff  --git a/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp 
> b/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
> index c2ca9c12b025..4a7e0d91ea23 100644
> --- a/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
> +++ b/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
> @@ -285,6 +285,7 @@ CheckerRegistry::CheckerRegistry(
>resolveDependencies();
>resolveDependencies();
>
> +#ifndef NDEBUG // avoid -Wunused warnings in release build.
>for (auto  : Dependencies) {
>  for (auto  : WeakDependencies) {
>// Some assertions to enforce that strong dependencies are relations in
> @@ -298,6 +299,7 @@ CheckerRegistry::CheckerRegistry(
>   "A strong dependency mustn't be a weak dependency as well!");
>  }
>}
> +#endif
>
>resolveCheckerAndPackageOptions();
>
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] e4b3fc1 - Get rid of -Wunused warnings in release build, NFC.

2020-06-12 Thread Haojian Wu via cfe-commits 

Author: Haojian Wu
Date: 2020-06-12T15:42:29+02:00
New Revision: e4b3fc18d33199e2081d300f14687d81be48b6a0

URL: 
https://github.com/llvm/llvm-project/commit/e4b3fc18d33199e2081d300f14687d81be48b6a0
DIFF: 
https://github.com/llvm/llvm-project/commit/e4b3fc18d33199e2081d300f14687d81be48b6a0.diff

LOG: Get rid of -Wunused warnings in release build, NFC.

Added: 


Modified: 
clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp

Removed: 




diff  --git a/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp 
b/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
index c2ca9c12b025..4a7e0d91ea23 100644
--- a/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
+++ b/clang/lib/StaticAnalyzer/Frontend/CheckerRegistry.cpp
@@ -285,6 +285,7 @@ CheckerRegistry::CheckerRegistry(
   resolveDependencies();
   resolveDependencies();
 
+#ifndef NDEBUG // avoid -Wunused warnings in release build.
   for (auto  : Dependencies) {
 for (auto  : WeakDependencies) {
   // Some assertions to enforce that strong dependencies are relations in
@@ -298,6 +299,7 @@ CheckerRegistry::CheckerRegistry(
  "A strong dependency mustn't be a weak dependency as well!");
 }
   }
+#endif
 
   resolveCheckerAndPackageOptions();
 



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