codereview.cmake

# **********************************************************
# Copyright (c) 2010 VMware, Inc.  All rights reserved.
# **********************************************************

# Dr. Memory: the memory debugger
#
# This library is free software; you can redistribute it and/or
# modify it under the terms of the GNU Lesser General Public
# License as published by the Free Software Foundation;
# version 2.1 of the License, and no later version.
#
# This library is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
# Library General Public License for more details.
#
# You should have received a copy of the GNU Lesser General Public
# License along with this library; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

# **********************************************************
# Copyright (c) 2009-2010 VMware, Inc.    All rights reserved.
# **********************************************************

# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are met:
#
# * Redistributions of source code must retain the above copyright notice,
#   this list of conditions and the following disclaimer.
#
# * Redistributions in binary form must reproduce the above copyright notice,
#   this list of conditions and the following disclaimer in the documentation
#   and/or other materials provided with the distribution.
#
# * Neither the name of VMware, Inc. nor the names of its contributors may be
#   used to endorse or promote products derived from this software without
#   specific prior written permission.
#
# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
# ARE DISCLAIMED. IN NO EVENT SHALL VMWARE, INC. OR CONTRIBUTORS BE LIABLE
# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
# DAMAGE.

###########################################################################
# Code Review Automation
# Based on DynamoRIO's script of the same name
#
# See CodeReviews.wiki for full discussion of our code review process.
# To summarize: reviewer commits to /reviews/<user>/<year>/i###-descr.diff
#   with a commit log that auto-opens an Issue
# Inline comments for a regular patch diff should be the norm for now
# FIXME: we should also produce web diffs for quick reviews
# FIXME: xref discussion on using Google Code's post-commit review options
#
# Usage: prepare a notes file with comments for the reviewer after a line
# beginning "toreview:", and point to it w/ the NOTES variable
# (defult is ./diff.notes)
#
# Since we no longer invoke "make" from the soure dir (due to cmake: i#19, i#64),
# we have to invoke this explicitly "cmake -P ./codereview.cmake".
#
# Parameter list:
#
#   REVIEWS:PATH    = Checkout of https://drmemory.googlecode.com/svn/reviews
#   NOTES:FILEPATH  = File containing notes for this review.
#                     It must contain the string label "toreview:".
#                     Content prior to the label is discarded (private notes).
#                     Content after the label is included in the review request.
#   LABEL:STRING    = Name for review.  Should follow "i###-descr" format where
#                     ### is the Issue number.  For example: "i64-cmake-review".
#   AUTHOR:STRING   = Username on https://drmemory.googlecode.com/svn
#   REVIEWER:STRING = Username on https://drmemory.googlecode.com/svn
#   REVERT:BOOL     = Set to ON to revert locally-added files
#   COMMIT:BOOL     = Set to ON to officially commit the review request
#                     (the default is to simply create and locally add the
#                     .diff and .notes files)
#
# Examples:
#
# Dry run to ensure diff and notes file are as desired:
#   > cmake -DAUTHOR:STRING=derek.bruening -DREVIEWER:STRING=qin.zhao -DLABEL:STRING=i64-cmake-review -P ./codereview.cmake
#   -- notes file is "diff.notes"
#   -- destination is "../reviews/derek.bruening/2009/i64-cmake-review.{diff,notes}"
#   -- svn: A         ../reviews/derek.bruening/2009/i64-cmake-review.diff
#   -- svn: A         ../reviews/derek.bruening/2009/i64-cmake-review.notes
#   -- ready to commit
#
# Want to abort (maybe decided to change LABEL) so undoing local svn add:
#   > cmake -DAUTHOR:STRING=derek.bruening -DREVIEWER:STRING=qin.zhao -DLABEL:STRING=i64-cmake-review -DREVERT:BOOL=ON -P ./codereview.cmake
#   -- notes file is "diff.notes"
#   -- destination is "../reviews/derek.bruening/2009/i64-cmake-review.{diff,notes}"
#   -- svn: D         ../reviews/derek.bruening/2009/i64-cmake-review.diff
#   -- svn: D         ../reviews/derek.bruening/2009/i64-cmake-review.notes
#   -- revert complete
#
# Ready to commit:
#   > cmake -DAUTHOR:STRING=derek.bruening -DREVIEWER:STRING=qin.zhao -DLABEL:STRING=i64-cmake-review -DCOMMIT:BOOL=ON -P ./codereview.cmake
#   -- notes file is "diff.notes"
#   -- destination is "../reviews/derek.bruening/2009/i64-cmake-review.{diff,notes}"
#   -- svn: A         ../reviews/derek.bruening/2009/i64-cmake-review.diff
#   -- svn: A         ../reviews/derek.bruening/2009/i64-cmake-review.notes
#   -- svn: Sending        derek.bruening/2009/i64-cmake-review.diff
#   -- svn: Sending        derek.bruening/2009/i64-cmake-review.notes
#   -- svn: Transmitting file data .
#   -- svn: Committed revision 75.
#   -- committed
#
# Note that you can make a cmake script that sets common variables and point at it
# with the -C parameter to cmake.

