Implement and consume non-originating hresults

[antmor]

What: Address #1376

• Added new constructors to winrt::hresult_error to opt out of originating the error.
• Added an optional templated bool to map constructors and factories, to opt in to non-originating E_BOUNDS errors, since these errors are explicitly not helpful.
• To the cancellation_token, added a way to opt in to non-originating errors. Also
• added a .runsettings file to add the ability to run the tests from Visual Studio (using a Catch2ToVisualStudioTest adapter)

Why: When debugging explorer.exe, there are a lot of Cancellations and E_Bounds failures that pop up. To make the debugging experience better, let's opt out of those errors.

Verified: ran build_test_all.cmd Ran tests in Visual Studio:

[JaiganeshKumaran]

I don't think this is a good solution. You do want the error information when you call Lookup instead of TryLookup, but the latter does not really exist on the type.

[Scottj1s]

Generally, the approach here would be to use a policy template type so that there's no runtime impact (storage or execution). That would also support generality for other projected types.

E.g., single_threaded_map<hstring, int, ignore_errors>(...)

Can you refactor?

[kennykerr]

This seems like another good candidate to migrate to WIL and let cppwinrt focus on language projection.

[Scottj1s]

This seems like another good candidate to migrate to WIL and let cppwinrt focus on language projection.

Or at least split the responsibility - factor out an error handling policy template with default here, and let WIL supply specializations and/or subclasses

[antmor]

I don't think this is a good solution. You do want the error information when you call Lookup instead of TryLookup, but the latter does not really exist on the type.

The error for Lookup will still be thrown, and it will still be an E_BOUNDS hresult. The only difference is that it won't be ro-originated, thus it might be a tad harder to debug, but i assume E_BOUNDS is expected for a Lookup to fail, and thus it is not helpful to rooriginate a . And we would still log other errors outside of the E_BOUNDS.

[kennykerr]

Or at least split the responsibility - factor out an error handling policy template with default here, and let WIL supply specializations and/or subclasses

Or just use a WINRT_MIN_ORIGINATE preprocessor option. 😏

[JaiganeshKumaran]

I don't think this is a good solution. You do want the error information when you call Lookup instead of TryLookup, but the latter does not really exist on the type.

The error for Lookup will still be thrown, and it will still be an E_BOUNDS hresult. The only difference is that it won't be ro-originated, thus it might be a tad harder to debug, but i assume E_BOUNDS is expected for a Lookup to fail, and thus it is not helpful to rooriginate a . And we would still log other errors outside of the E_BOUNDS.

Then make it so that throwing E_BOUNDS/hresult_out_of_bounds never originates, instead of adding a separate flag for that?

[JaiganeshKumaran]

@kennykerr Hey Kenny, why don't you check out my pull request (#1375)?

[antmor]

This seems like another good candidate to migrate to WIL and let cppwinrt focus on language projection.

Or at least split the responsibility - factor out an error handling policy template with default here, and let WIL supply specializations and/or subclasses

Or just use a WINRT_MIN_ORIGINATE preprocessor option. 😏

I think we want something more scoped to these scenarios. Having it be all-or-nothing is a heavy hammer. I think it does make sense for cppwinrt to add a hook for both origination and logging, such that all HRESULTS will end up asking if there is an winrt_originate_and_log function, and if so, call that, else do a "default". (Right now, DavidMac only added the logging function). But at this function layer, we don't know the source of the hresult, whether it was a map_View or a vector_view or the async task framework, or a e_notimpl from a .as call, or an explicit throw winrt::hresult_error(...); by consuming code.

• For TryLookup E_BOUNDS, since TryLookup is a cppwinrt construct, and cppwinrt provides the map_view_base instance, we would have a map_view_rooriginate plugin function explicitly separate from the above. Or we could solve this at the ABI layer and create an interface IAttemptingLookupSuppressErrors that would tell the map we are asking to suppress errors. But at that point, might as well create IMapView2 with TryLookup explicitly. Either of those options are a bigger lift than fixing it at the implementation of map_view_base.
• For Async task cancellation, we could add a hook explicitly for ro-originating cancels.

the other options are for wil to re-implement the whole async framework / map_view_base creation. But those are also heavier lifts.

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.