Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor add_cpplint_files and add_clang_format files to support downstream projects, large file numbers. #3762

Merged
merged 8 commits into from
Dec 8, 2022
Merged

Refactor add_cpplint_files and add_clang_format files to support downstream projects, large file numbers. #3762

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d3f01a6
add_cpplint_files must be a macro, not a function.
fruffy-bfn Dec 7, 2022
8981b13
Use global properties instead.
fruffy-bfn Dec 7, 2022
d8db73d
Remove duplicates.
fruffy-bfn Dec 7, 2022
eb37a3b
Fix.
fruffy-bfn Dec 7, 2022
149a5c2
Review comments.
fruffy-bfn Dec 7, 2022
fcc93d9
Typo.
fruffy-bfn Dec 7, 2022
fb8f684
More usability improvements.
fruffy-bfn Dec 7, 2022
a347288
last edit.
fruffy-bfn Dec 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 33 additions & 11 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,8 @@ file(
frontends/*.h
ir/*.cpp
ir/*.h
midend/*.cpp
midend/*.h
tools/*.cpp
tools/*.h
)
Expand All @@ -507,18 +509,31 @@ list(FILTER P4C_LINT_LIST EXCLUDE REGEX "control-plane/p4runtime")
# cpplint
add_cpplint_files(${P4C_SOURCE_DIR} "${P4C_LINT_LIST}")

# Retrieve the global cpplint property.
get_property(CPPLINT_FILES GLOBAL PROPERTY cpplint-files)

if(DEFINED CPPLINT_FILES)
list ( SORT CPPLINT_FILES )
set (CPPLINT_CMD ${P4C_SOURCE_DIR}/tools/cpplint.py)
set (CPPLINT_ARGS --root=${P4C_SOURCE_DIR} --extensions=h,hpp,cpp,ypp,l)
add_custom_target(cpplint
COMMAND ${CPPLINT_CMD} ${CPPLINT_ARGS} ${CPPLINT_FILES}
# Replace the semicolon-separators with space separators.
string(REPLACE ";" " " CPPLINT_FILES "${CPPLINT_FILES}")
# Write the list to a file.
Copy link
Contributor

@davidbolvansky davidbolvansky Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would mention why need this workaround as me reach sh limitation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And probably you do not need this if you use xargs -d ';'

set(CPPLINT_FILE ${P4C_BINARY_DIR}/cpplint_files.txt)
file(WRITE ${CPPLINT_FILE} "${CPPLINT_FILES}")
list(SORT CPPLINT_FILES)
set(CPPLINT_CMD ${P4C_SOURCE_DIR}/tools/cpplint.py)
set(CPPLINT_ARGS --root=${P4C_SOURCE_DIR} --extensions=h,hpp,cpp,ypp,l)
# Pipe the file into cpplint.
add_custom_target(
cpplint
COMMAND cat ${CPPLINT_FILE} | xargs -n 10000000 ${CPPLINT_CMD} ${CPPLINT_ARGS}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need '-n 10000000'?

WORKING_DIRECTORY ${P4C_SOURCE_DIR}
COMMENT "cpplint")
add_custom_target(cpplint-quiet
COMMAND ${CPPLINT_CMD} --quiet ${CPPLINT_ARGS} ${CPPLINT_FILES}
COMMENT "cpplint"
)
add_custom_target(
cpplint-quiet
COMMAND cat ${CPPLINT_FILE} | xargs -n 10000000 ${CPPLINT_CMD} ${CPPLINT_ARGS} --quiet
WORKING_DIRECTORY ${P4C_SOURCE_DIR}
COMMENT "cpplint quietly")
COMMENT "cpplint quietly"
)
endif()

# clang-format
Expand All @@ -530,18 +545,25 @@ add_clang_format_files(${P4C_SOURCE_DIR} "${P4C_LINT_LIST}")
find_program(CLANG_FORMAT_CMD clang-format)

if(NOT ${CLANG_FORMAT_CMD})
# Retrieve the global clang-format property.
get_property(CLANG_FORMAT_FILES GLOBAL PROPERTY clang-format-files)
if(DEFINED CLANG_FORMAT_FILES)
# Replace the semicolon-separators with space separators.
string(REPLACE ";" " " CLANG_FORMAT_FILES "${CLANG_FORMAT_FILES}")
# Write the list to a file.
set(CLANG_FORMAT_FILE ${P4C_BINARY_DIR}/clang_format_files.txt)
file(WRITE ${CLANG_FORMAT_FILE} "${CLANG_FORMAT_FILES}")
list(SORT CLANG_FORMAT_FILES)
set(CLANG_FORMAT_CMD clang-format)
add_custom_target(
clang-format
COMMAND ${CLANG_FORMAT_CMD} --verbose --Werror --dry-run -i ${CLANG_FORMAT_FILES}
COMMAND cat ${CLANG_FORMAT_FILE} | xargs ${CLANG_FORMAT_CMD} --verbose --Werror --dry-run -i --
WORKING_DIRECTORY ${P4C_SOURCE_DIR}
COMMENT "Checking files for correct clang-format formatting."
)
add_custom_target(
clang-format-fix-errors
COMMAND ${CLANG_FORMAT_CMD} --verbose -i ${CLANG_FORMAT_FILES}
COMMAND cat ${CLANG_FORMAT_FILE} | xargs ${CLANG_FORMAT_CMD} --verbose -i --
WORKING_DIRECTORY ${P4C_SOURCE_DIR}
COMMENT "Formatting files using clang-format."
)
Expand Down
28 changes: 20 additions & 8 deletions cmake/P4CUtils.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ macro (p4c_add_library name symbol var)
endmacro(p4c_add_library)

# Add files with the appropriate path to the list of linted files
function(add_cpplint_files dir filelist)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably some warning comment is needed that these macros could be used by downstream projects so it is important to leave them as macros, etc..

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we are adding cpplint files to a global property this might actually not be necessary any longer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

macro(add_cpplint_files dir filelist)
unset (__cpplintFileList)
foreach(__f ${filelist})
string(REGEX MATCH "^/.*" abs_path "${__f}")
if (NOT ${abs_path} EQUAL "")
Expand All @@ -58,20 +59,31 @@ function(add_cpplint_files dir filelist)
list (APPEND __cpplintFileList "${dir}/${__f}")
endif()
endforeach(__f)
set (CPPLINT_FILES ${CPPLINT_FILES} ${__cpplintFileList} PARENT_SCOPE)
endfunction(add_cpplint_files)

function(add_clang_format_files dir filelist)
# Get the global cpplint property and append to it.
get_property(CPPLINT_FILES GLOBAL PROPERTY cpplint-files)
list (APPEND CPPLINT_FILES "${__cpplintFileList}")
list(REMOVE_DUPLICATES CPPLINT_FILES)
set_property(GLOBAL PROPERTY cpplint-files "${CPPLINT_FILES}")
endmacro(add_cpplint_files)

macro(add_clang_format_files dir filelist)
unset (__clangFormatFileList)
foreach(__f ${filelist})
string(REGEX MATCH "^/.*" abs_path "${__f}")
if (NOT ${abs_path} EQUAL "")
list (APPEND __cpplintFileList "${__f}")
list (APPEND __clangFormatFileList "${__f}")
else()
list (APPEND __cpplintFileList "${dir}/${__f}")
list (APPEND __clangFormatFileList "${dir}/${__f}")
endif()
endforeach(__f)
set (CLANG_FORMAT_FILES ${CLANG_FORMAT_FILES} ${__cpplintFileList} PARENT_SCOPE)
endfunction(add_clang_format_files)

# Get the global clang-format property and append to it.
get_property(CLANG_FORMAT_FILES GLOBAL PROPERTY clang-format-files)
list (APPEND CLANG_FORMAT_FILES "${__clangFormatFileList}")
list(REMOVE_DUPLICATES CLANG_FORMAT_FILES)
set_property(GLOBAL PROPERTY clang-format-files "${CLANG_FORMAT_FILES}")
endmacro(add_clang_format_files)


macro(p4c_test_set_name name tag alias)
Expand Down