Fix T76276 : Cleanup conversion of variable string names via hashed enumeration
AbandonedPublic
Actions

Authored by Nikhil Shringarpurey (Nikhil.Net) on Apr 30 2020, 10:46 PM.

Details

Reviewers

Sebastián Barschkis (sebbas)

Maniphest Tasks

T76276: Compiler Error C1061 due to too many nested if/else in MANTA_main.cpp

Summary

Fix T76276
This diff changes the behavior of using a very long series of string comparisons to check variable names and take action.

Originally, the code used a lot of if/else clauses, leading to the error in T76276 which blocked MSBuild due to exceeding 128 nested if/else statements.

The short-term fix was to convert them all to if statements, but that has a performance impact as every if is now checked for every variable that is tested. This diff creates an enumeration that allows the main code to use a switch() block, making it faster and removing any need for nesting if/elses in the future.

This is my first code submission, and I am open to adjusting/tweaking names as suggested by more senior folks.

Diff Detail

Repository: rB Blender

Event Timeline

Nikhil Shringarpurey (Nikhil.Net) requested review of this revision.Apr 30 2020, 10:46 PM

Nikhil Shringarpurey (Nikhil.Net) created this revision.

Nikhil Shringarpurey (Nikhil.Net) mentioned this in T76276: Compiler Error C1061 due to too many nested if/else in MANTA_main.cpp.

Added comments as suggested.

Ankit Meel (ankitm) retitled this revision from Cleanup conversion of variable string names via hashed enumeration to Fix T76276 : Cleanup conversion of variable string names via hashed enumeration.May 1 2020, 7:51 AM

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)

Ankit Meel (ankitm) added a reviewer: Ray Molenkamp (LazyDodo).

Ankit Meel (ankitm) added a task: T76276: Compiler Error C1061 due to too many nested if/else in MANTA_main.cpp.

Ankit Meel (ankitm) added a subscriber: Ankit Meel (ankitm).

Thanks for the patch! I'll have to give this some thought. Since getRealValueHash() is still doing all the string comparisons I am not sure if this is much more performant though.
In the current version of getRealValue() the if blocks could also immediately return a string.

Ray Molenkamp (LazyDodo) removed a reviewer: Ray Molenkamp (LazyDodo).May 1 2020, 2:53 PM

In D7581#182828, @Sebastián Barschkis (sebbas) wrote:

Thanks for the patch! I'll have to give this some thought. Since getRealValueHash() is still doing all the string comparisons I am not sure if this is much more performant though.
In the current version of getRealValue() the if blocks could also immediately return a string.

Please do. The thought is that this would be the same as what the old code was doing with the if/else structure. Since you had to remove the else's with yesterday's patch, that introduced some slowdown due to each if being evaluated. So this wouldn't really improve from the original code so much as restore performance back to that level.

The other option that I came up with was to use a hashing function and hash the strings to an integer value, but this would make the code really unreadable so I didn;t do that. As before, I trust your knowledge more than mine in this area though! :)

Committed a refactor that uses a hash map and is in my view a bit more concise (rB99ee1de094a1). Thanks for the patch anyways and feel free to add to the hash map approach!