diff --git a/qrenderdoc/Code/pyrenderdoc/PythonContext.cpp b/qrenderdoc/Code/pyrenderdoc/PythonContext.cpp index a6ab84609..a0afc5139 100644 --- a/qrenderdoc/Code/pyrenderdoc/PythonContext.cpp +++ b/qrenderdoc/Code/pyrenderdoc/PythonContext.cpp @@ -58,9 +58,9 @@ PyTypeObject **SbkPySide2_QtWidgetsTypes = NULL; #include "Code/QRDUtils.h" #include "PythonContext.h" -// exported by generated files, used to check docstrings in interfaces -bool CheckCoreDocstrings(); -bool CheckQtDocstrings(); +// exported by generated files, used to check interface compliance +bool CheckCoreInterface(); +bool CheckQtInterface(); // defined in SWIG-generated renderdoc_python.cpp extern "C" PyObject *PyInit__renderdoc(void); @@ -443,13 +443,13 @@ PythonContext::~PythonContext() outputTick(); } -bool PythonContext::CheckDocstrings() +bool PythonContext::CheckInterfaces() { bool errors = false; PyGILState_STATE gil = PyGILState_Ensure(); - errors |= CheckCoreDocstrings(); - errors |= CheckQtDocstrings(); + errors |= CheckCoreInterface(); + errors |= CheckQtInterface(); PyGILState_Release(gil); return errors; diff --git a/qrenderdoc/Code/pyrenderdoc/PythonContext.h b/qrenderdoc/Code/pyrenderdoc/PythonContext.h index e62415f86..6001f5a58 100644 --- a/qrenderdoc/Code/pyrenderdoc/PythonContext.h +++ b/qrenderdoc/Code/pyrenderdoc/PythonContext.h @@ -55,7 +55,7 @@ public: static void GlobalInit(); static void GlobalShutdown(); - bool CheckDocstrings(); + bool CheckInterfaces(); QString versionString(); diff --git a/qrenderdoc/Code/pyrenderdoc/document_check.h b/qrenderdoc/Code/pyrenderdoc/interface_check.h similarity index 55% rename from qrenderdoc/Code/pyrenderdoc/document_check.h rename to qrenderdoc/Code/pyrenderdoc/interface_check.h index bc7a2d279..5a37a591d 100644 --- a/qrenderdoc/Code/pyrenderdoc/document_check.h +++ b/qrenderdoc/Code/pyrenderdoc/interface_check.h @@ -24,7 +24,101 @@ #pragma once -inline bool check_docstrings(swig_type_info **swig_types, size_t numTypes) +// verify the interface. +// We check that docstrings aren't duplicated, which is a symptom of missing DOCUMENT() +// macros around newly added classes/members. +// For enums, verify that all constants are documented in the parent docstring +// Generally we ensure naming is roughly OK: +// * types, member functions, and enum values must match the regexp /[A-Z][a-zA-Z0-9]+/ +// ie. we don't use underscore_seperated_words or mixedCase / camelCase. +// * data members should be mixedCase / camelCase. So matching /[a-z][a-zA-Z0-9]+/ +// This isn't quite python standards but it fits best with the C++ code and the important +// thing is that it's self-consistent. + +enum class NameType +{ + Type, + EnumValue, + Method, + Member, +}; + +inline bool checkname(const char *baseType, std::string name, NameType nameType) +{ + // skip __ prefixed names + if(name.length() > 2 && name[0] == '_' && name[1] == '_') + return false; + + // skip any rdctype based types that are converted into equivalent python types + if((baseType && strstr(baseType, "rdcarray")) || name.find("rdcarray") != std::string::npos) + return false; + if((baseType && strstr(baseType, "bytebuf")) || name.find("bytebuf") != std::string::npos) + return false; + if((baseType && strstr(baseType, "rdcstr")) || name.find("rdcstr") != std::string::npos) + return false; + if((baseType && strstr(baseType, "StructuredBufferList")) || + name.find("StructuredBufferList") != std::string::npos) + return false; + if((baseType && strstr(baseType, "StructuredChunkList")) || + name.find("StructuredChunkList") != std::string::npos) + return false; + if((baseType && strstr(baseType, "StructuredObjectList")) || + name.find("StructuredObjectList") != std::string::npos) + return false; + + // allow the config to have different names + if((baseType && strstr(baseType, "PersistantConfig")) || + name.find("PersistantConfig") != std::string::npos) + return false; + + // skip swig internal type + if((baseType && strstr(baseType, "SwigPyObject")) || name.find("SwigPyObject") != std::string::npos) + return false; + + // remove the module prefix, if this is a type name we're checking + if(!strncmp(name.c_str(), "renderdoc.", 10)) + name.erase(0, 10); + if(!strncmp(name.c_str(), "qrenderdoc.", 11)) + name.erase(0, 11); + + // skip a few well-known members + if(name == "this" || name == "thisown") + return false; + + bool member = (nameType == NameType::Member); + + // look for invalid name + bool badfirstChar = false; + if(member) + badfirstChar = name[0] < 'a' || name[0] > 'z'; + else + badfirstChar = name[0] < 'A' || name[0] > 'Z'; + + if(badfirstChar || name.find('_') != std::string::npos) + { + const char *nameTypeStr = ""; + + switch(nameType) + { + case NameType::EnumValue: nameTypeStr = "enum value"; break; + case NameType::Member: nameTypeStr = "member variable"; break; + case NameType::Method: nameTypeStr = "method"; break; + case NameType::Type: nameTypeStr = "type"; break; + } + + snprintf(convert_error, sizeof(convert_error) - 1, + "Name of %s '%s.%s' does not match naming scheme.\n" + "Should start with %s letter and not contain underscores", + nameTypeStr, baseType, name.c_str(), member ? "lowercase" : "uppercase"); + RENDERDOC_LogMessage(LogType::Error, "QTRD", __FILE__, __LINE__, convert_error); + + return true; + } + + return false; +} + +inline bool check_interface(swig_type_info **swig_types, size_t numTypes) { // track all errors and fatal error at the end, so we see all of the problems at once instead of // requiring rebuilds over and over. @@ -56,6 +150,8 @@ inline bool check_docstrings(swig_type_info **swig_types, size_t numTypes) errors_found = true; } + errors_found |= checkname("renderdoc", typeobj->tp_name, NameType::Type); + PyObject *dict = typeobj->tp_dict; // check the object's dict to see if this is an enum (or struct with constants). @@ -74,8 +170,8 @@ inline bool check_docstrings(swig_type_info **swig_types, size_t numTypes) PyObject *key = PyList_GetItem(keys, i); PyObject *value = PyDict_GetItem(dict, key); - // if this key is a string (it should be) and the value is an integer, it's a constant - if(PyUnicode_Check(key) && PyLong_Check(value)) + // if this key is a string (it should be) + if(PyUnicode_Check(key)) { char *str = NULL; Py_ssize_t len = 0; @@ -84,12 +180,27 @@ inline bool check_docstrings(swig_type_info **swig_types, size_t numTypes) if(str == NULL || len == 0) { + snprintf(convert_error, sizeof(convert_error) - 1, + "Couldn't get member name for %i'th member of '%s'", (int)i, typeobj->tp_name); RENDERDOC_LogMessage(LogType::Error, "QTRD", __FILE__, __LINE__, convert_error); errors_found = true; } else { - constants.insert(std::string(str, str + len)); + std::string name(str, str + len); + + NameType nameType = NameType::Member; + + // if the value is an integer, it's a constant + if(PyLong_Check(value)) + { + constants.insert(name); + nameType = NameType::EnumValue; + } + + // if it's a callable it's a method, ignore it + if(!PyCallable_Check(value) && !PyType_IsSubtype(value->ob_type, &PyStaticMethod_Type)) + errors_found |= checkname(typeobj->tp_name, name, nameType); } Py_DecRef(bytes); @@ -150,6 +261,8 @@ inline bool check_docstrings(swig_type_info **swig_types, size_t numTypes) { std::string method_doc = method->ml_doc; + checkname(typeobj->tp_name, method->ml_name, NameType::Method); + size_t i = 0; while(method_doc[i] == '\n') i++; diff --git a/qrenderdoc/Code/pyrenderdoc/qrenderdoc.i b/qrenderdoc/Code/pyrenderdoc/qrenderdoc.i index be9de6866..e3431c3ee 100644 --- a/qrenderdoc/Code/pyrenderdoc/qrenderdoc.i +++ b/qrenderdoc/Code/pyrenderdoc/qrenderdoc.i @@ -113,30 +113,28 @@ TEMPLATE_ARRAY_INSTANTIATE_PTR(rdcarray, ICaptureViewer) %header %{ #include - #include "Code/pyrenderdoc/document_check.h" + #include "Code/pyrenderdoc/interface_check.h" - // verify that docstrings aren't duplicated, which is a symptom of missing DOCUMENT() - // macros around newly added classes/members. - // For enums, verify that all constants are documented in the parent docstring - static swig_type_info **docCheckTypes; - static size_t docCheckNumTypes = 0; + // check interface, see interface_check.h for more information + static swig_type_info **interfaceCheckTypes; + static size_t interfaceCheckNumTypes = 0; - bool CheckQtDocstrings() + bool CheckQtInterface() { #if defined(RELEASE) return false; #else - if(docCheckNumTypes == 0) + if(interfaceCheckNumTypes == 0) return false; - return check_docstrings(docCheckTypes, docCheckNumTypes); + return check_interface(interfaceCheckTypes, interfaceCheckNumTypes); #endif } %} %init %{ - docCheckTypes = swig_type_initial; - docCheckNumTypes = sizeof(swig_type_initial)/sizeof(swig_type_initial[0]); + interfaceCheckTypes = swig_type_initial; + interfaceCheckNumTypes = sizeof(swig_type_initial)/sizeof(swig_type_initial[0]); %} // declare functions for using swig opaque wrap/unwrap of QWidget, for when pyside isn't available. diff --git a/qrenderdoc/Code/pyrenderdoc/renderdoc.i b/qrenderdoc/Code/pyrenderdoc/renderdoc.i index dbb832203..ef0f1bc42 100644 --- a/qrenderdoc/Code/pyrenderdoc/renderdoc.i +++ b/qrenderdoc/Code/pyrenderdoc/renderdoc.i @@ -278,28 +278,26 @@ PyObject *PassObjectToPython(const char *type, void *obj) %header %{ #include - #include "Code/pyrenderdoc/document_check.h" + #include "Code/pyrenderdoc/interface_check.h" - // verify that docstrings aren't duplicated, which is a symptom of missing DOCUMENT() - // macros around newly added classes/members. - // For enums, verify that all constants are documented in the parent docstring - static swig_type_info **docCheckTypes; - static size_t docCheckNumTypes = 0; + // check interface, see interface_check.h for more information + static swig_type_info **interfaceCheckTypes; + static size_t interfaceCheckNumTypes = 0; - bool CheckCoreDocstrings() + bool CheckCoreInterface() { #if defined(RELEASE) return false; #else - if(docCheckNumTypes == 0) + if(interfaceCheckNumTypes == 0) return false; - return check_docstrings(docCheckTypes, docCheckNumTypes); + return check_interface(interfaceCheckTypes, interfaceCheckNumTypes); #endif } %} %init %{ - docCheckTypes = swig_type_initial; - docCheckNumTypes = sizeof(swig_type_initial)/sizeof(swig_type_initial[0]); + interfaceCheckTypes = swig_type_initial; + interfaceCheckNumTypes = sizeof(swig_type_initial)/sizeof(swig_type_initial[0]); %} diff --git a/qrenderdoc/Code/qrenderdoc.cpp b/qrenderdoc/Code/qrenderdoc.cpp index 318df7d61..4887b12e1 100644 --- a/qrenderdoc/Code/qrenderdoc.cpp +++ b/qrenderdoc/Code/qrenderdoc.cpp @@ -79,20 +79,20 @@ int main(int argc, char *argv[]) bool errors = false; - qInfo() << "Checking python binding docstrings."; + qInfo() << "Checking python binding consistency."; { PythonContextHandle py; - errors = py.ctx().CheckDocstrings(); + errors = py.ctx().CheckInterfaces(); } if(errors) { - qCritical() << "Found errors in python binding docstrings. Please fix!"; + qCritical() << "Found errors in python bindings. Please fix!"; return 1; } - qInfo() << "Python binding docstrings are consistent."; + qInfo() << "Python bindings are consistent."; return 0; } } diff --git a/qrenderdoc/qrenderdoc.pro b/qrenderdoc/qrenderdoc.pro index 8353852e8..4857abe0b 100644 --- a/qrenderdoc/qrenderdoc.pro +++ b/qrenderdoc/qrenderdoc.pro @@ -234,7 +234,7 @@ HEADERS += Code/CaptureContext.h \ Code/Resources.h \ Code/pyrenderdoc/PythonContext.h \ Code/pyrenderdoc/pyconversion.h \ - Code/pyrenderdoc/document_check.h \ + Code/pyrenderdoc/interface_check.h \ Code/Interface/QRDInterface.h \ Code/Interface/Analytics.h \ Code/Interface/CommonPipelineState.h \ diff --git a/qrenderdoc/qrenderdoc_local.vcxproj b/qrenderdoc/qrenderdoc_local.vcxproj index dfb8c999a..7eb64bd6e 100644 --- a/qrenderdoc/qrenderdoc_local.vcxproj +++ b/qrenderdoc/qrenderdoc_local.vcxproj @@ -882,7 +882,7 @@ - + @@ -1297,7 +1297,7 @@ Document - %(Fullpath);container_handling.i;pyconversion.i;cosmetics.i;document_check.h;$(SolutionDir)renderdoc\api\replay\basic_types.h;$(SolutionDir)renderdoc\api\replay\capture_options.h;$(SolutionDir)renderdoc\api\replay\control_types.h;$(SolutionDir)renderdoc\api\replay\d3d11_pipestate.h;$(SolutionDir)renderdoc\api\replay\d3d12_pipestate.h;$(SolutionDir)renderdoc\api\replay\data_types.h;$(SolutionDir)renderdoc\api\replay\gl_pipestate.h;$(SolutionDir)renderdoc\api\replay\renderdoc_replay.h;$(SolutionDir)renderdoc\api\replay\replay_enums.h;$(SolutionDir)renderdoc\api\replay\shader_types.h;$(SolutionDir)renderdoc\api\replay\stringise.h;$(SolutionDir)renderdoc\api\replay\structured_data.h;$(SolutionDir)renderdoc\api\replay\vk_pipestate.h;%(AdditionalInputs) + %(Fullpath);container_handling.i;pyconversion.i;cosmetics.i;interface_check.h;$(SolutionDir)renderdoc\api\replay\basic_types.h;$(SolutionDir)renderdoc\api\replay\capture_options.h;$(SolutionDir)renderdoc\api\replay\control_types.h;$(SolutionDir)renderdoc\api\replay\d3d11_pipestate.h;$(SolutionDir)renderdoc\api\replay\d3d12_pipestate.h;$(SolutionDir)renderdoc\api\replay\data_types.h;$(SolutionDir)renderdoc\api\replay\gl_pipestate.h;$(SolutionDir)renderdoc\api\replay\renderdoc_replay.h;$(SolutionDir)renderdoc\api\replay\replay_enums.h;$(SolutionDir)renderdoc\api\replay\shader_types.h;$(SolutionDir)renderdoc\api\replay\stringise.h;$(SolutionDir)renderdoc\api\replay\structured_data.h;$(SolutionDir)renderdoc\api\replay\vk_pipestate.h;%(AdditionalInputs) "$(ProjectDir)3rdparty\swig\swig.exe" -v -Wextra -Werror -O -c++ -python -modern -modernargs -enumclass -fastunpack -py3 -builtin -I"$(SolutionDir)renderdoc\api\replay" -outdir "$(IntDir)generated" -o "$(IntDir)generated\%(Filename)_python.cxx" "%(FullPath)" Compiling SWIG interface $(IntDir)generated\%(Filename).py;$(IntDir)generated\%(Filename)_python.cxx;%(Outputs) diff --git a/qrenderdoc/qrenderdoc_local.vcxproj.filters b/qrenderdoc/qrenderdoc_local.vcxproj.filters index 77f4bb8ba..eadccba05 100644 --- a/qrenderdoc/qrenderdoc_local.vcxproj.filters +++ b/qrenderdoc/qrenderdoc_local.vcxproj.filters @@ -1004,9 +1004,6 @@ Code\Interface - - Code\pyrenderdoc - Code @@ -1058,6 +1055,9 @@ Generated Files + + Code\pyrenderdoc +