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.

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

Use Clang-Format #4226

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from
Draft

Use Clang-Format #4226

wants to merge 5 commits into from

Conversation

nlohmann
Copy link
Owner

This PR replaces Artistic Style by Clang-Format as tool to indent the code. Astyle introduced a breaking change a while ago which made working with the tool very cumbersome.

This comment was marked as outdated.

@coveralls
Copy link

coveralls commented Nov 29, 2023

Coverage Status

coverage: 100.0%. remained the same
when pulling af3f87e on clang-format
into 9cca280 on develop.

This comment was marked as outdated.

enable_if_t < std::is_arithmetic<ArithmeticType>::value&&
!std::is_same<ArithmeticType, typename BasicJsonType::boolean_t>::value,
int > = 0 >
template<typename BasicJsonType, typename ArithmeticType, enable_if_t<std::is_arithmetic<ArithmeticType>::value && !std::is_same<ArithmeticType, typename BasicJsonType::boolean_t>::value, int> = 0>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh, not sure this is a good change. Is there a setting that will prevent it?

typename StringType,
enable_if_t<
std::is_assignable<StringType&, const typename BasicJsonType::string_t>::value && is_detected_exact<typename BasicJsonType::string_t::value_type, value_type_t, StringType>::value && !std::is_same<typename BasicJsonType::string_t, StringType>::value && !is_json_ref<StringType>::value,
int> = 0>
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that this one stayed multi-line. Perhaps there is a max line length setting?

Copy link
Owner Author

Choose a reason for hiding this comment

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

There is, but I have not tried it yet.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think it is a bit better with max width of ColumnLimit=160.

!std::is_same<ConstructibleArrayType, typename BasicJsonType::binary_t>::value&&
!is_basic_json<ConstructibleArrayType>::value,
int > = 0 >
template<typename BasicJsonType, typename ConstructibleArrayType, enable_if_t<is_constructible_array_type<BasicJsonType, ConstructibleArrayType>::value && !is_constructible_object_type<BasicJsonType, ConstructibleArrayType>::value && !is_constructible_string_type<BasicJsonType, ConstructibleArrayType>::value && !std::is_same<ConstructibleArrayType, typename BasicJsonType::binary_t>::value && !is_basic_json<ConstructibleArrayType>::value, int> = 0>
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is much longer, must be some other trigger for single line vs multi-line.

}
};
{
{0xAB70FE17C79AC6CA, -1060, -300},
Copy link
Contributor

Choose a reason for hiding this comment

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

Another questionable change. I find the whitespace inside the braces is helpful. Interesting that it did multi-line column alignment of comments on the includes and removed column alignment of the parameters here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I've seen it, but I have not found the config key to fix this yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cpp11BracedListStyle affects the whitespace.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I will try without.

include/nlohmann/detail/conversions/from_json.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@gregmarr gregmarr left a comment

Choose a reason for hiding this comment

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

There are some interesting changes. I wonder if the result would be different if you went back to the astyle code and formatted again with the current options rather than incremental changes with different options.

JSON_PRIVATE_UNLESS_TESTED:
// convenience aliases for types residing in namespace detail;
using lexer = ::nlohmann::detail::lexer_base<basic_json>;
JSON_PRIVATE_UNLESS_TESTED :
Copy link
Contributor

Choose a reason for hiding this comment

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

You can tell clang-format what this macro means, so it knows it's an access specifier: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#macros

Copy link
Owner Author

Choose a reason for hiding this comment

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

Interesting! I'll give it a try.

NLOHMANN_JSON_ABI_TAGS, \
NLOHMANN_JSON_NAMESPACE_VERSION) \
{
#define NLOHMANN_JSON_NAMESPACE_BEGIN \
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like AlignEscapedNewlines: Right aligns to ColumnLimit if it's non-zero. That's unfortunate.

Copy link

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @nlohmann
Please read and follow the Contribution Guidelines.

Copy link

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated. @nlohmann
Please read and follow the Contribution Guidelines.

@nlohmann
Copy link
Owner Author

I call it a day. There seems to be some work needed to move the NOLINT comments to the right place after reformatting. Could have been crazy to expect Clang-Format to know what Clang-Tidy is doing...

Furthermore, there seems to be the same version nightmare as with astyle. I am using Clang-Format 17.0.6 locally and Clang-Format 18.0.0 in the CI, and already the results are not the same. Not sure how to make it easy for contributors to use the "right" version.

Finally, Clang-Format is incredibly slow. Like 7 minutes to format single_include/json.hpp. Astyle took some seconds.

I'll pick this up next week.

@t-b
Copy link
Contributor

t-b commented Nov 29, 2023

I am using Clang-Format 17.0.6 locally and Clang-Format 18.0.0 in the CI, and already the results are not the same.

This is to be expected unforunately. Due to the complexity the devs decided not to be backwards compatible for every edge case.

Finally, Clang-Format is incredibly slow. Like 7 minutes to format single_include/json.hpp. Astyle took some seconds.

clang-format 17.03 takes here ~19minutes locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants