Refactor `TargetRegistry` to a templatized `PluginRegistry` and concrete `TargetSystemRegistry` #63

jgsogo · 2023-03-01T12:50:00Z

Together with #62, it substitutes most of #58

Here I'm adding a new PluginRegistry<PluginInfo> class to implement a conventional registry-factory for PluginInfo items. On top of this registry I'm implementing a TargetSystemRegistry that creates not only TargetSystemInfo instances, but TargetSystem ones with some cache-like functionality based on mlir::MLIRContext input.

This refactor supports a future PayloadRegistry as a specialization of PluginRegistry.

All existing functionality is preserved.

TODO:

Bring back all the comments
Remove commented lines
Apply some clang-format (?)

kitbarton

This looks like a great start! I think this is going to work well.
I just had one comment about namespace blocks in source files, but aside from that things look good so far.

lib/HAL/TargetSystemInfo.cpp

jgsogo · 2023-03-01T18:24:07Z

This is not working and, honestly, I don't really know why it was working before.

In the main branch, before this PR, and focusing on qss-opt which is listing the Available Targets: The auto registration is based on some function init() being called before main. We expect this method to be called because we have [[maybe_unused]] int registrar = init(); in a Target.inc file. This file is included from Targets.inc which is included from TargetRegistry.cpp. TargetRegistry.cpp is compiled into HAL library which is linked into qss-opt executable.

Everything are static libraries, so the linker should discard everything that is not used... and that init() is not used anywhere.

The only file in the qss-opt executable (qss-opt.cpp) includes TargetRegistry.h, but it doesn't see the .inc files that registers our mock target... so those symbols are discarded.

I'm missing something here and it's getting a bit late. Maybe I just need to walk away from the source code a little bit, maybe someone has the answer and it is evident. Right now I'm a bit confused.

jgsogo · 2023-03-02T11:26:20Z

Thanks @mhillenbrand 🙌 🙌

We need to be sure that Targets.inc is not included from a TU that could be optimized. 75c552f moves the include to TargetSystemInfo.cpp which won't be optimized as long as we are using TargetSystemInfo instances (the TU for the registry contains just one method that won't be used in many scenarios)

…-template

vrpascuzzi

Awesome addition.

A number of requested changes, but mostly non-functional.

include/Plugin/PluginRegistry.h

include/Plugin/PluginInfo.hpp

include/HAL/TargetSystemRegistry.h

vrpascuzzi · 2023-03-02T16:06:57Z

include/HAL/TargetSystem.h

