-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rebuild MS.CA.VisualBasic #51532
Rebuild MS.CA.VisualBasic #51532
Conversation
Fixed two issues: - The way VB preporocessor symbols are encoded and decoded - Syntax files were using the on disk path which can have casing differences from the PDB path, flipped to PDB path
@dotnet/roslyn-compiler PTAL |
@@ -4,7 +4,7 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
<PropertyGroup> | |||
<OutputType>Exe</OutputType> | |||
<TargetFrameworks>netcoreapp3.1;net472</TargetFrameworks> | |||
<TargetFrameworks>net472;netcoreapp3.1</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done to match the ordering of TargetFrameworks
in the csc / vbc project files. This way when F5 between them and BuildValidator locally you're using the same runtime. Important for inner dev loop.
Dim preprocessorStrings = Options.ParseOptions.PreprocessorSymbols.Select( | ||
Function(p) As String | ||
If TypeOf p.Value Is String Then | ||
Return p.Key + "=""" + p.Value.ToString() + """" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.ToString() [](start = 58, length = 11)
Is .ToString()
necessary?
IReadOnlyDictionary<string, object>? preprocessorSymbols = null; | ||
if (OptionToString(CompilationOptionNames.Define) is string defineString) | ||
{ | ||
preprocessorSymbols = VisualBasicCommandLineParser.ParseConditionalCompilationSymbols(defineString, out var diagnostics); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we just allow this diagnostic bag to propagate out, rather than passing one in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started with than and changed the return to (Compilation? Compilation, DiagnosticBag? DiagnosticBag)
but I felt the consumption code was very awkward to write. Eventually I went with the approach of passing a bag down as we do in other parts of the code and it felt a bit more natural.
Looks like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto-approval
Fixed two issues:
differences from the PDB path, flipped to PDB path