Merge pull request rapidsai#616 from teju85/enh-ext-clang-format

[REVIEW] Enable clang format

.gitignore

@@ -11,6 +11,7 @@ __pycache__
 *.pytest_cache
 htmlcov
 build/
+build_prims/
 cuml.egg-info/
 dist/
 python/cuml/**/*.cpp

BUILD.md

@@ -11,6 +11,7 @@ To install cuML from source, ensure the dependencies are met:
 5. Cython (>= 0.29)
 6. gcc (>=5.4.0)
 7. BLAS - Any BLAS compatible with cmake's [FindBLAS](https://cmake.org/cmake/help/v3.12/module/FindBLAS.html). Note that the blas has to be installed to the same folder system as cmake, for example if using conda installed cmake, the blas implementation should also be installed in the conda environment.
+8. clang-format (= 8.0.0) - enforces uniform C++ coding style; required to build cuML from source. The RAPIDS conda channel provides a package. If not using conda, install using your OS package manager.
 
 ## Installing from Source:
 

CHANGELOG.md

@@ -17,6 +17,7 @@
 - PR #596: Introduce cumlHandle for ols and ridge
 - PR #579: Introduce cumlHandle for cd and sgd, and propagate C++ errors in cython level for cd and sgd
 - PR #604: Adding cumlHandle to kNN, spectral methods, and UMAP
+- PR #616: Enable clang-format for enforcing coding style
 - PR #618: CI: Enable copyright header checks
 - PR #622: Updated to use 0.8 dependencies
 - PR #626: Added build.sh script, updated CI scripts and documentation

Dockerfile

@@ -7,6 +7,9 @@ ADD thirdparty /cuml/thirdparty
 ADD ml-prims /cuml/ml-prims
 ADD cuML /cuml/cuML
 WORKDIR /cuml/cuML
+RUN source activate ${CONDA_ENV} && \
+    conda install -c rapidsai libclang && \
+    conda clean -ya
 RUN source activate ${CONDA_ENV} && \
     mkdir build && \
     cd build && \

ci/gpu/build.sh

@@ -35,8 +35,7 @@ nvidia-smi
 
 logger "Activate conda env..."
 source activate gdf
-conda install -c rapidsai/label/cuda${CUDA_REL} -c rapidsai-nightly/label/cuda${CUDA_REL} cudf=${CUDF_VERSION} rmm=${RMM_VERSION} nvstrings=${NVSTRINGS_VERSION}
-conda install -c conda-forge lapack cmake==3.14.3 umap-learn
+conda install -c rapidsai/label/cuda${CUDA_REL} -c rapidsai-nightly/label/cuda${CUDA_REL} -c conda-forge -c rapidsai cudf=${CUDF_VERSION} rmm=${RMM_VERSION} nvstrings=${NVSTRINGS_VERSION} lapack cmake==3.14.3 umap-learn libclang
 
 logger "Check versions..."
 python --version

conda/recipes/libcuml/meta.yaml

@@ -30,6 +30,7 @@ requirements:
     - cudf 0.8*
     - cudatoolkit {{ cuda_version }}.*
     - lapack
+    - libclang
   run:
     - cudf 0.8*
     - {{ pin_compatible('cudatoolkit', max_pin='x.x') }}

cpp/.clang-format

