[Core] Add register process #7004
Open
+170
−52
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@tteschemacher this duplicates #6016, please add your modifications there |
loumalouomega
requested changes
Jun 1, 2020
|
sorry @loumalouomega I was not aware of the other PR. But this one has the actually intersting additional features. I either case, from the discussions I see that this is anyways not wanted, right? |
I would propose to move those additional features to #6016. In any case i would avoid if possible to create a factory, or avoid follow the current factory strategy, as we have discussed in #3185 simpler alternatives. #6016 is much simpler than this one |
|
@tteschemacher this duplicates #6016... |
|
No worries @loumalouomega . I am trying to do the suggested way, here, to avoid all REGISTERs in the kratos_application. I need to test some stuff as it is not as trivial. I do not want to pollute your branch. |
|
I have just added a prototype of the mechanism for the automatic registry. It works and there is a test for it. Nevertheless, I should move the mechanism to a new Registry class and add the corresponding macros and functions there. The idea is to have the registries in each process and avoiding a central file (like application) to do that. |
philbucher
reviewed
Sep 29, 2020
| @@ -92,8 +95,8 @@ class Process : public Flags | |||
| */ | |||
| virtual Process::Pointer Create( | |||
| Model& rModel, | |||
| Parameters ThisParameters | |||
| ) | |||
| const Parameters ThisParameters | |||
There was a problem hiding this comment.
Minor: this cannot be const otherwise one cannot get the validated parameters
There was a problem hiding this comment.
shouldn't it be stored and then updated by the validated parameters?
There was a problem hiding this comment.
the idea is that one can create the object, and then the parameters that were used in the creation contain the full set of parameters
There was a problem hiding this comment.
but then additionally it would need to be source as well.
There was a problem hiding this comment.
ValidateAndAssingDefaults modifyes the incoming object
|
Do I close #6016? |
|
I like the prototype of @pooyan-dadvand, I have two comments:
|
|
I thought I have answered you @philbucher For the first comment, It can be used for the templated classes, and in fact I have used a simple template process for that reason. Nevertheless, being templated affects the creation of the prototypes that should be created with the concrete template arguments. This is very similar to what we do in our pybind export that we define the arguments and pass them to pybind to register it for python. About the second one I like your idea very much and I agree with you that this would be a great improvement. I also consider another alternative which would be to have our interface wich mimics the pybind one and then calling once and do both registeration and python interface. |
|
I realized the design error from #7426 is also here @pooyan-dadvand, do you still want to proceed with this design? |
|
I don't think that this is a design error. Seems to be an implementation error. @roigcarlo Isn't it a linking exposure problem? BTW, I don't understand the CLang one. It is just giving a segfault but where? |
|
It looks like a linking problem, but i am unable to reproduce it
El mié., 18 nov. 2020 16:39, Pooyan Dadvand <[email protected]>
escribió:
… I don't think that this is a design error. Seems to be an implementation
error. @roigcarlo <https://github.com/roigcarlo> Isn't it a linking
exposure problem?
BTW, I don't understand the CLang one. It is just giving a segfault but
where?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7004 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAOYTL52FKSDZRUCPFP64RTSQPTE7ANCNFSM4NPPKLPQ>
.
|
I am, with the last version of VisualStudio 2019 + last update of Windows (before I updated Winodws in my Vm it worked, so it something related with some MS implementation) |
|
So finally I could reproduce the problem in my laptop. Now the problem is that dependency walker cannot load the kratos_run.exe so I can not figure out why it is failing. |
|
OK, The missing dlls are: |
|
It seems that there is a cyclic dependency somewhere in the chain which makes the dependency walker crazy. |
|
I don't know how we got dependencies to this dlls, but seems that we should not depend to them as they are very internal dlls of windows. |
|
Testing same as #7426 |
|
BTW, I am not using cotire in my machine and can reproduce the problem.... Seems that something is wrong with the compilation |
Huum, I did some other modifications in #7426, but after all this time I do not remember what... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi all,
this allows to register cpp processes and use them within the analysis_stages. This allows to avoid the python wrappers.
After #3185 is merged the factory can be removed again. However, python names will be kept similar.