Update embedded CFamily analyzer from 6.29.0.41127 to 6.31.0.44497 #2874
Conversation
AbbasNS
force-pushed
the
as/UseNewMode
branch
from
fd13eab
to
79df151
Compare
AbbasNS
force-pushed
the
as/UseNewMode
branch
from
79df151
to
db8b9d4
Compare
AbbasNS
force-pushed
the
as/cpp-analyzer
branch
from
824e200
to
fe109b3
Compare
AbbasNS
force-pushed
the
as/cpp-analyzer
branch
from
fe109b3
to
e295964
Compare
AbbasNS
changed the title
AbbasNS
changed the title
|
Hi @abbas-sabra-sonarsource. It looks like v6.32 may contain breaking changes around the legacy key changes so we'll need to do some more investigation before we merge it (see #2956). Can you think of anything that might cause a breaking change in v6.31? I can't see anything that looks problematic in the release notes. Assuming v6.31 doesn't require any more work on our side it's likely that we'll release that first and v6.32 later. |
|
Hello @duncanp-sonar, I confirm there are no breaking changes in |
AbbasNS
force-pushed
the
as/cpp-analyzer
branch
from
32c1493
to
e295964
Compare
AbbasNS
changed the title
|
@duncanp-sonar I changed the ticket and branch to target |
| // Use PCH in header files if project PCH is configured and not enforced through force include | ||
| // In normal code, it should be self to assume this since PCH should be used on every CPP file | ||
| if (string.IsNullOrEmpty(forcedIncludeFiles) && precompiledHeader == "Use") | ||
| // in case ClCompilerPath is not available on VS2017 |
There was a problem hiding this comment.
@abbas-sabra-sonarsource we've dropped support for VS 2017, so this code would only run in VS 2019+. Does this have any implications on the code below? Can it be simplified?
There was a problem hiding this comment.
Even though we dropped support to VS 2017, its toolchain can still be used with newer vs. So better to keep it. maybe we can improve the comment to say VS2017 toolchains
There was a problem hiding this comment.
I'll update the comment, toolchains helps to understand the problem much better 👍
|
|
||
| namespace SonarLint.VisualStudio.Integration.Vsix.CFamily.VcxProject | ||
| { | ||
| internal class CmdBuilder |
There was a problem hiding this comment.
@abbas-sabra-sonarsource is this the same logic that exists on the java side? it could be useful to link to it, so we could diff changes more easily.
There was a problem hiding this comment.
No. This file is loading options from the UI and is specific to VS. the idea was that CFamily java side change on every release. It is really hard to synchronize especially after a lot of refactoring on the java side that has no impact on SonarLint. We had a lot of bugs because it is hard to synchronize. Now we only have this code that is specific to the CFamily compilation options. It is localized and isolated. The idea is to be able to update the CFamily analyzer without any changes on the SonarLint side. Like the update to 6.30, 6.31..
Later, we can even drop the entire file if we find a way to load All option UI (as mentioned in #2661)
Of course CmdBuilder.cs need to be maintained to handle two things: discovering bugs and VS adding new compiler options in their UI(which doesn't happen that often). I'm happy to maintain CmdBuilder.cs which should be low effort. It should be the only place that contains CFamily specific code that supposes to evolve with new VS releases
| case "internal.fileDependency": // not currently handled. See https://github.com/SonarSource/sonarlint-visualstudio/issues/2611 | ||
| // Rules that start with internal shouldn't be treated as issues. | ||
| // Some of them should be handled like `internal.fileDependency`. See: https://github.com/SonarSource/sonarlint-visualstudio/issues/2611 | ||
| // Others should can simply ignored like `internal.z3RefutationRate`, which is used to log in the scanner how many issues are rejected by the Z3 solver |
There was a problem hiding this comment.
is this a bug fix? is this something that we already accidently report to the user?
There was a problem hiding this comment.
It can be a bug, but before it wasn't since most of the internal messages has invalid loc (line number 0..) which is ignored. but that is not a contract that we maintain.
I added this here because it is the right behavior and this code impacted the integration test after fixing the bug about Z3 (#2875) in the "Use the subprocess SonarLint mode" branch.
By the way, you are reviewing the #2661 code in the update of analyzer PR. even though they dependents on each other might be better to separate the review.
There was a problem hiding this comment.
Thanks for pointing that out, this branch contained everything, hence I missed the other PR.
I've changed the base of this branch to point to the other branch, and it makes it much simpler now.
rita-gorokhod
force-pushed
the
as/cpp-analyzer
branch
from
fb66072
to
8187132
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
fixes #2873 depends on #2661 (PR #2872)