Skip to content
This repository was archived by the owner on Mar 22, 2023. It is now read-only.

Make cpp-format targets always working#1118

Merged
lukaszstolarczuk merged 1 commit intopmem:masterfrom
karczex:cppformat_always_avialable
Jul 29, 2021
Merged

Make cpp-format targets always working#1118
lukaszstolarczuk merged 1 commit intopmem:masterfrom
karczex:cppformat_always_avialable

Conversation

@karczex
Copy link
Copy Markdown

@karczex karczex commented Jul 2, 2021

As cpp-format targets are always generated anyway, they should work even
if CHECK_CPP_STYLE flag is disabled.


This change is Reviewable

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 2, 2021

Codecov Report

Merging #1118 (7e2a005) into master (f1f199d) will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1118      +/-   ##
==========================================
+ Coverage   93.57%   93.62%   +0.04%     
==========================================
  Files          52       52              
  Lines        4483     4483              
==========================================
+ Hits         4195     4197       +2     
+ Misses        288      286       -2     
Flag Coverage Δ
tests_clang_debug_cpp17 93.18% <ø> (-0.03%) ⬇️
tests_gcc_debug 90.14% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ude/libpmemobj++/container/concurrent_hash_map.hpp 93.24% <0.00%> (+0.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1f199d...7e2a005. Read the comment docs.

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @karczex)


CMakeLists.txt, line 193 at r1 (raw file):

endif()

if(CHECK_CPP_STYLE)

this change was initially done to avoid looking for clang-format (v. 9) on some OSes. It will be an issue on these OSes, which don't have the required clang-format, so I believe it's not the solution.. 😉

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @karczex)


CMakeLists.txt, line 193 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

this change was initially done to avoid looking for clang-format (v. 9) on some OSes. It will be an issue on these OSes, which don't have the required clang-format, so I believe it's not the solution.. 😉

or maybe not 😄

but pls check the other OSes

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

beside the two minor issues it looks good 👍

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @karczex)


CMakeLists.txt, line 186 at r1 (raw file):

			BSD-3-Clause)

find_program(CLANG_FORMAT NAMES clang-format clang-format-9 clang-format-9.0)

I guess we should re-order these names - firstly try to find cf9 (and 9.0), then cf


CMakeLists.txt, line 189 at r1 (raw file):

set(CLANG_FORMAT_REQUIRED "9.0")
if(CLANG_FORMAT)
	message(STATUS "Found clang-format: ${CLANG_FORMAT} (version: ${CLANG_FORMAT_VERSION})")

pls switch the order of these two lines - ${CLANG_FORMAT_VERSION} is only set after the get_program_version_major_minor function call 😉

Copy link
Copy Markdown
Contributor

@KFilipek KFilipek left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @karczex)

Copy link
Copy Markdown
Contributor

@igchor igchor left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @karczex)


cmake/functions.cmake, line 102 at r1 (raw file):

# ${name} must be unique.
function(add_cppstyle name)
	if(NOT CLANG_FORMAT AND NOT (CLANG_FORMAT_VERSION VERSION_EQUAL CLANG_FORMAT_REQUIRED))

AND? Shouldn't this be OR?

As cpp-format targets are always generated anyway, they should work even
if CHECK_CPP_STYLE flag is disabled.
@karczex karczex force-pushed the cppformat_always_avialable branch from 6721c39 to 7e2a005 Compare July 29, 2021 16:19
Copy link
Copy Markdown
Author

@karczex karczex left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @igchor, @KFilipek, and @lukaszstolarczuk)


CMakeLists.txt, line 186 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I guess we should re-order these names - firstly try to find cf9 (and 9.0), then cf

Done.


CMakeLists.txt, line 189 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

pls switch the order of these two lines - ${CLANG_FORMAT_VERSION} is only set after the get_program_version_major_minor function call 😉

Done.


cmake/functions.cmake, line 102 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

AND? Shouldn't this be OR?

Done.

Copy link
Copy Markdown
Contributor

@KFilipek KFilipek left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @igchor and @lukaszstolarczuk)

Copy link
Copy Markdown
Contributor

@KFilipek KFilipek left a comment

Choose a reason for hiding this comment

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

:lgtm:

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @igchor)

@lukaszstolarczuk lukaszstolarczuk merged commit ca739fa into pmem:master Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants