Address review comments.

Author: serhiy-storchaka

Lib/test/test_capi/test_misc.py

@@ -1113,39 +1113,39 @@ def test_sys_getattr(self):
         # CRASHES sys_getattr(NULL)
 
     def test_sys_getattrstring(self):
-        sys_getattr = _testcapi.sys_getattrstring
+        getattrstring = _testcapi.sys_getattrstring
 
-        self.assertIs(sys_getattr(b'stdout'), sys.stdout)
+        self.assertIs(getattrstring(b'stdout'), sys.stdout)
         with support.swap_attr(sys, '\U0001f40d', 42):
-            self.assertEqual(sys_getattr('\U0001f40d'.encode()), 42)
+            self.assertEqual(getattrstring('\U0001f40d'.encode()), 42)
 
         with self.assertRaisesRegex(RuntimeError, r'lost sys\.nonexisting'):
-            sys_getattr(b'nonexisting')
-        self.assertRaises(UnicodeDecodeError, sys_getattr, b'\xff')
-        # CRASHES sys_getattr(NULL)
+            getattrstring(b'nonexisting')
+        self.assertRaises(UnicodeDecodeError, getattrstring, b'\xff')
+        # CRASHES getattrstring(NULL)
 
     def test_sys_getoptionalattr(self):
-        sys_getattr = _testcapi.sys_getoptionalattr
+        getoptionalattr = _testcapi.sys_getoptionalattr
 
-        self.assertIs(sys_getattr('stdout'), sys.stdout)
+        self.assertIs(getoptionalattr('stdout'), sys.stdout)
         with support.swap_attr(sys, '\U0001f40d', 42):
-            self.assertEqual(sys_getattr('\U0001f40d'), 42)
+            self.assertEqual(getoptionalattr('\U0001f40d'), 42)
 
-        self.assertIs(sys_getattr('nonexisting'), AttributeError)
-        self.assertIs(sys_getattr(1), AttributeError)
-        self.assertRaises(TypeError, sys_getattr, [])
-        # CRASHES sys_getattr(NULL)
+        self.assertIs(getoptionalattr('nonexisting'), AttributeError)
+        self.assertIs(getoptionalattr(1), AttributeError)
+        self.assertRaises(TypeError, getoptionalattr, [])
+        # CRASHES getoptionalattr(NULL)
 
     def test_sys_getoptionalattrstring(self):
-        sys_getattr = _testcapi.sys_getoptionalattrstring
+        getoptionalattrstring = _testcapi.sys_getoptionalattrstring
 
-        self.assertIs(sys_getattr(b'stdout'), sys.stdout)
+        self.assertIs(getoptionalattrstring(b'stdout'), sys.stdout)
         with support.swap_attr(sys, '\U0001f40d', 42):
-            self.assertEqual(sys_getattr('\U0001f40d'.encode()), 42)
+            self.assertEqual(getoptionalattrstring('\U0001f40d'.encode()), 42)
 
-        self.assertIs(sys_getattr(b'nonexisting'), AttributeError)
-        self.assertRaises(UnicodeDecodeError, sys_getattr, b'\xff')
-        # CRASHES sys_getattr(NULL)
+        self.assertIs(getoptionalattrstring(b'nonexisting'), AttributeError)
+        self.assertRaises(UnicodeDecodeError, getoptionalattrstring, b'\xff')
+        # CRASHES getoptionalattrstring(NULL)
 
     def test_sys_getobject(self):
         getobject = _testcapi.sys_getobject

Modules/_testcapimodule.c

@@ -3235,15 +3235,7 @@ static PyObject *
 sys_getattr(PyObject *Py_UNUSED(module), PyObject *name)
 {
     NULLABLE(name);
-    PyObject *result = PySys_GetAttr(name);
-    if (result == NULL && PyErr_Occurred()) {
-        return NULL;
-    }
-    if (result == NULL) {
-        result = PyExc_AttributeError;
-        Py_INCREF(PyExc_AttributeError);
-    }
-    return result;
+    return PySys_GetAttr(name);
 }
 
 static PyObject *
@@ -3254,15 +3246,7 @@ sys_getattrstring(PyObject *Py_UNUSED(module), PyObject *arg)
     if (!PyArg_Parse(arg, "z#", &name, &size)) {
         return NULL;
     }
-    PyObject *result = PySys_GetAttrString(name);
-    if (result == NULL && PyErr_Occurred()) {
-        return NULL;
-    }
-    if (result == NULL) {
-        result = PyExc_AttributeError;
-        Py_INCREF(PyExc_AttributeError);
-    }
-    return result;
+    return PySys_GetAttrString(name);
 }
 
 static PyObject *
@@ -3274,6 +3258,7 @@ sys_getoptionalattr(PyObject *Py_UNUSED(module), PyObject *name)
     switch (PySys_GetOptionalAttr(name, &value)) {
         case -1:
             assert(value == NULL);
+            assert(PyErr_Occurred());
             return NULL;
         case 0:
             assert(value == NULL);
@@ -3282,7 +3267,6 @@ sys_getoptionalattr(PyObject *Py_UNUSED(module), PyObject *name)
             return value;
         default:
             Py_FatalError("PySys_GetOptionalAttr() returned invalid code");
-            Py_UNREACHABLE();
     }
 }
 
@@ -3299,6 +3283,7 @@ sys_getoptionalattrstring(PyObject *Py_UNUSED(module), PyObject *arg)
     switch (PySys_GetOptionalAttrString(name, &value)) {
         case -1:
             assert(value == NULL);
+            assert(PyErr_Occurred());
             return NULL;
         case 0:
             assert(value == NULL);
@@ -3307,7 +3292,6 @@ sys_getoptionalattrstring(PyObject *Py_UNUSED(module), PyObject *arg)
             return value;
         default:
             Py_FatalError("PySys_GetOptionalAttrString() returned invalid code");
-            Py_UNREACHABLE();
     }
 }