- User Since
- Mar 9 2016, 4:07 AM (368 w, 3 d)
May 4 2021
Nov 19 2020
Nov 6 2020
Oct 29 2020
Oct 21 2020
Oct 20 2020
Tests added, code reformatted.
Oct 15 2020
Oct 14 2020
Oct 6 2020
Oct 5 2020
Sep 30 2020
@vabridgers Please try to apply D69726 and check whether it solves this crash!
Sep 29 2020
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.
Sep 28 2020
Sep 27 2020
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.
Sep 25 2020
@gamesh411, @whisperity, @martong and others, please suggest me new test cases if you think the current ones are not enough.
Sep 24 2020
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:
Sep 22 2020
Sep 21 2020
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.
Sep 20 2020
Sep 18 2020
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.
Sep 16 2020
Checker modernize-use-equals-delete adjusted.
std::size_t defined correctly in the tests.
Sep 15 2020
Created another matcher hasAnyBody, and updated the documentation of the matchers.
Sep 14 2020
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."
Sep 11 2020
It looks like it was not entirely my fault: D87527.
Sep 10 2020
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...
Test added. Thank you for the test @steakhal!
Fix for base constructors, test extended.
Sep 9 2020
Sep 8 2020
Sep 7 2020
Wrong diff uploaded previously. (Accidentally compared to master instead of the prerequisite.)
Sep 4 2020
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.
Sep 3 2020
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.
Sep 2 2020
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?
Aug 31 2020
Aug 14 2020
Updated according to a comment.
Tests andd for not yet handled cases. Known limiatations included in the docs.
It seems that release notes were not renamed automatically. I updated it manually now.
Aug 13 2020
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.