Fix clang-tidy support for tests and extensibility #237

mlxd · 2022-02-14T18:24:43Z

Context: This closes #236 and #235

Description of the Change: Clang-tidy is enabled for CI tests in the formatter action, and subsequent clang-tidy suggestions are applied. Also, The return type for data is modified to become auto-definable at compile time.

Benefits:

Possible Drawbacks:

Related GitHub Issues:

github-actions · 2022-02-14T18:24:59Z

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

A one-to-two sentence description of the change. You may include a small working example for new features.
A link back to this PR.
Your name (or GitHub username) in the contributors section.

codecov · 2022-02-14T18:26:28Z

Codecov Report

Merging #237 (4ffa555) into master (86efa02) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #237   +/-   ##
=======================================
  Coverage   99.70%   99.70%           
=======================================
  Files           4        4           
  Lines         344      344           
=======================================
  Hits          343      343           
  Misses          1        1

Impacted Files	Coverage Δ
pennylane_lightning/_version.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86efa02...4ffa555. Read the comment docs.

github-actions · 2022-02-14T18:28:25Z

Test Report (C++) on Ubuntu

      1 files ±0       1 suites ±0 0s ⏱️ ±0s
  555 tests ±0   555 ✔️ ±0 0 💤 ±0 0 ❌ ±0
2 289 runs ±0 2 289 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 4ffa555. ± Comparison against base commit 86efa02.

♻️ This comment has been updated with latest results.

chaeyeunpark · 2022-02-14T19:30:35Z

Hi @mlxd, it seems like the current error is due to several unused parameters/variables. In the previous version, I have just suppressed those warnings (only for tests codes) by -Wno-unused as there are too many places with this warning and I thought they are not much harmful (even though some variables are unused, they provide some information e.g. const auto num_qubits = 3;). See the lines below.

pennylane-lightning/pennylane_lightning/src/tests/CMakeLists.txt

Lines 43 to 45 in 2cb2727

    
           target_compile_options(lightning_tests_dependency INTERFACE "-Wall;-Wno-unused;") 
        
           target_sources(lightning_tests_dependency INTERFACE runner_main.cpp)

mlxd · 2022-02-14T21:35:46Z

Hi @mlxd, it seems like the current error is due to several unused parameters/variables. In the previous version, I have just suppressed those warnings (only for tests codes) by -Wno-unused as there are too many places with this warning and I thought they are not much harmful (even though some variables are unused, they provide some information e.g. const auto num_qubits = 3;). See the lines below.

pennylane-lightning/pennylane_lightning/src/tests/CMakeLists.txt

Lines 43 to 45 in 2cb2727

target_compile_options(lightning_tests_dependency INTERFACE "-Wall;-Wno-unused;")

target_sources(lightning_tests_dependency INTERFACE runner_main.cpp)

Thanks @chaeyeunpark . I have added the casts to get over the failures for now. As discussed, we can look at adapting this to another structure later on.

maliasadi

Thanks @mlxd for fixing these issues.

pennylane_lightning/src/tests/Test_DynamicDispatcher.cpp

chaeyeunpark

Hi @mlxd, thanks for dealing with this problem. Could you just check one thing here? If clang-tidy still complains about it, let's just remove this whole function (it anyway does nothing now...).

chaeyeunpark · 2022-02-15T16:33:58Z

pennylane_lightning/src/tests/Test_OpToMemberFuncPtr.cpp

-                                      gate_op>::value == nullptr) {
-                return false;
-            }
-        }


Could you check clang-tidy still raises errors with these lines? Basically, this template function tests whether GateOpMemberFuncPtr is well defined (in a compile-time). If it is well-defined, as clang-tidy-11 says, it cannot be nullptr. Otherwise, it raises a compile-time error.

Hey @chaeyeunpark

Ah, I understand your intention now. Yea, I was getting errors with these locally, and removed them based on the clang-tidy feedback. I'll re-add and retest with the act package, and if so we can leave them as deleted for now.

Huh, seems to be passing now (weird, may have been a version issue locally). I'll re-add this in and push it back up. Sorry for the confusion.

Update: scratch that, fails locally on clang-tidy-13

I will do some tests with different environments to see if this is an easy mitigation.

As it cannot be nullptr anyway, I think replacing the internal if statement just to

static_cast<void>(GateOpToMemberFuncPtr<PrecisionT, ParamT, GateImplemenation, gate_op>::value);

