Addresses some odds and ends #1253

kennyweiss · 2024-01-05T05:28:47Z

Summary

This PR resolves some minor issues/warnings and applies clang-tidy rules to the code
Resolves Warning in ArrayBase with XL compiler #1223
Resolves clang-tidy warning w/ clang@16.0.6 #1248
- I couldn't reproduce the issue exactly, but I think the change should resolve it based on the description
It also (partially) applies the following clang-tidy rules. In some cases, I had to revert clang-tidy changes, e.g. when it modified the auto-generated shroud files:
- readability-braces-around-statements
- modernize-use-using
- readability-static-definition-in-anonymous-namespace
- modernize-use-nullptr

I didn't add it to the clang-tidy file to avoid it changing sidre auto-generated files.

…ce rule

I couldn't reproduce #1248, but this will likely resolve the issue.

rhornung67 · 2024-01-05T17:01:35Z

src/axom/core/Array.hpp

@@ -730,22 +730,22 @@ class Array : public ArrayBase<T, DIM, Array<T, DIM, SPACE>>
                  "Cannot call Array<T>::resize() when T is non-trivially-"
                  "constructible. Use Array<T>::reserve() and emplace_back()"
                  "instead.");
-    const StackArray<IndexType, DIM> dims {static_cast<IndexType>(args)...};
+    const StackArray<IndexType, DIM> dims {{static_cast<IndexType>(args)...}};


👍

here and throughout

Is there a reason we don't apply the args with the integral type that is given?

I assume this allows users to supply signed/unsigned integral input and convert it to the represented axom::IndexType (and avoid warnings/compilation errors due to the templated types)

@publixsubfan might have some additional context.

rhornung67

Looks good. I just have a variety of minor questions.

rhornung67 · 2024-01-05T17:03:26Z

src/axom/core/Array.hpp

@@ -730,22 +730,22 @@ class Array : public ArrayBase<T, DIM, Array<T, DIM, SPACE>>
                  "Cannot call Array<T>::resize() when T is non-trivially-"
                  "constructible. Use Array<T>::reserve() and emplace_back()"
                  "instead.");
-    const StackArray<IndexType, DIM> dims {static_cast<IndexType>(args)...};
+    const StackArray<IndexType, DIM> dims {{static_cast<IndexType>(args)...}};


Is there a reason we don't apply the args with the integral type that is given?

src/axom/mint/examples/mint_heat_equation.cpp

rhornung67 · 2024-01-05T17:05:03Z

src/axom/mint/fem/FiniteElement.hpp

@@ -499,7 +499,7 @@ class FiniteElement
  /// \name Reference Element Attributes
  /// @{

-  typedef void (*ShapeFunctionPtr)(const double* lc, double* phi);
+  using ShapeFunctionPtr = void (*)(const double* lc, double* phi);


👍

here and throughout

rhornung67 · 2024-01-05T17:09:21Z

src/axom/primal/operators/winding_number.hpp

