TriBITSPub / TriBITS

TriBITS: Tribal Build, Integrate, and Test System,
http://tribits.org
Other
36 stars 46 forks source link

Convert rest of upper-case to lower-case CMake command calls (#480) #499

Closed bartlettroscoe closed 2 years ago

bartlettroscoe commented 2 years ago

Some of these flagged by @KyleFromKitware in review of #480. The rest I found by running:

  $ find tribits test -type f \
    -exec grep -l "[^a-zA-Z0-9_][A-Z][A-Z0-9_]*(" {} \;
KyleFromKitware commented 2 years ago

In the long run, it might be worth considering adding a CI test to enforce naming conventions so that these don't creep in.

+1

bartlettroscoe commented 2 years ago

In the long run, it might be worth considering adding a CI test to enforce naming conventions so that these don't creep in.

+1

@KyleFromKitware, that problem is that there is some text in CMake files that violates that because it is actually not CMake code but is generated code for another language. For example:

$ grep -nH "[^a-zA-Z0-9_-][A-Z][A-Z0-9_]*(" tribits/core/package_arch/TribitsConfigureFile.cmake 
tribits/core/package_arch/TribitsConfigureFile.cmake:162:      "     TEUCHOS_FUNC_TIME_MONITOR_DIFF(FUNCNAME, ${PARENT_PACKAGE_NAME_UC})\n"
tribits/core/package_arch/TribitsConfigureFile.cmake:164:      "     TEUCHOS_FUNC_TIME_MONITOR_DIFF(FUNCNAME, DIFF)\n"

Those are C++ macros that we want to be UPPERCASE because that is a good convention for C++ macros. But that is code created in a CMake file.

Now, we could get around this using a variable and then setting:

  set(timeMonitorDiff "TEUCHOS_FUNC_TIME_MONITOR_DIFF")
  string(APPEND configFileFragString
    "    ${timeMonitorDiff}(FUNCNAME, ...)"
    "    ${timeMonitorDiff}(FUNCNAME, DIFF)"
    )

that would not fail the test.

Also, this is not used very often as shown by:

$ find tribits test \( -name "CMakeLists.txt" -or -name "*.cmake*" \) \
    -exec grep -nH "[^a-zA-Z0-9_-][A-Z][A-Z0-9_]*(" {} \;
tribits/core/package_arch/TribitsConfigureFile.cmake:162:      "     TEUCHOS_FUNC_TIME_MONITOR_DIFF(FUNCNAME, ${PARENT_PACKAGE_NAME_UC})\n"
tribits/core/package_arch/TribitsConfigureFile.cmake:164:      "     TEUCHOS_FUNC_TIME_MONITOR_DIFF(FUNCNAME, DIFF)\n"
test/python_utils/extract_rst_cmake_doc/simpleDocText2.cmake:7:# @FUNCTION: SOME_FUNC_NAME2()
test/python_utils/extract_rst_cmake_doc/simpleDocText2.cmake:12:#   SOME_FUNC_NAME2(blah

so perhaps it is worth it to refactor this code using the variable-substitution trick above to pass the test?

KyleFromKitware commented 2 years ago

Alternatively, you might implement something more complex than just grep that actually lexes the CMake code and picks out the function names. We've had some interest in an open source CMake parser, which might become a reality at some point, allowing applications such as this.

bartlettroscoe commented 2 years ago

Alternatively, you might implement something more complex than just grep that actually lexes the CMake code and picks out the function names. We've had some interest in an open source CMake parser, which might become a reality at some point, allowing applications such as this.

@KyleFromKitware, it would be nice to have an open source CMake parser that we could use to create tools. For example, a code coverage tool for CMake code would be nice.