Page MenuHomePhabricator

frederic-tingaud-sonarsource (Fred Tingaud)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 28 2022, 2:09 AM (9 w, 4 d)

Recent Activity

Fri, Jun 24

frederic-tingaud-sonarsource updated the diff for D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments.

Adding an additional false negative fixed by this patch.

Fri, Jun 24, 6:10 AM · Restricted Project, Restricted Project

Tue, Jun 14

frederic-tingaud-sonarsource updated the diff for D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments.

Update diagnostic location to highlight the whole content of the initialization.

Tue, Jun 14, 6:35 AM · Restricted Project, Restricted Project

Tue, Jun 7

frederic-tingaud-sonarsource added a comment to D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments.

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<A>(b = createB());
                         ^~~~~~~~~

(I'm switching back to C++ because I'm not sure exactly how legal such a thing would be in objective-C).
With the current fix, that doesn't change.
It might be clearer for the user to either put the diagnostic on the whole assignment or at least the whole initialization.

Tue, Jun 7, 5:45 AM · Restricted Project, Restricted Project

Jun 1 2022

frederic-tingaud-sonarsource added a comment to D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments.

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 point about pointing at something that is nearer to the assignment, but in that case, wouldn't it make sense to point at the whole assignment?

id obj1 = (__bridge_transfer id)CFCreateSomething(); // expected-warning{{never read}}
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Jun 1 2022, 6:27 AM · Restricted Project, Restricted Project

May 27 2022

frederic-tingaud-sonarsource added a comment to D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments.

Thanks for looking at the PR!

May 27 2022, 7:56 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource updated the diff for D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments.

Document tests better

May 27 2022, 7:48 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource updated the diff for D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments.

Fix formatting

May 27 2022, 7:15 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource requested review of D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments.
May 27 2022, 6:08 AM · Restricted Project, Restricted Project

May 20 2022

frederic-tingaud-sonarsource added a comment to D124613: In MSVC compatibility mode, friend function declarations behave as function declarations.

@rnk Here is the test case that was broken: https://godbolt.org/z/Gef7PcqMf

May 20 2022, 6:38 AM · Restricted Project, Restricted Project

May 13 2022

frederic-tingaud-sonarsource added a comment to D124613: In MSVC compatibility mode, friend function declarations behave as function declarations.

I realized that this current patch does in fact mimic MSVC behavior up to its mishandling of compliant code. I understand that it is not in the spirit of clang's MSVC compatibility to have that, but the fixed version I was about to propose would stray away from MSVC behavior, which doesn't really make sense either.
So instead I think the right approach should be to revert and see later whether we could put it back under the hood of another flag.
Is there currently a flag that could be used for "potentially C++ compliant breaking MSVC compatibility"? If there isn't, would it make sense to propose one?

May 13 2022, 2:56 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource requested review of D125529: Revert MSVC compatibility feature that breaks conformant code.
May 13 2022, 2:45 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource abandoned D125526: Fix ADL leakage due to MSVC compatibility flag.

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 to see if there is a way to have it under another flag or something.

May 13 2022, 2:38 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource added a comment to D124613: In MSVC compatibility mode, friend function declarations behave as function declarations.

EDIT: patch cancelled, see next comment.

May 13 2022, 1:44 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource requested review of D125526: Fix ADL leakage due to MSVC compatibility flag.
May 13 2022, 1:41 AM · Restricted Project, Restricted Project

May 12 2022

frederic-tingaud-sonarsource added a comment to D124613: In MSVC compatibility mode, friend function declarations behave as function declarations.

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 gracefully as possible.
When we find gaps between MSVC and clang-cl, we try to fix them and if we think the fix will have negligible memory/performance/complexity impact on clang and be useful to the community, we try to upstream them. It's true that this fix purpose is not to fix handling of MSVC standard headers, but there are more and more tools that also target existing MSVC code (clang-tidy, Clang Power Tools, etc.) thanks to this compatibility feature and we felt that it could be valuable.
We also try to always have a warning attached to MSVC extensions, but this one has two particularities: it builds on top of an already existing MSVC compatibility feature (PerformFriendInjection in Decl::setObjectOfFriendDecl) and there is no way (without a big refactoring), that would allow to warn correctly when the extension is actually used.

May 12 2022, 7:02 AM · Restricted Project, Restricted Project

May 11 2022

frederic-tingaud-sonarsource added a comment to D124613: In MSVC compatibility mode, friend function declarations behave as function declarations.

Should we revert this, at least for now, until the investigation is complete?

May 11 2022, 10:31 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource added a comment to D124613: In MSVC compatibility mode, friend function declarations behave as function declarations.

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 make sure I'm not missing something important and push a pull-request.

May 11 2022, 10:23 AM · Restricted Project, Restricted Project

May 9 2022

frederic-tingaud-sonarsource added a comment to D124613: In MSVC compatibility mode, friend function declarations behave as function declarations.

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.

May 9 2022, 7:04 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource updated the diff for D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator.

Fix formatting

May 9 2022, 5:31 AM · Restricted Project, Restricted Project

May 5 2022

frederic-tingaud-sonarsource added a comment to D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator.

As I don't have merge rights, would it be possible for you to merge it?

May 5 2022, 8:47 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource added inline comments to D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator.
May 5 2022, 8:29 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource updated the diff for D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator.

Applying recommendations

May 5 2022, 8:27 AM · Restricted Project, Restricted Project

May 4 2022

frederic-tingaud-sonarsource added a comment to D124666: In MSVC compatibility mode, handle unqualified templated base class initialization.

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 without moving the compatibility first.

May 4 2022, 5:45 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource updated the diff for D124666: In MSVC compatibility mode, handle unqualified templated base class initialization.

Sorry about the confusion, it makes sense. Fixed.

May 4 2022, 1:44 AM · Restricted Project, Restricted Project

May 3 2022

frederic-tingaud-sonarsource updated the diff for D124666: In MSVC compatibility mode, handle unqualified templated base class initialization.

Added two similar names in the tests, to try to trick the typo correction.

May 3 2022, 10:19 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource requested review of D124845: StaticAnalyzer should inline overridden delete operator the same way as overridden new operator.
May 3 2022, 5:20 AM · Restricted Project, Restricted Project

Apr 29 2022

frederic-tingaud-sonarsource updated the diff for D124666: In MSVC compatibility mode, handle unqualified templated base class initialization.
Apr 29 2022, 5:09 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource updated the diff for D124666: In MSVC compatibility mode, handle unqualified templated base class initialization.
Apr 29 2022, 5:03 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource requested review of D124666: In MSVC compatibility mode, handle unqualified templated base class initialization.
Apr 29 2022, 3:26 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource added a comment to D124613: In MSVC compatibility mode, friend function declarations behave as function declarations.

@rnk I don't have the rights to merge. Could you do it when you have the time?

Apr 29 2022, 1:38 AM · Restricted Project, Restricted Project

Apr 28 2022

frederic-tingaud-sonarsource added a comment to D124613: In MSVC compatibility mode, friend function declarations behave as function declarations.

I haven't searched far in the past, but I expect this behavior to go way back, yes.

Apr 28 2022, 8:42 AM · Restricted Project, Restricted Project
frederic-tingaud-sonarsource requested review of D124613: In MSVC compatibility mode, friend function declarations behave as function declarations.
Apr 28 2022, 7:00 AM · Restricted Project, Restricted Project