@@ -657,9 +657,13 @@ double winding_number(const Point<T, 3>& query,
    // Find the direction of a ray perpendicular to that
    Vector<T, 3> v1;
    if(axom::utilities::isNearlyEqual(v0[0], v0[1], EPS))
+    {


Much better. this is a clang-tidy change, correct?

rhornung67 · 2024-01-05T17:10:07Z

src/axom/primal/tests/primal_octahedron.cpp

-  typedef double CoordType;
-  typedef primal::Point<CoordType, DIM> QPoint;
-  typedef primal::Octahedron<CoordType, DIM> QOct;
+  using CoordType = double;


is it possible to have a chang-tidy rule to catch this sort of thing?

These typedef -> using changes were from the modernize-use-using rule

That was actually the impetus for revisiting clang-tidy in this PR -- I had been manually updating the typedefs when I updated old primal test files (e.g. in #1251) and thought clang-tidy would have a modernize-* rule to automate it.

rhornung67 · 2024-01-05T17:10:50Z

src/axom/quest/tests/quest_c2c_reader.cpp

-static const std::string C2C_CIRCLE_FILENAME = "test_circle.contour";
-static const std::string C2C_SQUARE_FILENAME = "test_square.contour";
-static const std::string C2C_SPLINE_FILENAME = "test_spline.contour";
+const std::string C2C_LINE_FILENAME = "test_line.contour";


constexpr for these?

:) I tried that too and got an error about constexpr and strings.

Looks like we'll have to wait for std::string_view in C++17 -- https://stackoverflow.com/questions/27123306/is-it-possible-to-use-stdstring-in-a-constant-expression

rhornung67 · 2024-01-05T17:11:47Z

src/axom/sidre/tests/sidre_opaque_C.cpp

@@ -25,11 +25,11 @@ enum DType
  _UnknownType_
 };

-typedef struct
+using AA_extent = struct


can clang-tidy catch these?

yes! (see comment above)

rhornung67 · 2024-01-05T17:13:07Z

src/axom/slam/tests/slam_set_RangeSet.cpp

-static const SetElement upperIndex = static_cast<SetElement>(.7 * MAX_SIZE);
-static const SetElement range = upperIndex - lowerIndex;
+constexpr SetPosition MAX_SIZE = 20;
+constexpr SetElement lowerIndex = static_cast<SetElement>(.3 * MAX_SIZE);


Suggested change

constexpr SetElement lowerIndex = static_cast<SetElement>(.3 * MAX_SIZE);

constexpr SetElement lowerIndex(.3 * MAX_SIZE);

Does this do the same thing? If so, it is less verbose.

I think it probably makes sense to keep the static_cast here do explicitly highlight the cast from double

BradWhitlock

Let there be "using"!

format-robot and others added 9 commits January 4, 2024 16:39

Applies clang-tidy rules to the code

bd07107

Adds modernize-use-using rule to clang-tidy

1e98235

Only apply clang-tidy rules to c++ files

2fd614d

Applies clang-tidy modernize-use-using rule

4a3a27a

Applies clang-tidy modernize-use-nullptr rule

845a13a

I didn't add it to the clang-tidy file to avoid it changing sidre auto-generated files.

Applies clang-tidy readability-static-definition-in-anonymous-namespa…

cf34c4e

…ce rule

Adds std namespace to a uint32_t type

f0c6215

I couldn't reproduce #1248, but this will likely resolve the issue.

Fixes initialization warnings w/ XL compiler

b7a7831

Adds comments to clang-tidy config file

a88ffaf

kennyweiss added the maintenance Issues related to code maintenance label Jan 5, 2024

kennyweiss requested review from white238, cyrush, BradWhitlock, rhornung67, publixsubfan, gunney1, agcapps and bmhan12 January 5, 2024 05:28

kennyweiss self-assigned this Jan 5, 2024

rhornung67 reviewed Jan 5, 2024

View reviewed changes

rhornung67 approved these changes Jan 5, 2024

View reviewed changes

BradWhitlock approved these changes Jan 5, 2024

View reviewed changes

Manually fixes some clang-tidy changes

030edb3

kennyweiss merged commit 65dfc3a into develop Jan 8, 2024
12 checks passed

kennyweiss deleted the feature/kweiss/odds-and-ends branch January 8, 2024 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addresses some odds and ends #1253

Addresses some odds and ends #1253

kennyweiss commented Jan 5, 2024

rhornung67 Jan 5, 2024 •

edited

Loading

rhornung67 Jan 5, 2024

kennyweiss Jan 8, 2024

rhornung67 left a comment

rhornung67 Jan 5, 2024

rhornung67 Jan 5, 2024

rhornung67 Jan 5, 2024

rhornung67 Jan 5, 2024

kennyweiss Jan 8, 2024

kennyweiss Jan 8, 2024

rhornung67 Jan 5, 2024

kennyweiss Jan 8, 2024

rhornung67 Jan 5, 2024

kennyweiss Jan 8, 2024

rhornung67 Jan 5, 2024

kennyweiss Jan 8, 2024

BradWhitlock left a comment

	constexpr SetElement lowerIndex = static_cast<SetElement>(.3 * MAX_SIZE);
	constexpr SetElement lowerIndex(.3 * MAX_SIZE);

Addresses some odds and ends #1253

Addresses some odds and ends #1253

Conversation

kennyweiss commented Jan 5, 2024

Summary

rhornung67 Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhornung67 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BradWhitlock left a comment

Choose a reason for hiding this comment

rhornung67 Jan 5, 2024 •

edited

Loading