[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

frederic-tingaud-sonarsource added a comment. Hi @NoQ . Would you have time to look if the changes I did to this PR solve your concerns? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126534/new/ https://reviews.llvm.org/D126534 ___

[PATCH] D130709: MSVC compatibility mode: fix error on unqualified templated base class initialization in case of partial specialization

frederic-tingaud-sonarsource added a comment. @rnk , does this change answer your points? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130709/new/ https://reviews.llvm.org/D130709 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D130709: MSVC compatibility mode: fix error on unqualified templated base class initialization in case of partial specialization

frederic-tingaud-sonarsource updated this revision to Diff 448549. frederic-tingaud-sonarsource added a comment. Use getAs to see through ElaboratedType as recommended by https://reviews.llvm.org/D112374 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130709/new/

[PATCH] D130709: MSVC compatibility mode: fix error on unqualified templated base class initialization in case of partial specialization

frederic-tingaud-sonarsource created this revision. frederic-tingaud-sonarsource added a reviewer: rnk. Herald added a project: All. frederic-tingaud-sonarsource requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. I introduced a patch to

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

frederic-tingaud-sonarsource marked an inline comment as done. frederic-tingaud-sonarsource added a comment. Hi @NoQ , Do the changes correspond to what you had in mind? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126534/new/ https://reviews.llvm.org/D126534

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

frederic-tingaud-sonarsource updated this revision to Diff 439722. frederic-tingaud-sonarsource added a comment. Adding an additional false negative fixed by this patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126534/new/ https://reviews.llvm.org/D126534 Files:

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

frederic-tingaud-sonarsource updated this revision to Diff 436766. frederic-tingaud-sonarsource added a comment. Update diagnostic location to highlight the whole content of the initialization. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126534/new/ https://reviews.llvm.org/D126534

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

frederic-tingaud-sonarsource added a comment. It might also be interesting to think about cases that went through transitive assignment stripping when talking about the diagnostic position: Before my patch, a dead store on variable `a` was displayed as follows: B b; A a = static_cast(b =

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

frederic-tingaud-sonarsource added a comment. Thanks @NoQ! The change in behavior is indeed what I was mentioning in the summary. After discussing it with a colleague, it seemed to us that pointing to a more precise range where the object is allocated was an improvement. I understand your

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

frederic-tingaud-sonarsource marked an inline comment as done. frederic-tingaud-sonarsource added a comment. Thanks for looking at the PR! In D126534#3542402 , @steakhal wrote: > You also mentioned this copy elision, but I don't see anywhere checks for

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

frederic-tingaud-sonarsource updated this revision to Diff 432558. frederic-tingaud-sonarsource added a comment. Document tests better CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126534/new/ https://reviews.llvm.org/D126534 Files:

[PATCH] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

frederic-tingaud-sonarsource updated this revision to Diff 432552. frederic-tingaud-sonarsource added a comment. Fix formatting CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126534/new/ https://reviews.llvm.org/D126534 Files: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp

[PATCH] D126534: Deadstore static analysis: Fix false positive on C++17 assignments

frederic-tingaud-sonarsource created this revision. frederic-tingaud-sonarsource added a reviewer: dcoughlin. Herald added a subscriber: martong. Herald added a project: All. frederic-tingaud-sonarsource requested review of this revision. Herald added a project: clang. Herald added a subscriber:

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

frederic-tingaud-sonarsource added a comment. @rnk Here is the test case that was broken: https://godbolt.org/z/Gef7PcqMf One point where I was wrong in my initial analysis is that the behavior doesn't actually change based on `/std:c++17` VS `/std:c++latest`, but based on `/permissive` VS

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

frederic-tingaud-sonarsource added a comment. I realized that this current patch did in fact mimic MSVC behavior up to its mishandling on ADL. I understand that it is not in the spirit of clang's philosophy for MSVC compatibility to have that, but the fixed version I was about to propose would

[PATCH] D125529: Revert MSVC compatibility feature that breaks conformant code

frederic-tingaud-sonarsource created this revision. frederic-tingaud-sonarsource added reviewers: rnk, thakis. Herald added a reviewer: shafik. Herald added a project: All. frederic-tingaud-sonarsource requested review of this revision. Herald added a project: clang. Herald added a subscriber:

[PATCH] D125526: Fix ADL leakage due to MSVC compatibility flag

frederic-tingaud-sonarsource abandoned this revision. frederic-tingaud-sonarsource added a comment. After checking, I realize that the previous fix was really mimicking MSVC behavior up to its mishandling of standard compliant code. https://godbolt.org/z/cEPGfMz3f I'll revert it for now and try

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

frederic-tingaud-sonarsource added a comment. Sorry for the delay, the fix is here: https://reviews.llvm.org/D125526 If there is any risk or problem identified with this new fix that could delay it, I'll propose a revert of this one to avoid breaking more legitimate code. Repository: rG LLVM

[PATCH] D125526: Fix ADL leakage due to MSVC compatibility flag

frederic-tingaud-sonarsource created this revision. frederic-tingaud-sonarsource added a reviewer: rnk. Herald added a project: All. frederic-tingaud-sonarsource requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Patch

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

frederic-tingaud-sonarsource added a comment. Regarding the "why": Our tools (SonarQube / SonarCloud / SonarLint) use the clang front-end to parse our customer's code and find bugs/bad patterns in it. Said customer code can target various compilers including MSVC and we need to handle it as

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

frederic-tingaud-sonarsource added a comment. In D124613#3506740 , @thakis wrote: > Should we revert this, at least for now, until the investigation is complete? I started typing my message before you sent yours so it was not a direct answer. We are

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

frederic-tingaud-sonarsource added a comment. Thanks a lot for pointing out the reproducer, it made the debug a lot easier. I have pinpointed the problem to an unintended increase in ADL visibility for friend classes. I do have a test-case and fix for that. I'll make a last check tomorrow to

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

frederic-tingaud-sonarsource added a comment. That is definitely not expected. I'm looking into it, but as this is my first time building chromium, I'm taking a bit more time finding my way around. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator

frederic-tingaud-sonarsource updated this revision to Diff 428044. frederic-tingaud-sonarsource added a comment. Fix formatting CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124845/new/ https://reviews.llvm.org/D124845 Files: clang/include/clang/Analysis/ConstructionContext.h

[PATCH] D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator

frederic-tingaud-sonarsource marked an inline comment as not done. frederic-tingaud-sonarsource added a comment. As I don't have merge rights, would it be possible for you to merge it? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124845/new/ https://reviews.llvm.org/D124845

[PATCH] D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator

frederic-tingaud-sonarsource marked an inline comment as done. frederic-tingaud-sonarsource added inline comments. Comment at: clang/include/clang/Analysis/ConstructionContext.h:122-124 assert(isa(E) || isa(E) || - isa(E) || isa(E)); + isa(E) || isa(E)

[PATCH] D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator

frederic-tingaud-sonarsource updated this revision to Diff 427332. frederic-tingaud-sonarsource added a comment. Applying recommendations CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124845/new/ https://reviews.llvm.org/D124845 Files:

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

frederic-tingaud-sonarsource added a comment. By the way, in the current state of the code, there is no risk of such conflict between typo correction and UnqualifiedBase, because when an unqualified base exists the lookup result will not be empty. That's why the test was not failing even

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

frederic-tingaud-sonarsource updated this revision to Diff 426944. frederic-tingaud-sonarsource added a comment. Sorry about the confusion, it makes sense. Fixed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124666/new/ https://reviews.llvm.org/D124666 Files:

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

frederic-tingaud-sonarsource updated this revision to Diff 426759. frederic-tingaud-sonarsource added a comment. Added two similar names in the tests, to try to trick the typo correction. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124666/new/ https://reviews.llvm.org/D124666 Files:

[PATCH] D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator

frederic-tingaud-sonarsource created this revision. frederic-tingaud-sonarsource added a reviewer: dcoughlin. Herald added subscribers: manas, ASDenysPetrov, martong, dkrupp, donat.nagy, Szelethus, a.sidorin, baloghadamsoftware. Herald added a project: All. frederic-tingaud-sonarsource requested

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

frederic-tingaud-sonarsource updated this revision to Diff 426022. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124666/new/ https://reviews.llvm.org/D124666 Files: clang/include/clang/Basic/DiagnosticSemaKinds.td clang/lib/Sema/SemaDeclCXX.cpp

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

frederic-tingaud-sonarsource updated this revision to Diff 426021. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124666/new/ https://reviews.llvm.org/D124666 Files: clang/include/clang/Basic/DiagnosticSemaKinds.td clang/lib/Sema/SemaDeclCXX.cpp

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

frederic-tingaud-sonarsource created this revision. frederic-tingaud-sonarsource added a reviewer: rnk. Herald added a project: All. frederic-tingaud-sonarsource requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Before C++20, MSVC was

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

frederic-tingaud-sonarsource added a comment. @rnk I don't have the rights to merge. Could you do it when you have the time? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124613/new/ https://reviews.llvm.org/D124613

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

frederic-tingaud-sonarsource added a comment. I haven't searched far in the past, but I expect this behavior to go way back, yes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124613/new/ https://reviews.llvm.org/D124613

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

frederic-tingaud-sonarsource created this revision. frederic-tingaud-sonarsource added a reviewer: rnk. Herald added a reviewer: shafik. Herald added a project: All. frederic-tingaud-sonarsource requested review of this revision. Herald added a project: clang. Herald added a subscriber: