In D101790#2734937, @NoQ wrote:

+Adam, the original author of bugprone-redundant-branch-condition. Adam, do you have any thoughts regarding the tests regressed by this patch? Are they something we should absolutely keep warning on?

In D89380#2330076, @alexfh wrote:

Thanks for the fix! However, I'm not sure it's possible to correctly rewrite code in all cases where macros are involved. See a couple of motivating examples in the comment.

Tests added, code reformatted.

In D88831#2311802, @lebedev.ri wrote:

In D88831#2311800, @Eugene.Zelenko wrote:

Does this check make sense in content of other style guides?

I'd +1 to moving it to readability.

In D86743#2303922, @NoQ wrote:

One slightly redeeming thing about this crash is that it's assertion-only. When built without assertions clang doesn't crash and this patch doesn't really change its behavior (adding transition to a null state is equivalent to adding no transitions at all). This means that the assertion did its job and notified us of the serious issue but simply removing the assertion doesn't bring *that* much benefit and we can probably afford to wait for a more solid fix.

@vabridgers Please try to apply D69726 and check whether it solves this crash!

In D86743#2303910, @NoQ wrote:

The last comment for that bug is D69726, but the bug is not closed to it seems to me that D69726 does not solve it, just takes a single step towards the solution.

It might as well be that the patch isn't, well, committed.

Yes, @NoQ is right. If the constraint manager cannot reason about a value, then then ProgramState::assume() will return the same state with the new assumption in the constraints. Whenever ProgramState::assume() returns nullptr it means that the assumption is impossible. Thus in this case DynSize and *ArraySizeNL cannot be equal. To me this really seems to be a similar issue to Bug 28450. The last comment for that bug is D69726, but the bug is not closed to it seems to me that D69726 does not solve it, just takes a single step towards the solution.

In D88332#2297374, @NoQ wrote:

Also what's your excuse for using visitors instead of ASTMatchers this time? :) I think even a simple matcher callExpr().bind("result") that delegates all the work to the imperative match-callback may make the code a lot more concise (by the virtue of ditching the visitor). Argument type checks may look pretty nice in matcher form as well.

Now I completely know what the source of our misunderstanding is. You thought that this patch will fix an issue, namely that we store iterator positions for both the iterator values and the locations of the iterator variables. While this is definitely a wrong approach, it could not be fixed until we got rid from the hack with LazyCompoundVals. These functions keep this existing issue unfixed. However, I could not understand you because I was completely focusing on how these functions could introduce a new issue and I could not find it. No wonder. Thus all my efforts were focused on finding a test cases which passed in the earlier versions but fail in this one because of these functions. Of course I could not find any and I was continuously proving that my patch is correct in the sense that it does not make things worse than they were. That is why I implemented the pointer-based iterators (it would have been better after this patch and a subsequent one that fixes this issue) where I was facing problems because of this wrong approach. In the same time you continuously came with new examples which tried to prove the issue, but I could not understand it because all these new test cases failed in the master version as well. We talked about two very different things because we had very different perceptions about the goal of this patch. That was all.

In D87146#2294514, @baloghadamsoftware wrote:

In D87146#2294423, @ASDenysPetrov wrote:

It would be nice if someone had time to look at this. Thanks.

I am just looking, but I am not a pthread expert.

In D87146#2294423, @ASDenysPetrov wrote:

It would be nice if someone had time to look at this. Thanks.

@gamesh411, @whisperity, @martong and others, please suggest me new test cases if you think the current ones are not enough.

Oh, I think now what do you mean: iterators stored in containers, thus iterators iterating over iterators. Yes, the current working of the checker does not support it because it stores iterator positions for both the prvalues and the glvalues. This is so from the beginning and this patch does not change anything about this behavior. Of course, this design concept is questionable, we can change it in the future. But not in this patch, this one is purely an NFC: it has exactly the same functionality as the previous versions, the only difference is that it does not hamper anymore with LazyCompoundVals but reaches the real region of the objects (and only in case of objects by value, I check the AST type for paramters), exactly as you requested.

Test coverage was hugely increased by rebasing this patch to D88216. Two kind of tests failed. This last update addreses them:

New test cases added.

Prerequeisite patch is committed, the check is tested now on the LLVM Project. @lebedev.ri, @aaron.ballman can I recommit it?

Updated according to the comments.

Tests updated: some execution paths merged by refactoring assertions.

In D81272#2276342, @sylvestre.ledru wrote:

Thanks for this checker. FYI, it found (at least) 3 defects in Firefox code (fixed with the autofix):
https://hg.mozilla.org/mozilla-central/rev/651e68f628d0

