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

CI: add lint github workflow for running clang-format #3749

Merged
merged 1 commit into from Dec 13, 2023

Conversation

tchaikov
Copy link
Contributor

so that we can identify changes which do not confirm to the clang-format rules.

so that we can identify changes which do not confirm to the
clang-format rules.

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
@tchaikov
Copy link
Contributor Author

tchaikov commented Dec 10, 2023

@vitaut i found you've been applying clang-format to the tree every now and then. so i think it might be better to integrate it as a part of the CI. a run of this workflow action can be found at my github repo, see https://github.com/tchaikov/fmt/actions/runs/7155365967/job/19483902537

but there are couple issues (or todos):

  1. clang-format's behavior varies from version to version. i just formatted the tree with (yet-released) clang-18 and clang-17.0.4. their results are different. but the latter is closer to what we have in master HEAD at the time of writing. probably i picked a wrong version.
  2. even the latest master HEAD cannot appease clang-format 17.0.4 with the latest .clang-format. some of the diff make sense to me, while some of them do not. i am attaching the diff at the end of this comment
  3. all of the diffs are taken into account. in other words, even the existing code are checked. if we enforce clang-format in the whole tree, this is a no-issue, though.
  4. instead of simply failing the check. could use better UX. for instance, to integrate using github annotation. we could use some more sophisticated github workflow or create a python script to paste the diff as a comment, or better off using github annotation manually. but before going further into this direction. i would like to have more feedbacks from the maintainer and the wider community.
diff --git a/include/fmt/color.h b/include/fmt/color.h
index 57b7f578..34de634c 100644
--- a/include/fmt/color.h
+++ b/include/fmt/color.h
@@ -563,9 +563,10 @@ OutputIt vformat_to(
                    fmt::emphasis::bold | fg(fmt::color::red), "{}", 42);
   \endrst
 */