# FIXME i#66: should include pre-commit regression test suite results here

if (NOT DEFINED LABEL)
  message(FATAL_ERROR "must set LABEL variable")
endif (NOT DEFINED LABEL)

if (NOT DEFINED AUTHOR)
  message(FATAL_ERROR "must set AUTHOR variable")
endif (NOT DEFINED AUTHOR)

if (NOT DEFINED REVIEWER)
  message(FATAL_ERROR "must set REVIEWER variable")
endif (NOT DEFINED REVIEWER)

if (NOT DEFINED NOTES)
  set(NOTES diff.notes)
endif (NOT DEFINED NOTES)
if (NOT EXISTS ${NOTES})
  message(FATAL_ERROR "cannot find NOTES=\"${NOTES}\"")
endif (NOT EXISTS ${NOTES})
message(STATUS "notes file is \"${NOTES}\"")

if (NOT DEFINED REVIEWS)
  set(REVIEWS ../reviews)
endif (NOT DEFINED REVIEWS)
if (NOT EXISTS ${REVIEWS})
  message(FATAL_ERROR "cannot find REVIEWS=\"${REVIEWS}\"")
endif (NOT EXISTS ${REVIEWS})

find_program(SVN svn DOC "subversion client")
if (NOT SVN)
  message(FATAL_ERROR "svn not found")
endif (NOT SVN)
# we run svn in the review checkout
function(run_svn)
  execute_process(COMMAND ${SVN} ${ARGV}
    WORKING_DIRECTORY ${REVIEWS}
    RESULT_VARIABLE svn_result
    ERROR_VARIABLE svn_err
    OUTPUT_VARIABLE svn_out)
  if (svn_result OR svn_err)
    message(FATAL_ERROR "*** ${SVN} ${ARGV} failed: ***\n${svn_err}")
  endif (svn_result OR svn_err)
  string(STRIP "${svn_out}" svn_out)
  string(REGEX REPLACE "\r?\n" "\n-- svn: " svn_out "${svn_out}")
  message(STATUS "svn: ${svn_out}")
endfunction(run_svn)

find_program(DIFFSTAT diffstat DOC "diff statistics displayer")
if (NOT DIFFSTAT)
  message(STATUS "diffstat not found: will not have diff statistics")
endif (NOT DIFFSTAT)

# get the year
if (UNIX)
  find_program(DATE date DOC "unix date")
  if (NOT DATE)
    message(FATAL_ERROR "date not found")
  endif (NOT DATE)
  execute_process(COMMAND ${DATE} +%Y
    RESULT_VARIABLE date_result
    ERROR_VARIABLE date_err
    OUTPUT_VARIABLE year)
  if (date_result OR date_err)
    message(FATAL_ERROR "*** ${DATE} failed: ***\n${date_err}")
  endif (date_result OR date_err)
  string(STRIP "${year}" year)
