SLVS-1717 Fix C++ Language standard recognition when a C language standard is also specified #5948
Conversation
…ndard is also specified * Only set one language standard for the file * Always set either /TC or /TP flag for the file (/TC for C and /TP for CPP) * Assume the file is C if CompileAs == CompileAsC or if CompileAs != CompileAsCpp and vcFile.ContentType == CCode * ASsume the file is CPP otherwise
georgii-borovinskikh-sonarsource
requested review from
michael-jabbour-sonarsource and
gabriela-trutan-sonarsource
| var projectItemConfig = new ProjectItemConfig | ||
| { | ||
| ItemType = "ClCompile", | ||
| FileConfigProperties = new Dictionary<string, string> |
There was a problem hiding this comment.
Please extract the creation of the properties in a method to prevent cluttering the test. Then reuse the method in all other tests.
| private void AddLanguageFlags(IVCRulePropertyStorage properties) | ||
| { | ||
| var (compileAsFlag, languageStandardFlag) = languageFlagsProvider.GetLanguageConfiguration( | ||
| GetPotentiallyUnsupportedPropertyValue(properties, "CompileAs", ""), |
There was a problem hiding this comment.
Please use string.Empty
| GetPotentiallyUnsupportedPropertyValue(properties, "CompileAs", ""), | |
| GetPotentiallyUnsupportedPropertyValue(properties, "CompileAs", string.Empty), |
| { | ||
| var (compileAsFlag, languageStandardFlag) = languageFlagsProvider.GetLanguageConfiguration( | ||
| GetPotentiallyUnsupportedPropertyValue(properties, "CompileAs", ""), | ||
| GetPotentiallyUnsupportedPropertyValue(properties, "LanguageStandard_C", ""), |
There was a problem hiding this comment.
| GetPotentiallyUnsupportedPropertyValue(properties, "LanguageStandard_C", ""), | |
| GetPotentiallyUnsupportedPropertyValue(properties, "LanguageStandard_C", string.Empty), |
| var (compileAsFlag, languageStandardFlag) = languageFlagsProvider.GetLanguageConfiguration( | ||
| GetPotentiallyUnsupportedPropertyValue(properties, "CompileAs", ""), | ||
| GetPotentiallyUnsupportedPropertyValue(properties, "LanguageStandard_C", ""), | ||
| GetPotentiallyUnsupportedPropertyValue(properties, "LanguageStandard", "")); |
There was a problem hiding this comment.
| GetPotentiallyUnsupportedPropertyValue(properties, "LanguageStandard", "")); | |
| GetPotentiallyUnsupportedPropertyValue(properties, "LanguageStandard", string.Empty)); |
| value switch | ||
| { | ||
| // https://github.com/SonarSource/sonarlint-visualstudio/issues/738 | ||
| "" or "Default" => "", |
There was a problem hiding this comment.
| "" or "Default" => "", | |
| "" or "Default" => string.Empty, |
| public static string GetCppStandardFlagValue(string value) => | ||
| value switch | ||
| { | ||
| null or "" or "Default" => "", |
There was a problem hiding this comment.
| null or "" or "Default" => "", | |
| null or "" or "Default" => string.Empty, |
| public static string GetCStandardFlagValue(string value) => | ||
| value switch | ||
| { | ||
| null or "" or "Default" => "", |
There was a problem hiding this comment.
| null or "" or "Default" => "", | |
| null or "" or "Default" => string.Empty, |
| { | ||
| var compileAsFlag = CompileAsValueConverter.GetFlagValue(compileAs); | ||
|
|
||
| return compileAsFlag == CompileAsCFlag || (compileAsFlag != CompileAsCppFlag && vcFileContentType == ContentTypeCCode) |
There was a problem hiding this comment.
Would it make sense to add a comment to explain why checking compileAsFlag isn't sufficient?
I think we mentioned that we now need vcFileContentType as well for .c files when compileAsFlag is kept as default.
| [DataRow(true, "", "", "")] | ||
| [DataRow(false, "1", "", "1 ")] | ||
| [DataRow(true, "1", "", "1 ")] | ||
| [DataRow(false, "", "2", "2 ")] |
There was a problem hiding this comment.
Minor comment:
The real LanguageFlagsProvider seems to always return a non-empty string for the compile-as flag (i.e. either /TC or /TP, but never blank), as described in the PR summary. This implies that the expected command should always have either of these.
I was confused by the expected "2 " here (which means that no /TC or /TP were added). As far as I understand, the mock in this test doesn't behave similarly to the real implementation since it returns empty compileAs switches.
Would it make sense to somehow align the behavior of the mock here with the real implementation by always returning a non-empty for compileAsFlag?
(I would leave it for you to decide if it is worth it, I am just sharing it because it took me a bit to understand what is going on)
There was a problem hiding this comment.
The idea of the test was more to show that the implementation for choosing the values is delegated and should be tested in the LanguageFlagsProviderTests. I will just change the values to be more readable than just 1 & 2.
|
georgii-borovinskikh-sonarsource
deleted the
gb/fix-cfamily-language-standard
branch
/TCor/TPflag for the file (/TCfor C and/TPfor CPP)CompileAsCor if notCompileAsCppandvcFile.ContentTypeisCCodeNote: it's entirely possible to have a file with
CompileAsCppandCCode,CompileAsCandCppCode,DefaultandCppHeaderor any other combination of these values by manually setting the overrides in vcxproj. For consistency,CompileAsis chosen as the primary indicator for the language,vcFile.ContentTypeis only used ifCompileAsis not set toCompileAsCpp