@@ -0,0 +1,157 @@
+---
+# Refer to the following link for the explanation of each params:
+#   http://releases.llvm.org/8.0.0/tools/clang/docs/ClangFormatStyleOptions.html
+Language:        Cpp
+# BasedOnStyle:  Google
+AccessModifierOffset: -1
+AlignAfterOpenBracket: Align
+AlignConsecutiveAssignments: false
+AlignConsecutiveDeclarations: false
+AlignEscapedNewlines: Left
+AlignOperands:   true
+AlignTrailingComments: true
+AllowAllParametersOfDeclarationOnNextLine: true
+AllowShortBlocksOnASingleLine: false
+AllowShortCaseLabelsOnASingleLine: false
+AllowShortFunctionsOnASingleLine: All
+AllowShortIfStatementsOnASingleLine: true
+AllowShortLoopsOnASingleLine: true
+# This is deprecated
+AlwaysBreakAfterDefinitionReturnType: None
+AlwaysBreakAfterReturnType: None
+AlwaysBreakBeforeMultilineStrings: true
+AlwaysBreakTemplateDeclarations: Yes
+BinPackArguments: true
+BinPackParameters: true
+BraceWrapping:
+  AfterClass:      false
+  AfterControlStatement: false
+  AfterEnum:       false
+  AfterFunction:   false
+  AfterNamespace:  false
+  AfterObjCDeclaration: false
+  AfterStruct:     false
+  AfterUnion:      false
+  AfterExternBlock: false
+  BeforeCatch:     false
+  BeforeElse:      false
+  IndentBraces:    false
+  # disabling the below splits, else, they'll just add to the vertical length of source files!
+  SplitEmptyFunction: false
+  SplitEmptyRecord: false
+  SplitEmptyNamespace: false
+BreakBeforeBinaryOperators: None
+BreakBeforeBraces: Attach
+BreakBeforeInheritanceComma: false
+BreakInheritanceList: BeforeColon
+BreakBeforeTernaryOperators: true
+BreakConstructorInitializersBeforeComma: false
+BreakConstructorInitializers: BeforeColon
+BreakAfterJavaFieldAnnotations: false
+BreakStringLiterals: true
+ColumnLimit:     80
+CommentPragmas:  '^ IWYU pragma:'
+CompactNamespaces: false
+ConstructorInitializerAllOnOneLineOrOnePerLine: true
+# Kept the below 2 to be the same as `IndentWidth` to keep everything uniform
+ConstructorInitializerIndentWidth: 2
+ContinuationIndentWidth: 2
+Cpp11BracedListStyle: true
+DerivePointerAlignment: true
+DisableFormat:   false
+ExperimentalAutoDetectBinPacking: false
+FixNamespaceComments: true
+ForEachMacros:   
+  - foreach
+  - Q_FOREACH
+  - BOOST_FOREACH
+IncludeBlocks:   Preserve
+IncludeCategories: 
+  - Regex:           '^<ext/.*\.h>'
+    Priority:        2
+  - Regex:           '^<.*\.h>'
+    Priority:        1
+  - Regex:           '^<.*'
+    Priority:        2
+  - Regex:           '.*'
+    Priority:        3
+IncludeIsMainRegex: '([-_](test|unittest))?$'
+IndentCaseLabels: true
+IndentPPDirectives: None
+IndentWidth:     2
+IndentWrappedFunctionNames: false
+JavaScriptQuotes: Leave
+JavaScriptWrapImports: true
+KeepEmptyLinesAtTheStartOfBlocks: false
+MacroBlockBegin: ''
+MacroBlockEnd:   ''
+MaxEmptyLinesToKeep: 1
+NamespaceIndentation: None
+ObjCBinPackProtocolList: Never
+ObjCBlockIndentWidth: 2
+ObjCSpaceAfterProperty: false
+ObjCSpaceBeforeProtocolList: true
+PenaltyBreakAssignment: 2
+PenaltyBreakBeforeFirstCallParameter: 1
+PenaltyBreakComment: 300
+PenaltyBreakFirstLessLess: 120
+PenaltyBreakString: 1000
+PenaltyBreakTemplateDeclaration: 10
+PenaltyExcessCharacter: 1000000
+PenaltyReturnTypeOnItsOwnLine: 200
+PointerAlignment: Left
+RawStringFormats: 
+  - Language:        Cpp
+    Delimiters:      
+      - cc
+      - CC
+      - cpp
+      - Cpp
+      - CPP
+      - 'c++'
+      - 'C++'
+    CanonicalDelimiter: ''
+  - Language:        TextProto
+    Delimiters:
+      - pb
+      - PB
+      - proto
+      - PROTO
+    EnclosingFunctions:
+      - EqualsProto
+      - EquivToProto
+      - PARSE_PARTIAL_TEXT_PROTO
+      - PARSE_TEST_PROTO
+      - PARSE_TEXT_PROTO
+      - ParseTextOrDie
+      - ParseTextProtoOrDie
+    CanonicalDelimiter: ''
+    BasedOnStyle:    google
+# Enabling comment reflow causes doxygen comments to be messed up in their formats!
+ReflowComments:  false
+SortIncludes:    true
+SortUsingDeclarations: true
+SpaceAfterCStyleCast: false
+SpaceAfterTemplateKeyword: true
+SpaceBeforeAssignmentOperators: true
+SpaceBeforeCpp11BracedList: false
+SpaceBeforeCtorInitializerColon: true
+SpaceBeforeInheritanceColon: true
+SpaceBeforeParens: ControlStatements
+SpaceBeforeRangeBasedForLoopColon: true
+SpaceInEmptyParentheses: false
+SpacesBeforeTrailingComments: 2
+SpacesInAngles:  false
+SpacesInContainerLiterals: true
+SpacesInCStyleCastParentheses: false
+SpacesInParentheses: false
+SpacesInSquareBrackets: false
+# Being explicit here, since we are anyways C++11 (and soon to be C++14)
+Standard:        Cpp11
+StatementMacros: 
+  - Q_UNUSED
+  - QT_REQUIRE_VERSION
+# Be consistent with indent-width, even for people who use tab for indentation!
+TabWidth:        2
+UseTab:          Never
+...

cpp/CMakeLists.txt

@@ -107,6 +107,11 @@ else()
   message(STATUS "Manually setting BLAS to ${BLAS_LIBRARIES}")
 endif()
 