else (UNIX)
  find_program(CMD cmd DOC "cmd shell")
  if (NOT CMD)
    message(FATAL_ERROR "cmd not found")
  endif (NOT CMD)
  # If use forward slashes => "The syntax of the command is incorrect."
  file(TO_NATIVE_PATH "${CMD}" CMD)
  execute_process(COMMAND ${CMD} /c date /T
    RESULT_VARIABLE date_result
    ERROR_VARIABLE date_err
    OUTPUT_VARIABLE date_out)
  if (date_result OR date_err)
    message(FATAL_ERROR "*** ${CMD} /c date /T failed: ***\n${date_err}")
  endif (date_result OR date_err)
  string(STRIP "${date_out}" date_out)
  # result should be like this: "Fri 03/06/2009"
  # cmake regexp doesn't have {N}
  string(REGEX REPLACE "^....../../([0-9][0-9][0-9][0-9])" "\\1" year "${date_out}")
endif (UNIX)

# we run from the REVIEWS dir so no need to include it here
set(DEST_BASE "${AUTHOR}/${year}")
set(DEST "${DEST_BASE}/${LABEL}")
message(STATUS "destination is \"${REVIEWS}/${DEST}.{diff,notes}\"")

if (REVERT)

  execute_process(COMMAND ${SVN} status ${DEST}.diff
    WORKING_DIRECTORY ${REVIEWS}
    RESULT_VARIABLE svn_result
    ERROR_VARIABLE svn_err
    OUTPUT_VARIABLE svn_out)
  if (svn_result OR svn_err)
    message(FATAL_ERROR "*** ${SVN} status failed: ***\n${svn_err}")
  endif (svn_result OR svn_err)
  if ("${svn_out}" MATCHES "^\\A")
    # svn revert doesn't delete local copy if new => svn delete
    # FIXME: if AUTHOR was mistyped should we remove the directories too?
    run_svn("delete;--force;${DEST}.diff;${DEST}.notes")
  else ("${svn_out}" MATCHES "^\\A")
    run_svn("revert;${DEST}.diff;${DEST}.notes")
  endif ("${svn_out}" MATCHES "^\\A")

  message(STATUS "revert complete")

