-
Notifications
You must be signed in to change notification settings - Fork 123
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
Make Platform functions to return az_result #1256
Changes from 6 commits
a88401f
97a83a1
292d43f
9ef7685
74fa1ef
94bf74f
d67c4a7
be7d438
c8a87cf
908d9c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,19 @@ | |
// SPDX-License-Identifier: MIT | ||
|
||
#include <azure/core/az_platform.h> | ||
#include <azure/core/internal/az_precondition_internal.h> | ||
|
||
#include <azure/core/_az_cfg.h> | ||
|
||
AZ_NODISCARD int64_t az_platform_clock_msec() { return 0; } | ||
AZ_NODISCARD az_result az_platform_clock_msec(int64_t* out_clock_msec) | ||
{ | ||
_az_PRECONDITION_NOT_NULL(out_clock_msec); | ||
*out_clock_msec = 0; | ||
return AZ_ERROR_DEPENDENCY_NOT_PROVIDED; | ||
} | ||
|
||
void az_platform_sleep_msec(int32_t milliseconds) { (void)milliseconds; } | ||
AZ_NODISCARD az_result az_platform_sleep_msec(int32_t milliseconds) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at this again, I agree with Rick on the sleep API, we should consider removing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should. It is us who calls these APIs. Rick's assumption is that 3 things are happening: 1. Customer calls PAL function. 2. Doesn't handle a result, and doesn't need/want to. 3. It is painful to ignore the result, which I think is not:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case, we should consider updating the documentation of |
||
{ | ||
(void)milliseconds; | ||
return AZ_ERROR_DEPENDENCY_NOT_PROVIDED; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,25 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// SPDX-License-Identifier: MIT | ||
|
||
#include <azure/core/internal/az_config_internal.h> | ||
#include <azure/core/az_platform.h> | ||
#include <azure/core/internal/az_config_internal.h> | ||
#include <azure/core/internal/az_precondition_internal.h> | ||
|
||
#include <time.h> | ||
|
||
#include <unistd.h> | ||
|
||
#include <azure/core/_az_cfg.h> | ||
|
||
AZ_NODISCARD int64_t az_platform_clock_msec() | ||
AZ_NODISCARD az_result az_platform_clock_msec(int64_t* out_clock_msec) | ||
{ | ||
return (int64_t)((clock() / CLOCKS_PER_SEC) * _az_TIME_MILLISECONDS_PER_SECOND); | ||
_az_PRECONDITION_NOT_NULL(out_clock_msec); | ||
*out_clock_msec = (int64_t)((clock() / CLOCKS_PER_SEC) * _az_TIME_MILLISECONDS_PER_SECOND); | ||
return AZ_OK; | ||
} | ||
|
||
void az_platform_sleep_msec(int32_t milliseconds) | ||
AZ_NODISCARD az_result az_platform_sleep_msec(int32_t milliseconds) | ||
{ | ||
(void)usleep((useconds_t)milliseconds * _az_TIME_MICROSECONDS_PER_MILLISECOND); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unrelated: precondition that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So far we say in the docs, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We can add a precondition now to help folks write correct code, and loosen that in the future, if that value becomes meaningful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We could precondition, but we would be limiting to a particular implementation of POSIX. Its not clear if every implementation places the same constraints. e.g. some platforms say the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is one particular implementation though. Adding a precondition here doesn't impact users who want to provide their own PAL implementation. Passing in a negative millisecond will overflow when cast to unsigned int, and that could be an unintended bug in the user's code (given |
||
return AZ_OK; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
// SPDX-License-Identifier: MIT | ||
|
||
#include <azure/core/az_platform.h> | ||
#include <azure/core/internal/az_precondition_internal.h> | ||
|
||
// Two macros below are not used in the code below, it is windows.h that consumes them. | ||
#define WIN32_LEAN_AND_MEAN | ||
|
@@ -10,6 +11,15 @@ | |
|
||
#include <azure/core/_az_cfg.h> | ||
|
||
AZ_NODISCARD int64_t az_platform_clock_msec() { return GetTickCount64(); } | ||
AZ_NODISCARD az_result az_platform_clock_msec(int64_t* out_clock_msec) | ||
{ | ||
_az_PRECONDITION_NOT_NULL(out_clock_msec); | ||
*out_clock_msec = GetTickCount64(); | ||
return AZ_OK; | ||
} | ||
|
||
void az_platform_sleep_msec(int32_t milliseconds) { Sleep(milliseconds); } | ||
AZ_NODISCARD az_result az_platform_sleep_msec(int32_t milliseconds) | ||
{ | ||
Sleep(milliseconds); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think we should add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we have a variety of opinions, let's cover it outside of the scope of this PR, it can be added later without affecting this change. |
||
return AZ_OK; | ||
} |
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 an unnecessary burden to the implementor of the PAL.
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.
@ahsonkhan are you referring to the AZ_NODISCARD or the az_result. I agree that we shouldn't force AZ_NODISCARD onto the implementations. I however do prefer the az_result return codes on the PAL. The minimal burden is greatly outweighed by the fact we can consistently validate the API performed as expected
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.
Both.
I don't understand what you mean by this. What are we getting here?
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.
Ha, what I was thinking of, is that you Ahson was talking about the contrast of this variant to the proposal in #1257, saying that out parameters and az_result are generally more cumbersome to implement. I didn't read it as if you are commenting on the AZ_NODISCARD.
Regarding AZ_NODISCARD, I do not think it is a burden on the implementor. It is an intentional burden on the user, i.e. us. So, if we ignore the return result, we get a warning treated as compilation error. For the implementor, I am not sure if they even have to put AZ_NODISCARD in the signature of their imlpementation - having it in this header alone is probably enough (I am not sure, because AZ_NODISCARD is really compiler-specific).
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.
OMHO, platform should not depend on az_core. Only az_core should depend on some specific implementation.
Right now, in order to implement a platform, people need to get az_core, and people getting az_core need an az_platform. I don't like how that look XD. It works using cmake targets, but not sure if it would work without it (including pah directly)
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.
Interesting point on layering the dependency @vhvb1989