Open
Conversation
Member
|
@tteschemacher this duplicates #6016, please add your modifications there |
loumalouomega
requested changes
Jun 1, 2020
Member
loumalouomega
left a comment
loumalouomega
left a comment
There was a problem hiding this comment.
Blocking as it duplicates #6016
Contributor
Author
|
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? |
Member
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 |
Member
|
@tteschemacher this duplicates #6016... |
Contributor
Author
|
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. |
Member
|
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
| Model& rModel, | ||
| Parameters ThisParameters | ||
| ) | ||
| const Parameters ThisParameters |
Member
There was a problem hiding this comment.
Minor: this cannot be const otherwise one cannot get the validated parameters
Contributor
Author
There was a problem hiding this comment.
shouldn't it be stored and then updated by the validated parameters?
Member
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
Contributor
Author
There was a problem hiding this comment.
but then additionally it would need to be source as well.
Member
There was a problem hiding this comment.
ValidateAndAssingDefaults modifyes the incoming object
Member
|
Do I close #6016? |
Member
|
I like the prototype of @pooyan-dadvand, I have two comments:
|
Member
|
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. |
Member
|
I realized the design error from #7426 is also here @pooyan-dadvand, do you still want to proceed with this design? |
Member
|
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? |
Member
|
It looks like a linking problem, but i am unable to reproduce it
El mié., 18 nov. 2020 16:39, Pooyan Dadvand <notifications@github.com>
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>
.
|
Member
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) |
Member
|
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. |
Member
|
OK, The missing dlls are: |
Member
|
It seems that there is a cyclic dependency somewhere in the chain which makes the dependency walker crazy. |
Member
|
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. |
Member
|
Testing same as #7426 |
Member
|
BTW, I am not using cotire in my machine and can reproduce the problem.... Seems that something is wrong with the compilation |
Member
Huum, I did some other modifications in #7426, but after all this time I do not remember what... |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.