@@ -56,6 +56,9 @@ class Target {
 };

 class TargetSystem : public Target {
+public:
+    using PluginConfiguration = llvm::StringRef;


Used anywhere?

Update copyright.

It is used in PluginInfo:

using PluginConfiguration = typename PluginType::PluginConfiguration;

A bit convoluted?

Why is this necessary?

lib/HAL/PassRegistration.cpp

include/HAL/TargetSystemInfo.h

mock_target/Target.inc

test/unittest/CMakeLists.txt

test/unittest/HAL/TargetSystemRegistryTest.cpp

steleman · 2023-03-08T14:11:16Z

include/HAL/TargetSystem.h

@@ -56,6 +56,9 @@ class Target {
 };

 class TargetSystem : public Target {
+public:
+    using PluginConfiguration = llvm::StringRef;


This is unnecessary and creates obfuscated code.

It is used in the generic template<typename TPluginType> PluginInfo, other TPluginType (Payloads, which is the final purpose of this change) will declare their own PluginConfiguration type.

This is entirely unnecessary. My question wasn't where is this used?. My question was why is this necessary?

/// Returns a new instance of the registered PluginType llvm::Expected<std::unique_ptr<PluginType>> createPluginInstance(llvm::Optional<PluginConfiguration> configuration = llvm::None) { return factoryFunction(configuration); }

Every PluginType requires a different PluginConfiguration. In this PR, we can see that TargetSystem requires a llvm::StringRef, but a Payload will require None.

When PluginInfo is instantiated for the actual type, this method is instantiated with the corresponding PluginConfiguration type.

Why is the argument to createPluginInstance a llvm::Optional<PluginConfiguration> with a default value?

The fact that the function argument has a default value proves that it's not optional at all. If it's not optional, why is it wrapped inside a llvm::Optional?

Why does a std::unique_ptr need to be wrapped in a llvm::Expected?

std::unique_ptr already provides its only two possible states: either it's null, or it contains something. What role does llvm::Expected play here?

IMO this design is inherently broken.

Why is the argument to createPluginInstance a llvm::Optional<PluginConfiguration> with a default value?

The fact that the function argument has a default value proves that it's not optional at all. If it's not optional, why is it wrapped inside a llvm::Optional?

I agree on this part. I added a default to simplify usage, but you are right. Removing the default argument as it's not needed 👍

Why does a std::unique_ptr need to be wrapped in a llvm::Expected?

std::unique_ptr already provides its only two possible states: either it's null, or it contains something. What role does llvm::Expected play here?

IMO this design is inherently broken.

A std::unique_ptr tells about ownership. In the previous code things were much worse, as it was a raw pointer (it says nothing about ownership), now the std::unique_ptr at least is passing explicitly ownership to the caller.

llvm::Expected is not about ownership, but about returning errors https://llvm.org/doxygen/classllvm_1_1Expected.html

I could have written this method differently, but the interface (expected + pointer) was already there (blame to 6 months ago, with the initial commit). I will support you if you advocate to spend one sprint paying existing technical debt, please, keep me on that loop. Thanks!

This is not an appropriate response to a code review comment.

If every single comment in every code review was solved by saying you fix it because I'm not gonna, why bother with code reviews in the first place.

steleman · 2023-03-08T14:14:14Z

include/HAL/TargetSystemInfo.h

+  /// and a description.
+  class TargetSystemInfo : public qssc::plugin::registry::PluginInfo<qssc::hal::TargetSystem> {
+    using PluginInfo = qssc::plugin::registry::PluginInfo<qssc::hal::TargetSystem>;
+    using PassesFunction = std::function<llvm::Error()>;


Why are there two different using directives pointing to the same exact definition?

using PassesFunction = std::function<llvm::Error()>; using PassPipelinesFunction = std::function<llvm::Error()>;

More importantly: why does llvm::Error() need to be wrapped in std::function?
Can llvm::Error() be called directly, without two extra layers of indirection?

Why repeated definition? I preferred not to modify existing code (below it is the snippet from removed TargetRegistry.h). Of course I can modify it, but I didn't want to pay (unrelated) tech debt in this PR

About the type: this is declaring a function that returns an llvm::Error, it is not a function call, there is no indirection here.

using TargetRegisterPassesFunction = std::function<llvm::Error()>; using TargetRegisterPassPipelinesFunction = std::function<llvm::Error()>;

This is wrapping a constructor call to llvm::Error() inside a std::function<>. So yes, it is an indirection to a function call. Why is this necessary?

No, it is a function wrapper: https://en.cppreference.com/w/cpp/utility/functional/function

None of this answers my original question.

Original question:

Why are there two different using directives pointing to the same exact definition?

I can trace those two lines back to December 16th, 2021. The person who wrote that PR and those who reviewed and approved can give you the answer you are looking for. Here I was just answering to your "More importantly" part.

This PR is not trying to pay technical debt, it is trying to introduce a generic PluginRegistry modifying the rest of the existing code as less as possible.

This PR is not trying to pay technical debt, it is trying to introduce a generic PluginRegistry modifying the rest of the existing code as less as possible.

I do not understand what this means.

There are serious problems in the existing code, as well as in the new code you are proposing. The new code that you are trying to introduce is based on the older, existing code.

Are you suggesting that none of these problems should be addressed because they are (a) technical debt or (b) would require changes?

If code reviews shouldn't require code changes, why bother having them?

As a general comment, and in response to your previous responses to my comments and change requests: I am unable to approve your PR in its current form. There are serious issues with this PR that have not been addressed. Whether these issues originate in older code, or they are created by new code, is irrelevant.

include/Plugin/PluginRegistry.h

steleman · 2023-03-08T14:24:24Z

mock_target/MockTarget.cpp

+          llvm::inconvertibleErrorCode(),
+          "Configuration file must be specified.\n");
+
+    auto config = std::make_unique<MockConfig>(*configurationPath);


Why is this auto when its type is already known?

easier to read

https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

Here it applies "do use auto with initializers like cast(...) or other places where the type is already obvious from the context.".

I'm not advocating for using auto always. In fact I barely use it in my projects unless it makes it easier to read (container's typedef, nested template declarations,...)

Use auto if and only if it makes the code more readable or easier to maintain.

Which it almost never does.

auto was invented and introduced in C++11 for one and only one purpose: lambda captures.

Any use of auto outside of the lambda capture use case is unjustified. It makes everything hard to read.

And it does not apply in this case at all. The return type of std::make_unique is known. There is no need for auto abuse.

We have clang-tidy set up to enforce this idiom. It's simply less characters to read.

The return type of std::make_unique is known. There is no need for auto abuse.

steleman · 2023-03-08T14:30:03Z

mock_target/MockTarget.cpp

+          "Configuration file must be specified.\n");
+
+    auto config = std::make_unique<MockConfig>(*configurationPath);
+    return std::make_unique<MockSystem>(std::move(config));