-template <typename OutputIt, typename S, typename... Args,
-          bool enable = detail::is_output_iterator<OutputIt, char_t<S>>::value&&
-              detail::is_string<S>::value>
+template <
+    typename OutputIt, typename S, typename... Args,
+    bool enable = detail::is_output_iterator<OutputIt, char_t<S>>::value &&
+                  detail::is_string<S>::value>
 inline auto format_to(OutputIt out, const text_style& ts, const S& format_str,
                       Args&&... args) ->
     typename std::enable_if<enable, OutputIt>::type {
diff --git a/include/fmt/core.h b/include/fmt/core.h
index 3bb281b2..04dccbd2 100644
--- a/include/fmt/core.h
+++ b/include/fmt/core.h
@@ -2429,7 +2429,8 @@ FMT_CONSTEXPR FMT_INLINE auto parse_format_specs(
     case 'G':
       return parse_presentation_type(pres::general_upper, float_set);
     case 'c':
-      if (arg_type == type::bool_type) throw_format_error("invalid format specifier");
+      if (arg_type == type::bool_type)
+        throw_format_error("invalid format specifier");
       return parse_presentation_type(pres::chr, integral_set);
     case 's':
       return parse_presentation_type(pres::string,
diff --git a/include/fmt/format-inl.h b/include/fmt/format-inl.h
index 5f8c83a2..96b5f141 100644
--- a/include/fmt/format-inl.h
+++ b/include/fmt/format-inl.h
@@ -984,7 +984,8 @@ template <> struct cache_accessor<double> {
       {0xe0accfa875af45a7, 0x93eb1b80a33b8606},
       {0x8c6c01c9498d8b88, 0xbc72f130660533c4},
       {0xaf87023b9bf0ee6a, 0xeb8fad7c7f8680b5},
-      {0xdb68c2ca82ed2a05, 0xa67398db9f6820e2}
+      { 0xdb68c2ca82ed2a05,
+        0xa67398db9f6820e2 }
 #else
       {0xff77b1fcbebcdc4f, 0x25e8e89c13bb0f7b},
       {0xce5d73ff402d98e3, 0xfb0a3d212dc81290},
diff --git a/include/fmt/ostream.h b/include/fmt/ostream.h
index 2ed35df1..3086191a 100644
--- a/include/fmt/ostream.h
+++ b/include/fmt/ostream.h
@@ -95,7 +95,7 @@ void format_value(buffer<Char>& buf, const T& value) {
   auto&& format_buf = formatbuf<std::basic_streambuf<Char>>(buf);
   auto&& output = std::basic_ostream<Char>(&format_buf);
 #if !defined(FMT_STATIC_THOUSANDS_SEPARATOR)
-  output.imbue(std::locale::classic()); // The default is always unlocalized.
+  output.imbue(std::locale::classic());  // The default is always unlocalized.
 #endif
   output << value;
   output.exceptions(std::ios_base::failbit | std::ios_base::badbit);
diff --git a/include/fmt/std.h b/include/fmt/std.h
index 6f6b23ff..e855ba51 100644
--- a/include/fmt/std.h
+++ b/include/fmt/std.h
@@ -137,8 +137,7 @@ template <typename Char> struct formatter<std::filesystem::path, Char> {
       debug_ = true;
       ++it;
     }
-    if (it != end && (*it == 'g' || *it == 'n'))
-      path_type_ = *it++;
+    if (it != end && (*it == 'g' || *it == 'n')) path_type_ = *it++;
     return it;
   }

@@ -246,8 +245,7 @@ FMT_END_NAMESPACE
 #ifdef __cpp_lib_source_location
 FMT_BEGIN_NAMESPACE
 FMT_EXPORT
-template<>
-struct formatter<std::source_location> {
+template <> struct formatter<std::source_location> {
   template <typename ParseContext> FMT_CONSTEXPR auto parse(ParseContext& ctx) {
     return ctx.begin();
   }
@@ -384,7 +382,7 @@ template <typename Char> struct formatter<std::error_code, Char> {
 FMT_EXPORT
 template <typename T, typename Char>
 struct formatter<
-    T, Char, // DEPRECATED! Mixing code unit types.
+    T, Char,  // DEPRECATED! Mixing code unit types.
     typename std::enable_if<std::is_base_of<std::exception, T>::value>::type> {
  private:
   bool with_typename_ = false;
diff --git a/include/fmt/xchar.h b/include/fmt/xchar.h
index 625ec369..5a7c997f 100644
--- a/include/fmt/xchar.h
+++ b/include/fmt/xchar.h
@@ -70,7 +70,7 @@ constexpr format_arg_store<wformat_context, T...> make_wformat_args(

 inline namespace literals {
 #if FMT_USE_USER_DEFINED_LITERALS && !FMT_USE_NONTYPE_TEMPLATE_ARGS
-constexpr detail::udl_arg<wchar_t> operator"" _a(const wchar_t* s, size_t) {
+constexpr detail::udl_arg<wchar_t> operator"" _a(const wchar_t * s, size_t) {
   return {s};
 }
 #endif
@@ -172,11 +172,11 @@ inline auto vformat_to(
   return detail::get_iterator(buf, out);
 }

-template <
-    typename OutputIt, typename Locale, typename S, typename... T,
-    typename Char = char_t<S>,
-    bool enable = detail::is_output_iterator<OutputIt, Char>::value&&
-        detail::is_locale<Locale>::value&& detail::is_exotic_char<Char>::value>
+template <typename OutputIt, typename Locale, typename S, typename... T,
+          typename Char = char_t<S>,
+          bool enable = detail::is_output_iterator<OutputIt, Char>::value &&
+                        detail::is_locale<Locale>::value &&
+                        detail::is_exotic_char<Char>::value>
 inline auto format_to(OutputIt out, const Locale& loc, const S& format_str,
                       T&&... args) ->
     typename std::enable_if<enable, OutputIt>::type {
diff --git a/src/fmt.cc b/src/fmt.cc
index 6638bb4a..5330463a 100644
--- a/src/fmt.cc
+++ b/src/fmt.cc
@@ -101,7 +101,7 @@ extern "C++" {

 // gcc doesn't yet implement private module fragments
 #if !FMT_GCC_VERSION
-module : private;
+module :private;
 #endif

 #include "format.cc"

@vitaut
Copy link
Contributor

vitaut commented Dec 12, 2023

Thanks for the PR! This was discussed in the past and the main concern was variability of clang-format output between versions. I am fine with giving it a try but we should also apply clang-format to existing code to avoid failures for new PRs.

@tchaikov
Copy link
Contributor Author

@vitaut thank you for your review and suggestion. i just posted #3755. hopefully it can help to unblock this change.

@vitaut vitaut merged commit 7d757cb into fmtlib:master Dec 13, 2023
40 checks passed
@vitaut
Copy link
Contributor

vitaut commented Dec 13, 2023

Merged, thanks.

@tchaikov tchaikov deleted the ci-clang-format branch December 13, 2023 15:40
happymonkey1 pushed a commit to happymonkey1/fmt that referenced this pull request Apr 7, 2024
so that we can identify changes which do not confirm to the
clang-format rules.

Signed-off-by: Kefu Chai <tchaikov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants