Use std::error_code with custom error category #95

jimson-msft · 2021-10-13T03:07:02Z

No description provided.

Co-authored-by: Jimmy Song <55897143+jimson-msft@users.noreply.github.com> Cherry-picked from #89

… develop

shishirb-MSFT · 2021-10-13T21:46:01Z

sdk-cpp/include/do_errors.h

+
+private:
+    int32_t _code;
+    //error_category _category;


error_category

IIRC, this should be a reference. Commonly, a reference to a static instance. See the constructor here: https://en.cppreference.com/w/cpp/error/error_code/error_code

and cpprest's category here:
https://github.com/microsoft/cpprestsdk/blob/master/Release/src/utilities/asyncrt_utils.cpp

#Resolved

Now that we have a better understanding of this - I believe that all we need is our own error_category instances and not a new error_code class.

We should reuse the std::error_code class and provide our own error_category instance where relevant (i.e.,, when we store a DO/boost/libcurl error within).

We can have our own errc enum and still reuse std::error_code.

Let me know if you want to discuss this over a call.

Hmm agreed, rather than construct std::error_code with that extra param, what do you think of iter 15 where we use our own error_code class to supply the do_category via overloading category()

Thanks. Been thinking some more about this -
DU actually adds their own 'facility' bits to the error code to identify that the error came from DO.

So as a c++ library author, we should not worry about how our callers identify the source of errors in their telemetry. Each caller might want a different method.

Considering that, and the fact that we are trying to follow c++ standards, I think we should use std::error_code with our own error_category instances (more than one if needed; for now keep what we have).

You can leave the insertion of DO facility bits into the error codes as-is for now. Just remove the custom error_code class.

sounds good to me

sdk-cpp/src/do_download.cpp

sdk-cpp/include/do_error_macros.h

shishirb-MSFT · 2021-10-13T22:29:29Z

sdk-cpp/include/do_error_macros.h

-#endif
+#define DO_OK error_code(0)
+
+#define DO_SUCCEEDED(code) (((int32_t)(code.value())) < 0)


((int32_t)(code.value())) < 0

Since we expect only error_code type here, we should use its operator bool instead. At which point, I question the utility of these two macros. #Resolved

Read this comment after my other one in do_errors.h about using std::error_code.

shishirb-MSFT · 2021-10-13T22:44:50Z

sdk-cpp/include/do_error_macros.h

 #endif

-//TODO(jimson) Looks like these conversions are causing test issues with tests
+//TODO(jimson): Do we really need these after switching to error code class? The error_category should help us differentiate what type of error it is


need

Agree we don't need these anymore. #Resolved

Actually, wait, we need to think about this because DU client can only report an integer error code so the error_category information gets lost there.

Yeah that's a good point - so I'm not sure if using only the error_category is sufficient - unless we ask them to perform the bit operations to add our facility code based on the error category in returned error_code

Can't we expose error code with the facility bits already added to the internal value? Would this make the error category redundant if the error code itself contains the 'category' via the facility bits?

Let's still apply the facility code where needed for DU purposes, and make use of error_category

shishirb-MSFT · 2021-10-13T22:54:09Z

int32_t error_code() const;

IMO this is redundant since caller can get it from std::error_code. We should remove this and rename get_error_code() to error_code().

In reply to: 942778026

Refers to: sdk-cpp/include/do_errors.h:84 in efea6d9. [](commit_id = efea6d9, deletion_comment = False)

shishirb-MSFT

jimson-msft · 2021-10-14T21:41:01Z

So weird, there's some issues with github/CF - replying to a comment closed the PR. I'll look into submitting a ticket to CF team

In reply to: 943756987

… develop

jimson-msft · 2021-10-18T20:00:07Z

There are some failing tests with these changes, looking into fixing now

revoking review

shishirb-MSFT

…os work

jimson-msft · 2021-10-18T22:20:55Z

There are some failing tests with these changes, looking into fixing now

Resolved

shishirb-MSFT · 2021-10-18T23:18:39Z

sdk-cpp/src/do_errors.cpp


