Add argument validation to public APIs. Throw or Precondition/assert failure?#2189
Add argument validation to public APIs. Throw or Precondition/assert failure?#2189ahsonkhan merged 16 commits intoAzure:masterfrom
Conversation
antkmsft
left a comment
antkmsft
left a comment
There was a problem hiding this comment.
I like either exceptions or sometimes just going without validation is also fine, especially when do-noting/0-result and so on is possible, but sometimes when validation is expensive UB is fine. Not a fan of asserts/exits/terminations. But I my views are probably not mainstream, at least in our team.
vhvb1989
left a comment
vhvb1989
left a comment
There was a problem hiding this comment.
The caller should fix the issue instead of adding code to recover for trying, for example, to read negative values.
|
This pull request is protected by Check Enforcer. What is Check Enforcer?Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass. Why am I getting this message?You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged. What should I do now?If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows: What if I am onboarding a new service?Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment: |
| AZURE_ASSERT_MSG( | ||
| (data && length >= 0) || (!data && length == 0), | ||
| "Length cannot be negative, and length must be 0 if the data pointer is null."); |
There was a problem hiding this comment.
length >= 0 is a tautology; std::size_t cannot represent negative values. Also, you've strengthened the condition here to forbid data != nullptr && length == 0 which seems like a perfectly valid empty range.
| AZURE_ASSERT_MSG( | |
| (data && length >= 0) || (!data && length == 0), | |
| "Length cannot be negative, and length must be 0 if the data pointer is null."); | |
| AZURE_ASSERT_MSG( | |
| data != nullptr || length == 0, | |
| "Length must be 0 if the data pointer is null."); |
There was a problem hiding this comment.
length >= 0is a tautology;std::size_tcannot represent negative values.
Agreed, already fixed. The change you were looking at is outdated.
Also, you've strengthened the condition here to forbid
data != nullptr && length == 0which seems like a perfectly valid empty range.
That was intentional, but let me revisit that to make sure it is what we want. Good call out :)
There was a problem hiding this comment.
Also, you've strengthened the condition here to forbid
data != nullptr && length == 0which seems like a perfectly valid empty range.
With the original assert ((data && length >= 0) || (!data && length == 0)), I don't think I had. Isn't that identical? That said, yours is simpler.
|
|
||
| FileBodyStream::FileBodyStream(const std::string& filename) | ||
| { | ||
| AZURE_ASSERT_MSG(filename.size() > 0, "The file name must not be an empty string."); |
There was a problem hiding this comment.
It seems peculiar to me that this particular invalid file name is forbidden by precondition, whereas all other invalid file names result in a runtime error. I'd remove this precondition and let all invalid filenames be treated the same.
There was a problem hiding this comment.
Unlike other filename checks, the size of string is something the caller can trivially check. At least, that's what motivated this change. An empty string size is potentially an upstream string parsing bug, whereas invalid filename could be caused by file not existing or other reasons, so the type of root causes of the errors seem sufficiently different to me.
| #if defined(NDEBUG) | ||
| // Release build won't provide assert msg | ||
| ASSERT_DEATH(tb.Rewind(), ""); | ||
| #else | ||
| ASSERT_DEATH( | ||
| tb.Rewind(), | ||
| "The specified BodyStream doesn't support Rewind which is required to guarantee fault "); |
There was a problem hiding this comment.
This is begging for a new macro ASSERT_DEATH_MEOW(x, y) that expands to ASSERT_DEATH(x, "") when defined(NDEBUG) and to ASSERT_DEATH(x, y) otherwise.
There was a problem hiding this comment.
sure, it's a Macro just for tests. We can create an azure_assert_test.hpp with this macro and use it from cpp. It's a pretty suggestion :)
There was a problem hiding this comment.
Even a function would be good instead, as usually we don't like MACROS as per our guidelines :)
5 participants
Fixes #1769
Currently, the PR shows one mechanism of validating arguments, which is to check and throw an exception.Depending on which approach we take, new tests would be needed.
Depends on what we do in #2170