NRT Migration Phase 1 #317
Conversation
I'm not sure how these didn't show up beforehand.
| m_ungot = top.m_value; | ||
| Pop(ref top,((ParserReduce)pe).m_depth,er); | ||
| newtop.pos = top.m_value.pos; | ||
| newtop!.pos = top.m_value.pos; |
There was a problem hiding this comment.
wont this throw if pe.m_action is null?
There was a problem hiding this comment.
Yes, it will. The way I interpret this section is, "if pe is non-null, so is pe.m_action"
src/CSTools/Tools/parser.cs
Outdated
| for (;;) | ||
| { | ||
| string cnm = top.m_value.yyname(); | ||
| string cnm = top.m_value!.yyname(); |
There was a problem hiding this comment.
this could be null because top is reassigned in the call to Pop(ref top ... meaning m_value could be null here. I say we just explicitly throw a null error before this line if top.m_value is null as that's what it would have done before.
There was a problem hiding this comment.
But it returns after that call. It shouldn't ever do another iteration in this loop if it gets the successful parse. Right?
There was a problem hiding this comment.
oh you're right, man this code is just confusing. I'd still be inclined to throw as that's always going to be a better error then what gets thrown automatically, and then it's obvious to anyone looking at the code what will happen if it's null here.
src/SIL.LCModel.Core/WritingSystems/CoreWritingSystemDefinition.cs
Outdated
Show resolved
Hide resolved
src/SIL.LCModel/Application/ApplicationServices/XmlImportData.cs
Outdated
Show resolved
Hide resolved
src/SIL.LCModel/Application/ApplicationServices/XmlImportData.cs
Outdated
Show resolved
Hide resolved
hahn-kev
left a comment
hahn-kev
left a comment
There was a problem hiding this comment.
left some more comments
| MergeRedundantEntries(leDup, le); | ||
| List<WsString> delKeys = new List<WsString>(); | ||
| foreach (var x in m_mapFormEntry) | ||
| foreach (var x in m_mapFormEntry!) |
There was a problem hiding this comment.
this could be removed if we annotate FillEntryMap with [MemberNotNull(nameof(m_mapFormEntry))] which indicates that calling this method will make the field not null
There was a problem hiding this comment.
Oddly enough, this required me to upgrade to C# 9. I would've thought it wouldn't matter if we're using the Nullable package?
src/SIL.LCModel/Application/ApplicationServices/XmlImportData.cs
Outdated
Show resolved
Hide resolved
src/SIL.LCModel/Application/ApplicationServices/XmlImportData.cs
Outdated
Show resolved
Hide resolved
src/SIL.LCModel/Application/ApplicationServices/XmlImportData.cs
Outdated
Show resolved
Hide resolved
| break; | ||
| case CellarPropertyType.Integer: | ||
| int val = sda.get_IntProp(cmoOld.Hvo, flid); | ||
| int val = sda!.get_IntProp(cmoOld.Hvo, flid); |
There was a problem hiding this comment.
now that you're throwing when sda is null I don't think you need ! here or any of the other lines below
src/SIL.LCModel/DomainServices/DataMigration/DataMigration7000010.cs
Outdated
Show resolved
Hide resolved
tests/SIL.LCModel.FixData.Tests/SIL.LCModel.FixData.Tests.csproj
Outdated
Show resolved
Hide resolved
hahn-kev
left a comment
hahn-kev
left a comment
There was a problem hiding this comment.
looks good to me, I think there's still a couple comments to resolve. I left 2 comments about weird indenting in the test projects but they're all weird.
|
@hahn-kev I think I got everything. Thanks for approving |
This change is