+std::error_code make_error_code(std::errc e)
+{
+    return std::error_code(DO_ERROR_FROM_STD_ERROR(e), do_category());


DO_ERROR_FROM_STD_ERROR

Need to fix tests to avoid doing this but it's okay for now.

I'll sign off.

Hmm I'm not sure I follow - I think we do need to do this in order to make use of the same error code macros/convention. the std::errc enum are a set of positive integer values, and so need to call DO_ERROR_FROM_STD_ERROR to make them hresult-like.

revoking review

shishirb-MSFT

shishirb-MSFT and others added 10 commits October 6, 2021 22:09

Templatize Github release jobs per platform & Tar the .deb output (#90)

b49acd2

Co-authored-by: Jimmy Song <55897143+jimson-msft@users.noreply.github.com> Cherry-picked from #89

Merge branch 'develop' of https://github.com/microsoft/do-client into…

ac1e9eb

… develop

Merge branch 'develop' of https://github.com/microsoft/do-client into…

a881893

… develop

use make method

375f541

fix some variant nits

498eab6

introduce error category class

53ce98a

convert internal error codes to new do error code

056bc33

fix error codes on linux side

7389b46

fix rest side

40cc949

add comment

efea6d9

jimson-msft requested a review from a team as a code owner October 13, 2021 03:07

shishirb-MSFT reviewed Oct 13, 2021

View reviewed changes

sdk-cpp/src/do_download.cpp Outdated Show resolved Hide resolved

shishirb-MSFT reviewed Oct 13, 2021

View reviewed changes

sdk-cpp/src/do_download.cpp Outdated Show resolved Hide resolved

shishirb-MSFT reviewed Oct 13, 2021

View reviewed changes

sdk-cpp/include/do_error_macros.h Outdated Show resolved Hide resolved

shishirb-MSFT reviewed Oct 13, 2021

View reviewed changes

shishirb-MSFT previously approved these changes Oct 13, 2021

View reviewed changes

cleanup exception class

f9930e6

jimson-msft closed this Oct 14, 2021

jimson-msft reopened this Oct 14, 2021

jimson-msft added 6 commits October 14, 2021 14:41

fix

ecd3dec

Merge branch 'develop' of https://github.com/microsoft/do-client into…

77d70cd

… develop

resolve merge conflict

7a4ad68

use our own error code class for referencing the do category

dbe216c

use std::error_code

1f6b0ef

download status now returns std::error_code

ef639ff

jimson-msft changed the title ~~Custom DO Error Class~~ Use std::error_code with custom error category Oct 18, 2021

shishirb-MSFT previously approved these changes Oct 18, 2021

View reviewed changes

jimson-msft added 3 commits October 18, 2021 13:44

std::errc should still be converted to use DO bits so that error macr…

d1f366b

…os work

fix

c3dc949

fix

fe846b3

shishirb-MSFT reviewed Oct 18, 2021

View reviewed changes

shishirb-MSFT approved these changes Oct 18, 2021

View reviewed changes

jimson-msft merged commit 5556b6d into develop Oct 19, 2021

jimson-msft deleted the user/jimson/error_class branch October 19, 2021 17:19

Use std::error_code with custom error category #95

Use std::error_code with custom error category #95

Uh oh!

Conversation

jimson-msft commented Oct 13, 2021

Uh oh!

shishirb-MSFT Oct 13, 2021 • edited by jimson-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shishirb-MSFT Oct 13, 2021 • edited by jimson-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Oct 13, 2021 • edited by jimson-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimson-msft Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT commented Oct 13, 2021 • edited by jimson-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shishirb-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

jimson-msft commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimson-msft commented Oct 18, 2021

Uh oh!

shishirb-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

jimson-msft commented Oct 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shishirb-MSFT Oct 13, 2021 •

edited by jimson-msft

Loading

shishirb-MSFT Oct 13, 2021 •

edited

Loading

shishirb-MSFT Oct 13, 2021 •

edited by jimson-msft

Loading

shishirb-MSFT Oct 13, 2021 •

edited by jimson-msft

Loading

shishirb-MSFT Oct 13, 2021 •

edited

Loading

jimson-msft Oct 14, 2021 •

edited

Loading

shishirb-MSFT commented Oct 13, 2021 •

edited by jimson-msft

Loading

jimson-msft commented Oct 14, 2021 •

edited

Loading