Out of interest, why not explicitly_PyErr_Clear(tstate) if_PyMapping_LookupItem returns-1?

Because the current code does not clear arbitrary errors (and doing this would be wrong). It only clearsKeyError, what is done internally by_PyMapping_LookupItem.

To me, "KeyError is silenced" suggests internal implementation details that the caller doesn't need to care about, and b) may or may not be true (in the exact-dict case, no KeyError is silenced because it isn't set in the first place). IMO "no KeyError is raised" is a clearer statement of the contract that is relevant to the caller.

On second thought, it may be important to specify "the KeyError is silenced" to clarify the behavior with a custom__getitem__ method that raisesKeyError explicitly. I take back that part of the suggestion; I would just replace all three instances ofa withthe.

I dislike specify that*pvalue is set to NULL on error. I would prefer to leave it undefined.

Code like(void)PyMapping_GetOptionalItem(modules, name, &m); would be unsafe: I prefer to handle explicitly the error case and no rely onm value.

In the implementation, I prefer to set it to NULL to avoid... the undefined behavior :-) Maybe in debug mode we could even set the value to a marker value to detect the usage of the undefined value.

I disagree. It will require explicitly settingresult = NULL before every call of this function, and if you forgot to do this, it will crash your code in rare case of error, which may be not covered by tests. The risk of writing errorenous code is too high even if there would be a benefit from not setting the result to NULL.

Also, it makes the code simpler. Instead of saving the return code into a variable and checking it in twoif\ s

intrc=PyMapping_GetOptionalItem(...);if (rc<0) {    gotoerror;}if (rc==0) {// handle default}

you can get rid of that variable and insert the call directly in theif.

if (PyMapping_GetOptionalItem(...)<0) {    gotoerror;}if (value==NULL) {// handle default}

And there are many other cases in which it is convenient that you have two ways to check for result. I suggest you to makePyMapping_GetOptionalItem() not setting the result to NULL on error and to look how much code it will require to rewrite and how much uglier it will become. It is the same forPyObject_GetOptionalAttr.

I disagree. It will require explicitly setting result = NULL before every call of this function, and if you forgot to do this, it will crash your code in rare case of error, which may be not covered by tests.

+1 and slightly related, I have a PR for the devguide repo regarding explicitly clearing output parameters:python/devguide#1129python/devguide#1128

cc.@serhiy-storchaka,@carljm

I dislike specify that*pvalue is set to NULL on error. I would prefer to leave it undefined.

Code like(void)PyMapping_GetOptionalItem(modules, name, &m); would be unsafe: I prefer to handle explicitly the error case and no rely onm value.

In the implementation, I prefer to set it to NULL to avoid... the undefined behavior :-) Maybe in debug mode we could even set the value to a marker value to detect the usage of the undefined value.