+set(CMAKE_MODULE_PATH ${PROJECT_SOURCE_DIR}/cmake)
+find_package(ClangFormat 8.0.0 EXACT REQUIRED)
+# TODO: enable this when we are ready!
+#find_package(ClangTidy 8.0.0 EXACT REQUIRED)
+
 ###################################################################################################
 # - External Dependencies--------------------------------------------------------------------------
 
@@ -192,6 +197,13 @@ if(CMAKE_COMPILER_IS_GNUCXX)
   endif(NOT CMAKE_CXX11_ABI)
 endif(CMAKE_COMPILER_IS_GNUCXX)
 
+###################################################################################################
+# - clang-format targets --------------------------------------------------------------------------
+
+add_clang_format(
+  DSTDIR ${PROJECT_BINARY_DIR}/clang-format
+  SRCDIR ${PROJECT_SOURCE_DIR})
+
 ###################################################################################################
 # - FAISS Build  ----------------------------------------------------------------------------------
 
@@ -309,6 +321,7 @@ if(BUILD_CUML_CPP_LIBRARY)
   endif(OPENMP_FOUND)
 
   target_link_libraries(${CUML_CPP_TARGET} ${CUML_LINK_LIBRARIES})
+  add_dependencies(${CUML_CPP_TARGET} ${ClangFormat_TARGET})
 
 endif(BUILD_CUML_CPP_LIBRARY)
 
@@ -350,4 +363,3 @@ install(TARGETS ${CUML_CPP_TARGET} DESTINATION lib)
 # add_doxygen_target(IN_DOXYFILE src_prims/Doxyfile.in
 #   OUT_DOXYFILE ${CMAKE_CURRENT_BINARY_DIR}/Doxyfile
 #   CWD ${CMAKE_CURRENT_BINARY_DIR})
-

cpp/DEVELOPER_GUIDE.md

@@ -11,6 +11,26 @@ The implementation of cuML is single threaded.
 
 ## Coding style
 
