[PATCH] D97403: [flang] updated character tests to gtest

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thank you for updating this, @ashermancinelli! You've increased the test coverage for `Verify{N}` and `Scan{N}` methods - that's great to see.

I agree with your design decisions and I don't see any obvious ways to improve this. IMHO, this implementation is relatively easy to follow - you've done a really good job here!

I've just realized one thing.  ALL_CAPS is usually reserved for macros (though I couldn't find this in LLVM's coding style docs). When specifying type aliases with `using`, I believe that we should use `CamelCase` instead. That's based on what's seem to be most widely used inside Flang. So, for example,

- `VerifyParameterTy` instead of `VERIFY_PARAMETER`
- `VerifyFuncTy` instead of `VERIFY_FUNC`

Apologies for not raising this earlier. I really liked your original approach, but we should follow the existing precedent in the project (I couldn't find any explicit notes on `using`, neither here <https://github.com/llvm/llvm-project/blob/main/flang/docs/C%2B%2Bstyle.md> nor here <https://llvm.org/docs/CodingStandards.html>).

Few more comments/suggestions inline.



================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:57
+template <typename CHAR>
+using COMPARISON_FUNC =
+    std::function<int(const CHAR *, const CHAR *, std::size_t, std::size_t)>;
----------------
Perhaps `ComparisonFuncTy`?


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:61
+static std::tuple<COMPARISON_FUNC<char>, COMPARISON_FUNC<char16_t>,
+    COMPARISON_FUNC<char32_t>>
+    ComparisonFuncs{
----------------
[nit] Would it be worth adding a comment here to improve visibility e.g. `The list of CharacterCompareScalar{N} functions tested here`? It's the key bit, isn't it? Same in other places.


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:70
+template <typename CHAR>
+using COMPARISON_PARAMETER =
+    std::tuple<const CHAR *, const CHAR *, int, int, int>;
----------------
[nit] PARAMETER --> PARAMETERS (same in other places)


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:153
+template <typename CHAR>
+using SCAN_PARAMETER = std::tuple<const CHAR *, const CHAR *, bool, int>;
+
----------------
`ScanParameterTy` instead?


================
Comment at: flang/unittests/RuntimeGTest/CharacterTest.cpp:197
+    auto res{
+        this->characterScanFunc(str, std::char_traits<TypeParam>::length(str),
+            set, std::char_traits<TypeParam>::length(set), back)};
----------------
ashermancinelli wrote:
> AFAIK, `char_traits<>::length` is exactly the same as `strlen` but I can't verify this on cppref. If someone can verify that would be great.
Yes, that's how I read [[ https://en.cppreference.com/w/cpp/string/char_traits/length | this ]].


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97403/new/

https://reviews.llvm.org/D97403