Clang tidy - apply (unicode-org#326)

* Add plural rules to CPP * Clang-tidy applied to *.cpp * More style fixes * DDT-234 Apply clang-tidy to *.cpp * Added Clang Tidy to CI * Apply changes from clang-tidy * Make icu4c setup script more robust * Add a clang `compile_commands.txt` to assist compilation output msgs * Apply clang-tidy change * Add clang-tidy config file * Test clang-tidy run on CI to sidestep CI upload step permissions * Mod to workaround GHA permissions * Try to include json-c dependency in clang-tidy CI action * Refactor clang-tidy CI jobs into a separate workflow * Update CI job names * Change workflow name to trigger the 2nd workflow * Remove second workflow b/c it won't trigger without user PAT tokens on a PR branch * test CI by inducing a known error * Revert "test CI by inducing a known error" This reverts commit d7a90ff. * Add CLI usage for clang-tidy locally --------- Co-authored-by: Elango Cheran <[email protected]>
sven-oly · Nov 7, 2024 · b3900bc · b3900bc
1 parent afc6eb1
commit b3900bc
Show file tree

Hide file tree

Showing 17 changed files with 206 additions and 147 deletions.
diff --git a/.github/workflows/clang-tidy-review.yml b/.github/workflows/clang-tidy-review.yml
@@ -0,0 +1,36 @@
+name: clang-tidy-review
+
+# You can be more specific, but it currently only works on pull requests
+on: [pull_request]
+
+jobs:
+  build:
+    name: Lint ICU4C C++ executor
+    runs-on: ubuntu-latest
+
+    steps:
+    - uses: actions/checkout@v4
+
+    # Optionally generate compile_commands.json
+
+    # Run clang-tidy
+    # Note: when running locally at the command line, use the equivalent
+    # command when in the directory `executors/cpp`:
+    #     clang-tidy  *.cpp --fix-errors --config-file="clang-tidy-config.yml" -p .
+    # Note: you must run setup.sh and also run install_icu4c_binary.sh (for a given ICU4C version) first
+    # before running the above clang-tidy command
+    - uses: ZedThree/[email protected]
+      with:
+        # clang-tidy specific configs
+        build_dir: './executors/cpp'
+        config_file: './executors/cpp/clang-tidy-config.yml'
+        # Action-specific config
+        split_workflow: true
+        apt_packages: "libjson-c-dev,libicu-dev"
+      id: review
+
+    - uses: ZedThree/clang-tidy-review/[email protected]
+
+    # If there are any comments, fail the check
+    - if: steps.review.outputs.total_comments > 0
+      run: exit 1
diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml
@@ -16,7 +16,7 @@ permissions:
 
 jobs:
   run_all:
-    name: End-to-end (Gen data, run tests, gen GH Pages)
+    name: End-to-end
     runs-on: ubuntu-latest
     steps:
       - name: Checkout repo

diff --git a/.github/workflows/run-rust.yml b/.github/workflows/run-rust.yml
@@ -12,7 +12,7 @@ jobs:
       fail-fast: false
       matrix:
         icu4x-version: [ '1.3', '1.4' ]
-    name: Lint the executor code for ICU4X
+    name: Lint ICU4X Rust executor
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v2

diff --git a/executors/cpp/clang-tidy-config.yml b/executors/cpp/clang-tidy-config.yml
@@ -0,0 +1 @@
+HeaderFilterRegex:   '.*'
diff --git a/executors/cpp/coll.cpp b/executors/cpp/coll.cpp
@@ -42,12 +42,10 @@ using icu::RuleBasedCollator;
 
 const char error_message[] = "error";
 
-bool debug = false;
-
 /**
  * TestCollator  --  process JSON inputs, run comparator, return result
  */
-const string TestCollator(json_object *json_in) {
+string TestCollator(json_object *json_in) {
   UErrorCode status = U_ZERO_ERROR;
 
   json_object *label_obj = json_object_object_get(json_in, "label");
@@ -68,7 +66,7 @@ const string TestCollator(json_object *json_in) {
 
   json_object *locale_obj = json_object_object_get(json_in, "locale");
   const char *locale_string;
-  if (locale_obj) {
+  if (locale_obj != nullptr) {
     locale_string = json_object_get_string(locale_obj);
   } else {
     locale_string = "und";
@@ -78,7 +76,7 @@ const string TestCollator(json_object *json_in) {
   json_object *compare_type_obj =
       json_object_object_get(json_in, "compare_type");
   string compare_type_string = "";
-  if (compare_type_obj) {
+  if (compare_type_obj != nullptr) {
     compare_type_string = json_object_get_string(compare_type_obj);
   }
 
@@ -87,7 +85,7 @@ const string TestCollator(json_object *json_in) {
   string strength_string = "";
 
   json_object *strength_obj = json_object_object_get(json_in, "strength");
-  if (strength_obj) {
+  if (strength_obj != nullptr) {
     strength_string = json_object_get_string(strength_obj);
     if (strength_string == "primary") {
       strength_type = Collator::PRIMARY;
@@ -105,14 +103,14 @@ const string TestCollator(json_object *json_in) {
   // Check for rule-based collation
   json_object *rules_obj = json_object_object_get(json_in, "rules");
   string rules_string = "";
-  if (rules_obj) {
+  if (rules_obj != nullptr) {
     rules_string = json_object_get_string(rules_obj);
   }
   UnicodeString uni_rules = UnicodeString::fromUTF8(rules_string);
 
   // Allow for different levels or types of comparison.
   json_object *compare_type = json_object_object_get(json_in, "compare_type");
-  if (compare_type) {
+  if (compare_type != nullptr) {
     // TODO: Apply this in tests.
     const char *comparison_type = json_object_get_string(compare_type);
   }
@@ -172,12 +170,12 @@ const string TestCollator(json_object *json_in) {
       return json_object_to_json_string(return_json);
     }
 
-    if (strength_obj) {
+    if (strength_obj != nullptr) {
       uni_coll->setStrength(strength_type);
     }
 
-    if (ignore_obj) {
-      const bool ignore_punctuation_bool = json_object_get_boolean(ignore_obj);
+    if (ignore_obj != nullptr) {
+      const bool ignore_punctuation_bool = json_object_get_boolean(ignore_obj) != 0;
       if (ignore_punctuation_bool) {
         uni_coll->setAttribute(UCOL_ALTERNATE_HANDLING, UCOL_SHIFTED, status);
         if (check_icu_error(
@@ -202,7 +200,7 @@ const string TestCollator(json_object *json_in) {
       return json_object_to_json_string(return_json);
     }
 
-    if (uni_coll) {
+    if (uni_coll != nullptr) {
       uni_coll->getAttribute(UCOL_ALTERNATE_HANDLING, status);  // ignore result
     }
     delete uni_coll;
@@ -214,11 +212,6 @@ const string TestCollator(json_object *json_in) {
   coll_result = (uni_result != UCOL_GREATER);
   if (!coll_result) {
     // Test did not succeed!
-    if (debug) {
-      cout << "# UNI_RESULT: " << label_string << " " << uni_result <<
-          "  s1: " << string1 << " s2: " << string2 << endl;
-    }
-
     // Include data compared in the failing test
     json_object_object_add(
         return_json, "s1", json_object_new_string(string1.c_str()));
@@ -231,7 +224,7 @@ const string TestCollator(json_object *json_in) {
   }
 
   json_object_object_add(
-      return_json, "result", json_object_new_boolean(coll_result));
+      return_json, "result", json_object_new_boolean(static_cast<json_bool>(coll_result)));
 
   return  json_object_to_json_string(return_json);
 }
diff --git a/executors/cpp/compile_flags.txt b/executors/cpp/compile_flags.txt
@@ -0,0 +1,9 @@
+-L/tmp/icu/icu/usr/local/lib
+-I/usr/include/c++/13
+-I/usr/include/x86_64-linux-gnu/c++/13
+-I/usr/include/c++/13/backward
+-I/usr/lib/gcc/x86_64-linux-gnu/13/include
+-I/usr/local/include
+-I/usr/include/x86_64-linux-gnu
+-I/usr/include
+
diff --git a/executors/cpp/datetime_fmt.cpp b/executors/cpp/datetime_fmt.cpp
@@ -4,8 +4,8 @@
 
 #include <json-c/json.h>
 
-#include <stdio.h>
-#include <stdlib.h>
+#include <cstdio>
+#include <cstdlib>
 
 #include <iostream>
 #include <string>
@@ -33,15 +33,19 @@ using std::cout;
 using std::endl;
 using std::string;
 
-icu::DateFormat::EStyle StringToEStyle(string style_string) {
-  if (style_string == "full") return icu::DateFormat::kFull;
-  if (style_string == "long") return icu::DateFormat::kLong;
-  if (style_string == "medium") return icu::DateFormat::kMedium;
-  if (style_string == "short") return icu::DateFormat::kShort;
+auto StringToEStyle(string style_string) -> icu::DateFormat::EStyle {
+  if (style_string == "full") { return icu::DateFormat::kFull;
+}
+  if (style_string == "long") { return icu::DateFormat::kLong;
+}
+  if (style_string == "medium") { return icu::DateFormat::kMedium;
+}
+  if (style_string == "short") { return icu::DateFormat::kShort;
+}
   return icu::DateFormat::kNone;
 }
 
-const string TestDatetimeFmt(json_object *json_in) {
+string TestDatetimeFmt(json_object *json_in) {
   UErrorCode status = U_ZERO_ERROR;
 
   json_object *label_obj = json_object_object_get(json_in, "label");
@@ -55,7 +59,7 @@ const string TestDatetimeFmt(json_object *json_in) {
   // The locale for formatted output
   json_object *locale_label_obj = json_object_object_get(json_in, "locale");
   string locale_string;
-  if (locale_label_obj) {
+  if (locale_label_obj != nullptr) {
     locale_string = json_object_get_string(locale_label_obj);
   } else {
     locale_string = "und";
@@ -72,19 +76,19 @@ const string TestDatetimeFmt(json_object *json_in) {
   // Get fields out of the options if present
   json_object* options_obj = json_object_object_get(json_in, "options");
 
-  if (options_obj) {
+  if (options_obj != nullptr) {
     // Check for timezone and calendar
     json_object* option_item =
         json_object_object_get(options_obj, "timeZone");
-    if (option_item) {
+    if (option_item != nullptr) {
       string timezone_str = json_object_get_string(option_item);
       UnicodeString u_tz(timezone_str.c_str());
       tz = TimeZone::createTimeZone(u_tz);
     }
 
     json_object* cal_item =
         json_object_object_get(options_obj, "calendar");
-    if (cal_item) {
+    if (cal_item != nullptr) {
       calendar_str = json_object_get_string(cal_item);
     }
   }
@@ -93,12 +97,12 @@ const string TestDatetimeFmt(json_object *json_in) {
   locale_string = locale_string + "@calendar=" + calendar_str;
   display_locale = locale_string.c_str();
 
-  if (tz) {
+  if (tz != nullptr) {
     cal = Calendar::createInstance(tz, display_locale, status);
   } else {
     cal = Calendar::createInstance(display_locale, status);
   }
-  if (U_FAILURE(status)) {
+  if (U_FAILURE(status) != 0) {
     json_object_object_add(
         return_json,
         "error",
@@ -125,15 +129,15 @@ const string TestDatetimeFmt(json_object *json_in) {
   // skeleton or date_style.
   string default_skeleton_string = "M/d/yyyy";
 
-  if (options_obj) {
+  if (options_obj != nullptr) {
     json_object* option_item = json_object_object_get(options_obj, "dateStyle");
-    if (option_item) {
+    if (option_item != nullptr) {
       dateStyle_str = json_object_get_string(option_item);
       date_style = StringToEStyle(dateStyle_str);
     }
 
     option_item = json_object_object_get(options_obj, "timeStyle");
-    if (option_item) {
+    if (option_item != nullptr) {
       timeStyle_str = json_object_get_string(option_item);
       time_style = StringToEStyle(timeStyle_str);
     }
@@ -146,13 +150,13 @@ const string TestDatetimeFmt(json_object *json_in) {
       time_style == icu::DateFormat::EStyle::kNone) {
     skeleton_string = default_skeleton_string;
   }
-  if (date_skeleton_obj) {
+  if (date_skeleton_obj != nullptr) {
     // Data specifies a date time skeleton. Make a formatter based on this.
     skeleton_string = json_object_get_string(date_skeleton_obj);
   }
   if (skeleton_string != "") {
     UnicodeString u_skeleton(skeleton_string.c_str());
-    if (cal) {
+    if (cal != nullptr) {
       df = DateFormat::createInstanceForSkeleton(cal,
                                                  u_skeleton,
                                                  display_locale,
@@ -183,7 +187,7 @@ const string TestDatetimeFmt(json_object *json_in) {
   }
 
   // !!! IS OFFSET ALREADY CONSIDERED?
-  if (tz) {
+  if (tz != nullptr) {
     df->setTimeZone(*tz);
   }
 
@@ -194,7 +198,7 @@ const string TestDatetimeFmt(json_object *json_in) {
   json_object *input_millis = json_object_object_get(json_in, "input_millis");
 
   UDate test_date_time;
-  if (input_string_obj) {
+  if (input_string_obj != nullptr) {
     Locale und_locale("und");
 
     string input_date_string = json_object_get_string(input_string_obj);
@@ -219,7 +223,7 @@ const string TestDatetimeFmt(json_object *json_in) {
 
     // TODO:  handles the offset +/-
     SimpleDateFormat iso_date_fmt(u"y-M-d'T'h:m:sZ", und_locale, status);
-    if (U_FAILURE(status)) {
+    if (U_FAILURE(status) != 0) {
       string error_name = u_errorName(status);
       string error_message =
           "# iso_date_fmt constructor failure: " +
@@ -240,7 +244,7 @@ const string TestDatetimeFmt(json_object *json_in) {
                         "# iso_date_fmt parse failure" + input_date_string)) {
       return json_object_to_json_string(return_json);
     }
-  } else if (input_millis) {
+  } else if (input_millis != nullptr) {
     test_date_time = json_object_get_double(input_millis);
   } else {
     json_object_object_add(

diff --git a/executors/cpp/likely_subtags.cpp b/executors/cpp/likely_subtags.cpp
@@ -10,8 +10,8 @@
 #include <unicode/unistr.h>
 #include <unicode/utypes.h>
 
-#include <stdio.h>
-#include <stdlib.h>
+#include <cstdio>
+#include <cstdlib>
 
 #include <cstring>
 #include <iostream>
@@ -25,7 +25,7 @@ using icu::Locale;
 using icu::StringByteSink;
 using icu::UnicodeString;
 
-const string TestLikelySubtags(json_object *json_in) {
+string TestLikelySubtags(json_object *json_in) {
   UErrorCode status = U_ZERO_ERROR;
 
   json_object *label_obj = json_object_object_get(json_in, "label");
@@ -55,12 +55,12 @@ const string TestLikelySubtags(json_object *json_in) {
     Locale maximized(displayLocale);
     maximized.addLikelySubtags(status);
 
-    if (U_FAILURE(status)) {
+    if (U_FAILURE(status) != 0) {
       test_result = "error in maximize";
     } else {
       maximized.toLanguageTag(byteSink, status);
 
-      if (U_FAILURE(status)) {
+      if (U_FAILURE(status) != 0) {
         json_object_object_add(
             return_json,
             "error",
@@ -72,14 +72,14 @@ const string TestLikelySubtags(json_object *json_in) {
              option_string == "minimizeFavorRegion") {
     // Minimize
     displayLocale.minimizeSubtags(status);
-    if (U_FAILURE(status)) {
+    if (U_FAILURE(status) != 0) {
       const string error_message_min = "error in minimize";
       test_result = error_message_min;
     } else {
       displayLocale.toLanguageTag(byteSink, status);
       test_result = name_string;
 
-      if (U_FAILURE(status)) {
+      if (U_FAILURE(status) != 0) {
         json_object_object_add(
             return_json,
             "error",
@@ -120,7 +120,7 @@ const string TestLikelySubtags(json_object *json_in) {
         json_object_new_string(option_string.c_str()));
   }
 
-  if (U_FAILURE(status)) {
+  if (U_FAILURE(status) != 0) {
     json_object_object_add(
         return_json,
         "error",