-
Notifications
You must be signed in to change notification settings - Fork 7
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
Even more comments #6
base: main
Are you sure you want to change the base?
Conversation
a2e0acc
to
7a123fa
Compare
/// Optionally, <see cref="ParallelOptions"/> can be passed in to configure parallelization. | ||
/// By default, the default settings are used (which has no limits). | ||
/// By default, the default settings are used (which have no limits). TODO: no limits on what??? |
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.
Was just specifying that the default ParallelOptions has no limit on the amount of threads it can use. Also yeah, maybe could abstract out the inner loop into a private function that can be called with a regular foreach? (in an overload)
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 meant the overload more because of options = null.
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'd rather resolve this before putting TODOs straight into the summary, lol.
} | ||
break; | ||
} | ||
// TODO: default case? |
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 just instruction pattern matching, it falls through to the return null;
afterwards if none of the conditions fully succeed.
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.
not what i meant. the todo was specifically referring to "Explicitly document all expected paths, and reserve default for unexpected value"
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'm still not sure why this needs to be here. The default case just means there's no function name to be found.
Underanalyzer/Decompiler/Warnings/DecompileDataLeftoverWarning.cs
Outdated
Show resolved
Hide resolved
7a123fa
to
ffc772d
Compare
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.
OK so just a few final nitpicky things before this is all set, I just don't want to make the code any more cluttered by adding TODOs into places that should really just get resolved (or if they potentially should just be in a GitHub issue)
} | ||
break; | ||
} | ||
// TODO: default case? |
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'm still not sure why this needs to be here. The default case just means there's no function name to be found.
/// Optionally, <see cref="ParallelOptions"/> can be passed in to configure parallelization. | ||
/// By default, the default settings are used (which has no limits). | ||
/// By default, the default settings are used (which have no limits). TODO: no limits on what??? |
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'd rather resolve this before putting TODOs straight into the summary, lol.
No description provided.