https://bugzilla.mozilla.org/show_bug.cgi?id=1664747
(unlikely that it was causing any actual bugs in the product)

Crash fixed and new tests added.

We found use cases which can be solved using this improvement. That is why I commandeer this patch now.

Checker modernize-use-equals-delete adjusted.

std::size_t defined correctly in the tests.

In D87683#2274569, @njames93 wrote:

Please fix the test case first, can't call operator new(unsigned long, void*) with an argument of type void*
The other failures the pre merge bot detected can safely be disregarded

Created another matcher hasAnyBody, and updated the documentation of the matchers.

No crash, no hang, no false positives on the LLVM Project.

Thank you for clarifying. I might nitpick that it's not "obviously wrong": I'm actually writing code now that depends on exactly that behavior -- I don't care where the method decl body is, as long as it's visible in the current TU. That said, I think you're right in wanting FunctionDecl's behavior to align with that of its derived classes and I think it better to behave that way than how it currently behaves. But, we should have an alternative available in case this change breaks anyone else depending on the current behavior. Is there a different matcher with the semantics I've described?

Also, please clarify the comments in the header for this matcher. The current description is quite unclear for functions: "Matches a 'for', 'while', 'do while' statement or a function
definition that has a given body."

In D87527#2270939, @ymandel wrote:

Can you expand on what is wrong currently for FunctionDecl descendants? Would the new test FindsBodyOfFunctionChildren fail with the current implementation?

In D87527#2268348, @whisperity wrote:

What about http://clang.llvm.org/doxygen/classclang_1_1CXXConversionDecl.html ?

It looks like it was not entirely my fault: D87527.

OK, I reversed the matcher expression now, it seems to work, but I will check it on the LLVM/Clang codebase first.

I found the problem: hasBody() always returns true if the function has somewhere a body. This means that also the hasBody matcher matches forward declarations. However, I only need the definition. This far I could not find any method to achieve that: isDefinition() also returns true for forward declarations, as well as comparing with the CanonicalDecl. I am looking further...

In D85424#2258886, @steakhal wrote:

void foo(int x) {

int uninit;
x - uninit; // will-crash

}

Test added. Thank you for the test @steakhal!

In D71199#2265594, @lebedev.ri wrote:

So i've just reverted this in rGebf496d805521b53022a351f35854de977fee844.

@aaron.ballman @baloghadamsoftware how's QC going on nowadays here?
Was this evaluated on anything other than it's tests?

Fix for base constructors, test extended.

Fixes.

Wrong diff uploaded previously. (Accidentally compared to master instead of the prerequisite.)

isIterator() updated because it did not work perfectly with the refactored handleBegin() after rebase. (Why did it work before the rebase?) The problem was that it only looked for the necessary operators in the actual class but not in its bases.

Tests separated.

I was indeed on vacation, so thanks for committing it, @NoQ! I was waiting for agreement for the prerequisite patch then I forgot to notify you that I was going on vacation.

I agree here with @Szelethus. We should investigate first why the assumption fails. Then we can decide about the best possible fix.

@NoQ could you please take a look on this short fix?

In D85351#2247095, @Szelethus wrote:

I think its a bad experience if you break something while developing. Instead of getting a test failure for "delegating constructor initializers", you'll have to deal with a test that handles a variety of things at once, and are forced to tease it apart to find what just broke. When the introduced assert fires, this wouldn't be an issue, but in any non-crashing case it might be.

In D85351#2215547, @Szelethus wrote:

Shouldn't we create a new test care for this, instead of expanding an existing one? Btw, this looks great, but I lack the confidence to accept.

Updated according to a comment.

Tests andd for not yet handled cases. Known limiatations included in the docs.

In D81272#2218050, @aaron.ballman wrote:

Thanks to the new info, I think the check basically LGTM. Can you add some negative tests and documentation wording to make it clear that the check doesn't currently handle all logically equivalent predicates, like:

if (foo) {
} else {
  if (!foo) {
  }
}

// or
if (foo > 5) {
  if (foo > 3) {
  }
}

// or
if (foo > 5) {
  if (5 < foo) {
  }
}

(I'm assuming these cases aren't handled currently and that handling them isn't necessary to land the patch.)

It seems that release notes were not renamed automatically. I updated it manually now.

Check renamed.

Since no better idea cam to my mind, in this version I check whether modernize-use-default-member-init is enabled. If it is, then we issue a warning and suggest a fix that uses default member initializer. We also take into account the option for that checker whether we should use brackets or assignment. This checker has no options now. If the other checker is not enabled, we always warn and suggest fix to use constructor member initializer.