else (REVERT)

  if (NOT EXISTS "${REVIEWS}/${DEST_BASE}")
    if (NOT EXISTS "${REVIEWS}/${AUTHOR}")
      run_svn("mkdir;${AUTHOR}")
    endif (NOT EXISTS "${REVIEWS}/${AUTHOR}")
    run_svn("mkdir;${DEST_BASE}")
  endif (NOT EXISTS "${REVIEWS}/${DEST_BASE}")

  set(NOTES_LOCAL "${DEST}.notes")
  set(DIFF_LOCAL "${DEST}.diff")
  set(NOTES_FILE "${REVIEWS}/${NOTES_LOCAL}")
  set(DIFF_FILE "${REVIEWS}/${DIFF_LOCAL}")

  # If someone manually created these files then this script
  # will fail: we assume not already there if haven't been
  # added to svn yet.
  if (NOT EXISTS "${DIFF_FILE}")
    file(WRITE "${DIFF_FILE}" "")
    run_svn("add;${DIFF_LOCAL}")
  endif (NOT EXISTS "${DIFF_FILE}")
  if (NOT EXISTS "${NOTES_FILE}")
    file(WRITE "${NOTES_FILE}" "")
    run_svn("add;${NOTES_LOCAL}")
  endif (NOT EXISTS "${NOTES_FILE}")

  # We want context diffs with procedure names for better readability
  # svn diff does show new files for us but we pass -N just in case
  execute_process(COMMAND ${SVN} diff --diff-cmd diff -x "-c -p -N"
    RESULT_VARIABLE svn_result
    ERROR_VARIABLE svn_err
    OUTPUT_FILE "${DIFF_FILE}")
  if (svn_result OR svn_err)
    message(FATAL_ERROR "*** ${SVN} diff failed: ***\n${svn_err}")
  endif (svn_result OR svn_err)

  file(READ ${NOTES} string)
  string(REGEX REPLACE "^.*\ntoreview:\r?\n" "" pubnotes "${string}")
  string(REGEX REPLACE "^toreview:\r?\n" "" pubnotes "${pubnotes}")
  if ("${string}" STREQUAL "${pubnotes}")
    message(FATAL_ERROR "${NOTES} is missing \"toreview:\" marker")
  endif ("${string}" STREQUAL "${pubnotes}")

  # Do "wc -l" ourselves
  file(READ "${DIFF_FILE}" string)
  string(REGEX MATCHALL "\n" newlines "${string}")
  list(LENGTH newlines lines )

  ##################################################
  # i#78: coding style checks
  #
  # Since these are on the diff, not the code, remember that there
  # are two extra columns at the start of the line!
  set(ugly "*** STYLE VIOLATION:")

  # the old "make ugly" rules
  # to make the regexps simpler I'm assuming no non-alphanums run up
  # against keywords: assuming spaces delimit
  string(REGEX MATCH "(TRACELOG\\([^l]|NOCHECKIN)" bad "${string}")
  if (bad)
    message("${ugly} remove debugging facilities: \"${bad}\"")
  endif (bad)
  string(REGEX MATCH "DO_ONCE\\( *{? *SYSLOG_INTERNAL" bad "${string}")
  if (bad)
    message("${ugly} use SYSLOG_INTERNAL_*_ONCE, not DO_ONCE(SYSLOG_: \"${bad}\"")
  endif (bad)
  # note that ASSERT_NOT_TESTED is already DO_ONCE so shouldn't see DO_ONCE on any ASSERT
  string(REGEX MATCH "ONCE\\( *{? *ASSERT" bad "${string}")
  if (bad)
    message("${ugly} use ASSERT_CURIOSITY_ONCE, not DO_ONCE(ASSERT: \"${bad}\"")
  endif (bad)
  string(REGEX MATCH "\n[^-\\*][^\n]*\t" bad "${string}")
  if (bad)
    message("${ugly} clean TABs w/ M-x untabify (cat -T | grep \"\^I\" to see)")
  endif (bad)
  string(REGEX MATCH " *(if|while) *[^{\n]*\r?\n[^;\n]*\r?\n" bad "${string}")
  if (bad)
    message("${ugly} use {} for multi-line body: \"${bad}\"")
  endif (bad)
  string(REGEX MATCH " if *\\([^\n]*;" bad "${string}")
  if (bad)
    message("${ugly} put if body on separate line: \"${bad}\"")
  endif (bad)
  # loop bodies
  string(REGEX MATCH "\n[^}]* while *\\([^\n]*;" bad "${string}")
  if (bad)
    message("${ugly} put loop body on separate line: \"${bad}\"")
  endif (bad)
  string(REGEX MATCH "\n[^ ]+} *while *\\([^\n]*;" bad "${string}")
  if (bad)
    message("${ugly} put loop body on separate line: \"${bad}\"")
  endif (bad)
  string(REGEX MATCH " do *{[^\n]*;" bad "${string}")
  if (bad)
    message("${ugly} put loop body on separate line: \"${bad}\"")
  endif (bad)
  string(REGEX MATCH " for *([^\n]*;[^\n]*;[^\n]*)[^\n]*;" bad "${string}")
  if (bad)
    message("${ugly} put loop body on separate line: \"${bad}\"")
  endif (bad)
  string(REGEX MATCH "\\( *([0-9]+|NULL) *== *[^0-9]" bad "${string}")
  if (bad)
    message("${ugly} (1 == var) vs. (var == 1): \"${bad}\"")
  endif (bad)
  # check that empty param list is declared void
  # FIXME: should we only require for decls by only running on header files?
  # But we do want to catch static decls.
  string(REGEX MATCH "\n.. *[A-Za-z0-9][A-Za-z0-9_\\* ]+\\(\\);" bad "${string}")
  if (bad)
    message("${ugly} empty param list should be (void) not (): \"${bad}\"")
  endif (bad)

  # the old "make pretty_ugly"
  # since we're only checking the diff and not the existing core we
  # go ahead and apply all the rules
  # Rules that should pass (and almost do, all have only a few violations left and
  # no false positives). FIXME - should be cleaned up and moved to ugly xref 8806.
  string(REGEX MATCH " (do|if|while|for|else|switch)\\(" bad "${string}")
  if (bad)
    message("${ugly} spacing, if( or if  ( vs if (: \"${bad}\"")
  endif (bad)
  string(REGEX MATCH "[}\\);](  +)?(do|if|while|for|else|switch)[ \n]" bad "${string}")
  if (bad)
    message("${ugly} spacing, }else or }  else vs } else: \"${bad}\"")
  endif (bad)

  # The old "really_ugly": see comments above.
  # Rules that don't pass at all, either because of excessive violations in the
  # codebase or because of false positives. FIXME - for rules with few or no false
  # positives but numerous violations we could use a count or something to try and
  # keep things from getting worse xref case 8806.

  # no false positives, but numerous violations
  # that's right, cmake regexp has no {N} so we do it the hard way
  string(REGEX MATCH "\n[^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n][^\n]" bad "${string}")
  if (bad)
    message("${ugly} line is too long: \"${bad}\"")
  endif (bad)

  # FIXME: not bothering to try and catch multiple statements on one line
  # at this time: look at old core/Makefile if interested.  Keeping comment below
  # for all the issues:
  #
  # With all the exemptions very few false positives, but still many violations
  # (and prob. some false negatives). The first grep looks for lines with two ; not
  # within quotes. 2nd, 3rd and 4th greps subtract out lines where the extra
  # semicolon is prob. part of a comment. NOTE, for some reason   doesn't
  # work for matching before the * in the 3rd grep. 5th grep subtracts out for (;;)
  # loop usage and 6th grep subtracts out the very common 'case: foo; break;' motif
  # (even though it technically breaks the rule). Final grep subtracts out certain
  # common macros whose usage often breaks the rule.

  # only a few false positives, but numerous violations
  string(REGEX MATCH "[A-Za-z0-9]\\*[^A-Za-z0-9/\\)\\(]" bad "${string}")
  if (bad)
    message("${ugly} instr_t* foo vs. instr_t *foo, etc.: \"${bad}\"")
  endif (bad)
  # only a few false positives, but numerous violations (even allowing for usage
  # inside /* */ comments)
  string(REGEX MATCH "//" bad "${string}")
  if (bad)
    message("${ugly} use C style comments: \"${bad}\"")
  endif (bad)
  # mostly false positives (we're too grammatical ;) due to using ; in comments
  string(REGEX MATCH "/\\*[^\\*].*;[^\\*]\\*/" bad "${string}")
  if (bad)
    message("${ugly} no commented-out code: \"${bad}\"")
  endif (bad)
  # some false positives and numerous violations
  string(REGEX MATCH "\n[^\\*\n]*long " bad "${string}")
  if (bad)
    message("${ugly} use int/uint instead of long/ulong: \"${bad}\"")
  endif (bad)
  # Only a few false positives and not that many violations (for the "( " rule at
  # least). FIXME - these could probably be moved to pretty_ugly at some point.
  string(REGEX MATCH "\\( +[^\\ ]+" bad "${string}")
  if (bad)
    message("${ugly} spacing, ( foo, bar ) vs. (foo, bar): \"${bad}\"")
  endif (bad)
  string(REGEX MATCH " \\)" bad "${string}")
  if (bad)
    message("${ugly} spacing, ( foo, bar ) vs. (foo, bar): \"${bad}\"")
  endif (bad)
  # no real false positives, but tons of violations (even ignoring asm listings)
  string(REGEX MATCH "\n[^\\*-][^\n]*,[^ \r\n]" bad "${string}")
  if (bad)
    message("${ugly} put spaces after commas: \"${bad}\"")
  endif (bad)

  #
  ##################################################

  # We commit the diff to review/ using special syntax to auto-generate
  # an Issue that covers performing the review
  file(WRITE "${NOTES_FILE}"
    "new review:\nowner: ${REVIEWER}\nsummary: ${AUTHOR}/${year}/${LABEL}.diff\n")
  file(APPEND "${NOTES_FILE}" "${pubnotes}")
  file(APPEND "${NOTES_FILE}" "\nstats: ${lines} diff lines\n")
  if (EXISTS "${DIFFSTAT}")
    execute_process(COMMAND ${DIFFSTAT} "${DIFF_FILE}"
      ERROR_QUIET OUTPUT_VARIABLE diffstat_out)
    file(APPEND "${NOTES_FILE}" "${diffstat_out}")
  endif (EXISTS "${DIFFSTAT}")
  message(STATUS "ready to commit")

  if (COMMIT)
    run_svn("commit;--force-log;-F;${NOTES_LOCAL}")
    message(STATUS "committed")
  endif (COMMIT)

endif (REVERT)