This seems very inefficient. Why does it need a lvalue-to-rvalue-conversion (std::move)?

it was there before (I'm not paying existing tech debt here), but copy/move elision should do its work

This code creates a std::unique_ptr - an lvalue - that is subsequently std::move'd to another std::unique_ptr.

How does copy/move elision do its work in this case? Because I don't see any copy or move elisions here. I do see a lvalue-to-rvalue-conversion, which is always expensive, because it involves constructing and destructing an on-the-fly xvalue.

MockSytem takes a std::unique_ptr<MockConfig>, it's probably not needed to mark it as movable because the compiler will probably optimize it (but the std::move added months ago is harmless). Then the method returns a std::unique_ptr<MockSystem> and copy/move elision will optimize it for sure.

Con you link a godbolt example to probe your point?

I don't know what probably means. Could you please explain?

Please explain how this probable optimization works.

Please explain how move semantics operate on an lvalue. There is no need for code or godbolt for that. It's all in the move semantics rules.

Can you link a godbolt example to probe your point?

No. I am reviewing your code. Not the other way around.

jgsogo

// vs. ///

Those are doxygen (documentation), not comments. Shall I remove them?

Copyright notices

We need to thing somethingn about it. They can kill any OSS contributor 😅

vrpascuzzi · 2023-03-08T15:09:31Z

Copyright notices

We need to thing somethingn about it. They can kill any OSS contributor 😅

I don't know what you mean here...

steleman · 2023-03-08T15:22:25Z

include/HAL/TargetSystem.h

@@ -56,6 +56,9 @@ class Target {
 };

 class TargetSystem : public Target {
+public:
+    using PluginConfiguration = llvm::StringRef;


Why is this necessary?

steleman · 2023-03-08T15:23:24Z

include/HAL/TargetSystem.h

@@ -56,6 +56,9 @@ class Target {
 };

 class TargetSystem : public Target {
+public:
+    using PluginConfiguration = llvm::StringRef;


This is entirely unnecessary. My question wasn't where is this used?. My question was why is this necessary?

steleman · 2023-03-08T15:27:56Z

include/HAL/TargetSystemInfo.h

+  /// and a description.
+  class TargetSystemInfo : public qssc::plugin::registry::PluginInfo<qssc::hal::TargetSystem> {
+    using PluginInfo = qssc::plugin::registry::PluginInfo<qssc::hal::TargetSystem>;
+    using PassesFunction = std::function<llvm::Error()>;


This is wrapping a constructor call to llvm::Error() inside a std::function<>. So yes, it is an indirection to a function call. Why is this necessary?

include/Plugin/PluginRegistry.h

jgsogo · 2023-03-08T15:48:05Z

Copyright notices

We need to thing somethingn about it. They can kill any OSS contributor 😅

I don't know what you mean here...

When qss-compiler goes open-source we should expect many new contributors. This is the first time contributing to OSS where I've seen these copyright notices that need to be modified whenever a file is changed (and reviewers note about it! kudos, @vrpascuzzi). Contributors can find these details very cumbersome. They are important when they are in place, but IMHO we should think about something else.

vrpascuzzi · 2023-03-08T15:50:34Z

copyright notices that need to be modified whenever a file is changed

This is not true. They only need to be updated when the file is updated in a later year.

taalexander · 2023-03-08T15:53:39Z

Contributors can find these details very cumbersome. They are important when they are in place, but IMHO we should think about something else.

Copyright headers is an IBM requirement. Regarding, the license header - these are new (#69) and is a common template for Qiskit projects.

jgsogo · 2023-03-08T16:02:30Z

Copyright headers is an IBM requirement. Regarding, the license header - these are new (#69) and is a common template for Qiskit projects.

Maybe they can be added somehow to some pre-commit hook, it would simplify things a lot. wdyt?

taalexander · 2023-03-08T16:05:37Z

Maybe they can be added somehow to some pre-commit hook, it would simplify things a lot. wdyt?

If we could this would be great 🚀 , at the very least an issue to capture this idea so we don't forget.

steleman · 2023-03-08T16:23:02Z

mock_target/MockTarget.cpp

+          "Configuration file must be specified.\n");
+
+    auto config = std::make_unique<MockConfig>(*configurationPath);
+    return std::make_unique<MockSystem>(std::move(config));


This code creates a std::unique_ptr - an lvalue - that is subsequently std::move'd to another std::unique_ptr.

How does copy/move elision do its work in this case? Because I don't see any copy or move elisions here. I do see a lvalue-to-rvalue-conversion, which is always expensive, because it involves constructing and destructing an on-the-fly xvalue.

steleman · 2023-03-08T16:23:08Z

mock_target/MockTarget.cpp

+          llvm::inconvertibleErrorCode(),
+          "Configuration file must be specified.\n");
+
+    auto config = std::make_unique<MockConfig>(*configurationPath);


Use auto if and only if it makes the code more readable or easier to maintain.

Which it almost never does.

auto was invented and introduced in C++11 for one and only one purpose: lambda captures.

Any use of auto outside of the lambda capture use case is unjustified. It makes everything hard to read.

And it does not apply in this case at all. The return type of std::make_unique is known. There is no need for auto abuse.

steleman · 2023-03-08T19:03:56Z

include/Plugin/PluginInfo.h

+
+    ~PluginInfo() = default;
+
+    [[nodiscard]] llvm::StringRef getName() const { return name; }


[[nodiscard]] is unnecessary, and, in fact, a loophole.

Can you elaborate what the problem here is, please? Maybe it's worth reporting the issue upstream to the linters. Thanks!

Please explain what [[nodiscard]] does in this case, and why you believe it should be present here.

This is a review of the code you are proposing.

This method is useless unless you use the returned value. With [[nodiscard]] we get a compiler warning if our code ignores the returned value (so we remove and clean the line). This [[nodiscard]] addition is also suggested by linters and I'm not smarter than them.

I honestly asking you about the loophole and the problem, because I don't know what could be the problem. I'm also here to learn, so, can you describe the problem, please?

The general problem with [[nodiscard]] for function return values is that it's pointless:

#include <iostream> #include <string> class Test { public: Test() : Name() { } Test(const char* N) : Name(N) { } [[nodiscard]] const std::string& getName() const { return Name; } [[nodiscard]] std::string getNameByValue() const { return Name; } const std::string& GetName() const { return Name; } const std::string GetNameByValue() const { return Name; } private: std::string Name; }; int main() { Test T("Test"); (void) T.getName(); T.GetName(); (void) T.getNameByValue(); T.GetNameByValue(); return 0;

[steleman@pettyofficer][~/tmp][03/09/2023 10:20:36][2467]>> g++ -g -O2 -std=c++17 -Wall -Wextra -Wpedantic nodiscard.cpp -o nodiscard [steleman@pettyofficer][~/tmp][03/09/2023 10:20:39][2468]>> echo $status 0 [steleman@pettyofficer][~/tmp][03/09/2023 10:20:41][2469]>> g++ --version g++ (GCC) 11.3.1 20220421 (Red Hat 11.3.1-3) Copyright (C) 2021 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

It's exactly the same with clang:

[steleman@pettyofficer][~/tmp][03/09/2023 10:23:27][2470]>> clang++ -g -O2 -std=c++17 -Wall -Wextra -Wpedantic nodiscard.cpp -o nodiscard [steleman@pettyofficer][~/tmp][03/09/2023 10:23:34][2471]>> echo $status 0 [steleman@pettyofficer][~/tmp][03/09/2023 10:23:39][2472]>> clang++ --version clang version 13.0.1 (Fedora 13.0.1-1.fc35) Target: x86_64-redhat-linux-gnu Thread model: posix InstalledDir: /usr/bin

The loophole is in the false sense of security it creates. The only thing it achieves in practice is cluttering the code with things that appear to do something useful, but don't.

The only way [[nodiscard]] would have been useful is if it disallowed the cast to (void). But it doesn't.

But, that's the point of [[nodiscard]], isn't it? If you cast to void you are using the value... Of course we can't prevent anyone cheating himself if this is a external interface, but at least our internal codebase will benefit from it: we as reviewers will block any (void) cast, the compiler will fail if the return value is unused.

Besides all this reasoning, I have to say that this project enforces this usage of [[nodiscard]] given this rule in clang-tidy (https://github.com/Qiskit/qss-compiler/blob/main/.clang-tidy#L72). The CI should fail if this attribute is not there. If you think we should remove this rule from the .clang-tidy file it's fine, but IMO it requires some broader consensus from the team (cc/ @kitbarton ).

If you cast to void you are using the value...

[citation needed]

void is not a value of any kind by definition. It can't be.

I believe that enforcing nodiscard in .clang-tidy is misplaced. Its primary concern appears to be modern, which a term best applied to fashion.

Please explain we as reviewers will block any (void) cast. What does this mean?

With a really big hammer, everything's a nail.

The point of this entire discussion is your PR code review. We are not making progress towards approval.

If you cast to void you are using the value...

[citation needed]

void is not a value of any kind by definition. It can't be.

Casting to void is an operation, it uses the value. In fact, the purpose of it is to bypass the warning emitted by -Wunused-variable (now it's preferred the [[maybe_unused]] attribute). I'm curious if you have ever seen a cast to void somewhere with any other purpose, I would love to know that example and include it in some talk.

I believe that enforcing nodiscard in .clang-tidy is misplaced. Its primary concern appears to be modern, which a term best applied to fashion.

I don't decide the rules that are enforced in this repository. If this is something we are going to reconsider, I will be happy to contribute.

Please explain we as reviewers will block any (void) cast. What does this mean?

If we review a PR that contains that cast ((void)) we should really pay attention to it and it's probably an example where we block the PR to request it to be changed. As you said, this is what code reviews are for.

The point of this entire discussion is your PR code review. We are not making progress towards approval.

We can create the corresponding issues and PRs to modify the existing codebase so this PR contains just the changes related to this issue and it can get your approval. That's a perfect valid point if you feel like those refactors and technical debt need to be addressed first 👍

I believe I have been very clear about what needs to be addressed in this PR:

Pathological abuse of auto.

Unexplained duplicate using directives pointing to the same exact declaration without explanation.

Unexplained - and unnecessary - use of std::move on a temporary lvalue that relies on an assumed to be probable compiler optimization that hasn't been fully explained.

Unconvincing - and in my view pointless - use of [[nodiscard]].

The .clang-tidy conversation is for another day, in a different PR.

https://github.com/llvm/llvm-project/blob/main/.clang-tidy

I think the use of nodiscard here is reasonable, for the reasons provided above.

My understanding of the attribute is to provide a mechanism for compilers to issue a warning when the callers of a function are ignoring return values that the callee does not think it should ignore. The way out of that is to cast to void, thereby alleviating the warning. The escape mechanism was likely added to the standard to give people an out, but it needs to be deliberately added, instead of (possibly accidentally) ignoring an important return value. If the cast to void is inappropriate, it can hopefully be caught in code reviews.

I don't think it adds visual clutter or noise. You are stating in the program, "pay attention to me, I'm important", and if you really don't want to, then you can cast to void to ignore it. I think this is far more future proof then putting these types of information in comments.

steleman · 2023-03-08T19:04:17Z

include/Plugin/PluginInfo.h

+
+    [[nodiscard]] llvm::StringRef getName() const { return name; }
+
+    [[nodiscard]] llvm::StringRef getDescription() const { return description; }


[[nodiscard]] is unnecessary, and, in fact, a loophole.

Same as above.

Please explain what [[nodiscard]] does in this case, and why you believe it should be present.

This is a review of the code you are proposing.

steleman · 2023-03-09T13:32:25Z

include/HAL/TargetSystem.h

@@ -56,6 +56,9 @@ class Target {
 };

 class TargetSystem : public Target {
+public:
+    using PluginConfiguration = llvm::StringRef;


This is not an appropriate response to a code review comment.

If every single comment in every code review was solved by saying you fix it because I'm not gonna, why bother with code reviews in the first place.

steleman · 2023-03-09T13:41:34Z

include/HAL/TargetSystemInfo.h

+  /// and a description.
+  class TargetSystemInfo : public qssc::plugin::registry::PluginInfo<qssc::hal::TargetSystem> {
+    using PluginInfo = qssc::plugin::registry::PluginInfo<qssc::hal::TargetSystem>;
+    using PassesFunction = std::function<llvm::Error()>;


This PR is not trying to pay technical debt, it is trying to introduce a generic PluginRegistry modifying the rest of the existing code as less as possible.

I do not understand what this means.

There are serious problems in the existing code, as well as in the new code you are proposing. The new code that you are trying to introduce is based on the older, existing code.

Are you suggesting that none of these problems should be addressed because they are (a) technical debt or (b) would require changes?

If code reviews shouldn't require code changes, why bother having them?

As a general comment, and in response to your previous responses to my comments and change requests: I am unable to approve your PR in its current form. There are serious issues with this PR that have not been addressed. Whether these issues originate in older code, or they are created by new code, is irrelevant.

steleman · 2023-03-09T13:42:24Z

include/Plugin/PluginInfo.h

+
+    ~PluginInfo() = default;
+
+    [[nodiscard]] llvm::StringRef getName() const { return name; }


Please explain what [[nodiscard]] does in this case, and why you believe it should be present here.

This is a review of the code you are proposing.

steleman · 2023-03-09T13:45:19Z

include/Plugin/PluginInfo.h

+
+    [[nodiscard]] llvm::StringRef getName() const { return name; }
+
+    [[nodiscard]] llvm::StringRef getDescription() const { return description; }


Please explain what [[nodiscard]] does in this case, and why you believe it should be present.

This is a review of the code you are proposing.

steleman · 2023-03-09T13:50:07Z

mock_target/MockTarget.cpp

+          "Configuration file must be specified.\n");
+
+    auto config = std::make_unique<MockConfig>(*configurationPath);
+    return std::make_unique<MockSystem>(std::move(config));


I don't know what probably means. Could you please explain?

Please explain how this probable optimization works.

Please explain how move semantics operate on an lvalue. There is no need for code or godbolt for that. It's all in the move semantics rules.

Can you link a godbolt example to probe your point?

No. I am reviewing your code. Not the other way around.

jgsogo

@steleman, I really welcome code reviews and I think they are the best way to contribute to a project and involve all the team into the development.

I think PRs should be small and focusing on one thing at a time. That way reviews are much easier and commit history is much cleaner. I really think that modifying just the things related to the actual feature or refactor being implemented and preserving the rest untouched is the right way to do things when contributing to any project in Github. Behind a PR title there should be just changes corresponding to that title.

Following that principle, I tried to keep existing code untouched, even if I think it can be improved. We can always open issues for those things and let other people contribute those changes. So, please, go ahead, and open issues for existing bugs or pending refactors you have detected. Maybe, this PR is never merged, or it takes months until it is merged and all those suggestions over existing sources can be contributed much earlier if they go via different and small PRs.

If this is not the way to go when contributing to qss-compiler and we should do the opposite... well, I would really like to read about the pros and cons of each approach and understand why one way is preferred over the other in this context.

kitbarton · 2023-03-09T22:18:54Z

I agree with @jgsogo on the approach to small, focused PRs, for all the reasons highlighted. I do not see any official policy supporting this in Qiskit, but I think it is something we should consider adding as we get closer to open sourcing.

In the spirit of that, I would suggest that we focus the review comments here to only changes made in this PR. If people see examples of egregious (existing) code during a code review, please open an issue with details or submit a PR to fix it. I believe this is in line with the LLVM developer/code review policy, and probably other OSS projects as well.

Futhermore, I think the use of of auto and [[nodiscard]] in this PR are acceptable based on the code style we have adopted and are trying to enforce with clang-tidy. Again, if people feel these are guidelines are not acceptable I think that should be discussed outside of this PR.

steleman · 2023-03-10T00:23:23Z

I agree with @jgsogo on the approach to small, focused PRs, for all the reasons highlighted. I do not see any official policy supporting this in Qiskit, but I think it is something we should consider adding as we get closer to open sourcing.

In the spirit of that, I would suggest that we focus the review comments here to only changes made in this PR. If people see examples of egregious (existing) code during a code review, please open an issue with details or submit a PR to fix it. I believe this is in line with the LLVM developer/code review policy, and probably other OSS projects as well.

Futhermore, I think the use of of auto and [[nodiscard]] in this PR are acceptable based on the code style we have adopted and are trying to enforce with clang-tidy. Again, if people feel these are guidelines are not acceptable I think that should be discussed outside of this PR.

I disagree. This pseudo-incremental let's fix it later approach is a recipe for carrying on a technical legacy that will never be corrected. The only thing we're doing is accumulating problems that get recorded.

Let's face it: nobody is going to go back and correct the pervasive abuse of auto, or the duplicate using directives that refer to the same thing in this PR. Once this PR is accepted in its current form, it will stay that way - broken - forever.

If this is the approach we are going to embrace from now on, then any PR that does not appear egregiously broken will be deemed ready for inclusion.

I also take exception with the comparison with LLVM, having contributed quite a bit there. They do not accept broken code.

mtreinish · 2023-03-10T01:16:21Z

I do not see any official policy supporting this in Qiskit, but I think it is something we should consider adding as we get closer to open sourcing.

It's not documented explicitly as part of the contribution guidelines (it used to be implicitly documented in the sections of for commit messages, see here: https://qiskit.org/documentation/stable/0.35/contributing_to_qiskit.html#commit-messages, specifically the "Read the commit message to see if it hints at improved code structure" sub section). That being said this philosophy is definitely the preference in the larger Qiskit project. It is generally better to have smaller PRs with a single logical change. This both makes it much easier to review and also maintain a git history that is easier to bisect errors or revert changes in isolation.

github-actions

clang-tidy made some suggestions

include/Plugin/PluginRegistry.h

lib/API/api.cpp

github-actions · 2023-03-15T11:33:13Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-03-15T11:52:53Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-03-15T20:45:34Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2023-03-15T22:03:10Z

clang-tidy review says "All clean, LGTM! 👍"

kitbarton

LGTM.

…yload`s (#68) This PR implements a `PayloadRegistry` factory as a template specialization of `plugin::registry::PluginRegistry<T>` (see #63). This registry can instantiate `PayloadInfo` items which, in turn, are responsible of instantiate concrete `Payload` implementations (same schema as `TargetSystem`). It also turns the `ZipPayload` into a pluggable `Payload` for this new registry. --------- Co-authored-by: Marius Hillenbrand <[email protected]>

jgsogo added 6 commits March 1, 2023 13:11

create base and targetsystem registries

e5ca821

forward arguments to constructors

e7cadac

remove legacy classes

b235e50

remove commented lines

859706d

fix tests

c236928

create object at startup (mock is not being registered)

472428e

jgsogo force-pushed the jgsogo-targetregistry-template branch from 01f49da to 472428e Compare March 1, 2023 15:40

kitbarton reviewed Mar 1, 2023

View reviewed changes

lib/HAL/TargetSystemInfo.cpp Outdated Show resolved Hide resolved

jgsogo added 2 commits March 2, 2023 09:18

remove copy ctors

80a1611

ensure Targets.inc is not optimized

75c552f

jgsogo added 3 commits March 2, 2023 13:21

recover some comments

03a5292

use 2 blanks to indent

0d1e778

Merge remote-tracking branch 'origin/main' into jgsogo-targetregistry…

8015cbe

…-template

jgsogo requested review from kitbarton and vrpascuzzi March 2, 2023 13:48

jgsogo changed the title ~~[WIP] Refactor TargetRegistry to a templatized PluginRegistry and concrete TargetSystemRegistry~~ Refactor TargetRegistry to a templatized PluginRegistry and concrete TargetSystemRegistry Mar 3, 2023

This was referenced Mar 3, 2023

Implement a PayloadRegister as a factory to register and create Payloads #68

Merged

[WIP] Adds a PayloadRegistry so we can plug-in payloads at runtime #58

Closed

Merge remote-tracking branch 'origin/main' into jgsogo-targetregistry…

243821e

…-template

vrpascuzzi suggested changes Mar 8, 2023

View reviewed changes

steleman suggested changes Mar 8, 2023

View reviewed changes

jgsogo commented Mar 8, 2023

View reviewed changes

review

e9f62f7

steleman suggested changes Mar 8, 2023

View reviewed changes

steleman reviewed Mar 8, 2023

View reviewed changes

This was referenced Mar 9, 2023

Doxygen comments and documentation - Add CI to generate it #72

Open

Enforce copyright notices at pre-commit or CI level #73

Open

steleman suggested changes Mar 9, 2023

View reviewed changes

jgsogo commented Mar 9, 2023

View reviewed changes

Merge branch 'main' into jgsogo-targetregistry-template

e05d0c0

github-actions bot reviewed Mar 15, 2023

View reviewed changes

include/Plugin/PluginRegistry.h Outdated Show resolved Hide resolved

include/Plugin/PluginRegistry.h Outdated Show resolved Hide resolved

lib/API/api.cpp Outdated Show resolved Hide resolved

clang-tidy

171c2ec

no need for default arg

db32996

Merge branch 'main' into jgsogo-targetregistry-template

8409f3c

fix format

5865586

kitbarton approved these changes Mar 17, 2023

View reviewed changes

kitbarton merged commit 87b0b56 into main Mar 17, 2023

vrpascuzzi deleted the jgsogo-targetregistry-template branch March 28, 2023 20:26


		~PluginInfo() = default;

		[[nodiscard]] llvm::StringRef getName() const { return name; }


		[[nodiscard]] llvm::StringRef getName() const { return name; }

		[[nodiscard]] llvm::StringRef getDescription() const { return description; }

Refactor TargetRegistry to a templatized PluginRegistry and concrete TargetSystemRegistry #63

Refactor TargetRegistry to a templatized PluginRegistry and concrete TargetSystemRegistry #63

Conversation

jgsogo commented Mar 1, 2023 • edited Loading

TODO:

kitbarton left a comment

Choose a reason for hiding this comment

jgsogo commented Mar 1, 2023

jgsogo commented Mar 2, 2023

vrpascuzzi 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

jgsogo Mar 8, 2023 • edited Loading

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

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

jgsogo left a comment

Choose a reason for hiding this comment

vrpascuzzi commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgsogo commented Mar 8, 2023

vrpascuzzi commented Mar 8, 2023

taalexander commented Mar 8, 2023

jgsogo commented Mar 8, 2023

taalexander commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgsogo Mar 9, 2023 • edited Loading

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

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

jgsogo left a comment

Choose a reason for hiding this comment

kitbarton commented Mar 9, 2023

steleman commented Mar 10, 2023

mtreinish commented Mar 10, 2023

github-actions bot left a comment

Refactor `TargetRegistry` to a templatized `PluginRegistry` and concrete `TargetSystemRegistry` #63

Refactor `TargetRegistry` to a templatized `PluginRegistry` and concrete `TargetSystemRegistry` #63

jgsogo commented Mar 1, 2023 •

edited

Loading

jgsogo Mar 8, 2023 •

edited

Loading

jgsogo Mar 9, 2023 •

edited

Loading