will work (for clang-tidy-13).

Hey @chaeyeunpark sorry for taking so long to get back to this. That above suggestion does indeed fix the clang-tidy-13 complaint! Thanks for the suggestion.

chaeyeunpark

Happy to approve this 👍

chaeyeunpark · 2022-02-25T19:51:59Z

pennylane_lightning/src/tests/Test_OpToMemberFuncPtr.cpp

-                                      gate_op>::value == nullptr) {
-                return false;
-            }
-        }


mlxd · 2022-02-25T19:55:01Z

Thanks @chaeyeunpark & @maliasadi

Fix clang-tidy errors

27dfa78

Auto update version

061255c

Update changelog

9ca20ee

Update clang tidy and g++ versions

d28c81c

mlxd added 2 commits February 14, 2022 16:28

Ensure unused are cast away for conditional compiles

57058b6

Fix formatting

0f432a3

mlxd requested a review from chaeyeunpark February 14, 2022 21:34

mlxd requested a review from maliasadi February 14, 2022 21:47

Update build support for clang-format

bdb2694

maliasadi approved these changes Feb 15, 2022

View reviewed changes

pennylane_lightning/src/tests/Test_DynamicDispatcher.cpp Show resolved Hide resolved

chaeyeunpark suggested changes Feb 15, 2022

View reviewed changes

mlxd and others added 3 commits February 25, 2022 13:16

Merge branch 'master' into crtp_datatypes

0640216

Auto update version

475c6de

Add fixx for clang-tidy-13 recommendation

4ffa555

mlxd requested a review from chaeyeunpark February 25, 2022 18:49

chaeyeunpark approved these changes Feb 25, 2022

View reviewed changes

pennylane_lightning/src/tests/Test_OpToMemberFuncPtr.cpp

gate_op>::value == nullptr) {

return false;

}

}

Copy link

Contributor

chaeyeunpark Feb 25, 2022

Choose a reason for hiding this comment

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

Good!

mlxd merged commit 602352b into master Feb 25, 2022

mlxd deleted the crtp_datatypes branch February 25, 2022 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix clang-tidy support for tests and extensibility #237

Fix clang-tidy support for tests and extensibility #237

mlxd commented Feb 14, 2022

github-actions bot commented Feb 14, 2022

codecov bot commented Feb 14, 2022 •

edited

Loading

github-actions bot commented Feb 14, 2022 •

edited

Loading

chaeyeunpark commented Feb 14, 2022

mlxd commented Feb 14, 2022

maliasadi left a comment

chaeyeunpark left a comment

chaeyeunpark Feb 15, 2022

mlxd Feb 15, 2022

mlxd Feb 15, 2022

mlxd Feb 15, 2022

chaeyeunpark Feb 15, 2022 •

edited

Loading

mlxd Feb 25, 2022

chaeyeunpark Feb 25, 2022

chaeyeunpark left a comment

chaeyeunpark Feb 25, 2022

mlxd commented Feb 25, 2022

Fix clang-tidy support for tests and extensibility #237

Fix clang-tidy support for tests and extensibility #237

Conversation

mlxd commented Feb 14, 2022

github-actions bot commented Feb 14, 2022

codecov bot commented Feb 14, 2022 • edited Loading

Codecov Report

github-actions bot commented Feb 14, 2022 • edited Loading

Test Report (C++) on Ubuntu

chaeyeunpark commented Feb 14, 2022

mlxd commented Feb 14, 2022

maliasadi left a comment

Choose a reason for hiding this comment

chaeyeunpark left a comment

Choose a reason for hiding this comment

chaeyeunpark Feb 15, 2022

Choose a reason for hiding this comment

mlxd Feb 15, 2022

Choose a reason for hiding this comment

mlxd Feb 15, 2022

Choose a reason for hiding this comment

mlxd Feb 15, 2022

Choose a reason for hiding this comment

chaeyeunpark Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

mlxd Feb 25, 2022

Choose a reason for hiding this comment

chaeyeunpark Feb 25, 2022

Choose a reason for hiding this comment

chaeyeunpark left a comment

Choose a reason for hiding this comment

chaeyeunpark Feb 25, 2022

Choose a reason for hiding this comment

mlxd commented Feb 25, 2022

codecov bot commented Feb 14, 2022 •

edited

Loading

github-actions bot commented Feb 14, 2022 •

edited

Loading

chaeyeunpark Feb 15, 2022 •

edited

Loading