+## Code format
+### Introduction
+cuML relies on `clang-format` to enforce code style across all C++ and CUDA source code. The coding style is based on the [Google style guide](https://google.github.io/styleguide/cppguide.html#Formatting). The only digressions from this style are the following.
+1. Do not split empty functions/records/namespaces.
+2. Two-space indentation everywhere, including the line continuations.
+3. Disable reflowing of comments.
+The reasons behind these deviations from the Google style guide are given in comments [here](./.clang-format).
+
+### How is the check done?
+[run-clang-format.py](scripts/run-clang-format.py) is run first by `make`. This script runs clang-format only on modified files. An error is raised if the code diverges from the format suggested by clang-format, and the build fails.
+
+### How to know the formatting violations?
+When there are formatting errors, `run-clang-format.py` prints a `diff` command, showing where there are formatting differences. Unfortunately, unlike `flake8`, `clang-format` does NOT print descriptions of the violations, but instead directly formats the code. So, the only way currently to know why there are formatting differences is to run the diff command as suggested by this script against each violating source file.
+
+### How to fix the formatting violations?
+When there are formatting violations, `run-clang-format.py` prints an `-inplace` command you can use to automatically fix formatting errors. This is the easiest way to fix formatting errors. [This screencast](https://asciinema.org/a/248215) shows a typical build-fix-build cycle during cuML development.
+
+### clang-format version?
+To avoid spurious code style violations we specify the exact clang-format version required, currently `8.0.0`. This is enforced by a CMake check for the required version. [See here for more details on the dependencies](./README.md#dependencies).
+
 ## Error handling
 Call CUDA APIs via the provided helper macros `CUDA_CHECK`, `CUBLAS_CHECK` and `CUSOLVER_CHECK`. These macros take care of checking the return values of the used API calls and generate an exception when the command is not successful. If you need to avoid an exception, e.g. inside a destructor, use `CUDA_CHECK_NO_THROW`, `CUBLAS_CHECK_NO_THROW ` and `CUSOLVER_CHECK_NO_THROW ` (currently not available, see https://github.com/rapidsai/cuml/issues/229). These macros log the error but do not throw an exception.
 

cpp/README.md

@@ -19,6 +19,7 @@ The test folder has subfolders that reflect this distinction between the compone
 3. CUDA (>= 9.2)
 4. gcc (>=5.4.0)
 5. BLAS - Any BLAS compatible with cmake's [FindBLAS](https://cmake.org/cmake/help/v3.12/module/FindBLAS.html). Note that the blas has to be installed to the same folder system as cmake, for example if using conda installed cmake, the blas implementation should also be installed in the conda environment.
+6. clang-format (= 8.0.0) - enforces uniform C++ coding style; required to build cuML from source. The RAPIDS conda channel provides a package. If not using conda, install using your OS package manager.
 
 ### Building cuML:
 

cpp/cmake/FindClangFormat.cmake

@@ -13,52 +13,64 @@
 # limitations under the License.
 #
 
-# Finds clang-tidy exe based on the PATH env variable
+# Finds clang-format exe based on the PATH env variable
 string(REPLACE ":" ";" EnvPath $ENV{PATH})
 find_program(ClangFormat_EXE
   NAMES clang-format
   PATHS EnvPath
-  DOC "path to clang-format exe"
-  NO_DEFAULT_PATH)
+  DOC "path to clang-format exe")
 find_program(ClangFormat_PY
   NAMES run-clang-format.py
-  PATHS ${MLPRIMS_DIR}/scripts
+  PATHS ${PROJECT_SOURCE_DIR}/scripts
   DOC "path to run-clang-format python script")
+
+# Figure out the version of clang-format, if found
+if(ClangFormat_EXE)
+  execute_process(COMMAND ${ClangFormat_EXE} --version
+    OUTPUT_VARIABLE __cf_version_out
+    OUTPUT_STRIP_TRAILING_WHITESPACE)
+  string(REGEX REPLACE
+    "^clang-format version ([0-9.-]+).*$" "\\1"
+    ClangFormat_VERSION_STRING
+    "${__cf_version_out}")
+endif()
+
 include(FindPackageHandleStandardArgs)
-find_package_handle_standard_args(ClangFormat DEFAULT_MSG
-  ClangFormat_EXE ClangFormat_PY)
+find_package_handle_standard_args(ClangFormat
+  REQUIRED_VARS ClangFormat_EXE ClangFormat_PY
+  VERSION_VAR ClangFormat_VERSION_STRING)
 
 include(CMakeParseArguments)
 
+set(ClangFormat_TARGET format)
+
 # clang formatting as a target in the final build stage
 function(add_clang_format)
   if(ClangFormat_FOUND)
     set(options "")
-    set(oneValueArgs "")
-    set(multiValueArgs TARGETS SRCS)
+    set(oneValueArgs DSTDIR SRCDIR)
+    set(multiValueArgs "")
     cmake_parse_arguments(cf "${options}" "${oneValueArgs}" "${multiValueArgs}" ${ARGN})
-    foreach(cf_TARGET ${cf_TARGETS})
-      if(NOT TARGET ${cf_TARGET})
-        message(FATAL_ERROR "add_clang_format: '${cf_TARGET}' is not a target")
-      endif()
-    endforeach()
-    set(dummy_file clang_format_output)
-    add_custom_command(OUTPUT ${dummy_file}
-      COMMENT "Clang-Format ${cf_TARGET}"
+    # to flag violations
+    add_custom_target(${ClangFormat_TARGET}
+      ALL
       COMMAND python
         ${ClangFormat_PY}
-        -bindir ${CMAKE_SOURCE_DIR}
+          -dstdir ${cf_DSTDIR}
+          -exe ${ClangFormat_EXE}
+          -onlyChangedFiles
+      COMMENT "Run clang-format on the cpp source files"
+      WORKING_DIRECTORY ${cf_SRCDIR})
+    # to fix the flagged violations (only to be run locally!)
+    add_custom_target(fix-${ClangFormat_TARGET}
+      COMMAND python
+      ${ClangFormat_PY}
+        -dstdir ${cf_DSTDIR}
         -exe ${ClangFormat_EXE}
-        -srcdir ${CMAKE_SOURCE_DIR} ${cf_SRCS})
-    # add the dependency on this dummy target
-    # So, if the main source file has been modified, clang-format will
-    # automatically be run on it!
-    add_custom_target(clang_format
-      SOURCES ${dummy_file}
-      COMMENT "Clang-Format for target ${_target}")
-    foreach(cf_TARGET ${cf_TARGETS})
-      add_dependencies(${cf_TARGET} clang_format)
-    endforeach()
+        -onlyChangedFiles
+        -inplace
+      COMMENT "Run the inplace fix for clang-format flagged violations"
+      WORKING_DIRECTORY ${cf_SRCDIR})
   else()
     message("add_clang_format: clang-format exe not found")
   endif()

cpp/examples/dbscan/CMakeLists.txt

@@ -17,4 +17,5 @@
 include_directories(${CUDA_INCLUDE_DIRS})
 
 add_executable(dbscan_example dbscan_example.cpp)
+add_dependencies(dbscan_example ${ClangFormat_TARGET})
 target_link_libraries(dbscan_example cuml++)

cpp/examples/kmeans/CMakeLists.txt

@@ -16,4 +16,5 @@
 
 include_directories(${CUDA_INCLUDE_DIRS})
 add_executable(kmeans_example kmeans_example.cpp)
+add_dependencies(kmeans_example ${ClangFormat_TARGET})
 target_link_libraries(kmeans_example cuml++)