search

Fattorino / ImNodeFlow

Node based editor/blueprints for ImGui
MIT License
119 stars 15 forks source link

Fixed compiler warnings #19

Closed biglari closed 3 months ago

biglari commented 3 months ago

This is my first pull request on github.

I was receiving some warnings for missing virtual destructors in the BaseNode. I added virtual destructors and fixed some other warnings.

fenix31415 commented 3 months ago

Great!

Looks like you uses tabs, while the author uses spaces.

Also I had those errors/warnings.

biglari commented 3 months ago

Great!

Looks like you uses tabs, while the author uses spaces.

Also I had those errors/warnings.

Thanks for letting me know, fixed the tabs!

Fattorino commented 3 months ago

Could you check the order of the parameters makes sense everywhere, following the same logic where you go from more relevant to less relevant (hence m_inf almost always at the end)? Also please check that the private class members follow the same order too. It's something I always wanted to check but never got around to do.

biglari commented 3 months ago

Could you check the order of the parameters makes sense everywhere, following the same logic where you go from more relevant to less relevant (hence m_inf almost always at the end)? Also please check that the private class members follow the same order too. It's something I always wanted to check but never got around to do.

I did not receive any other warnings regarding order missmatch. In regards to the order of the parameters from most to least relevant, I am not sure how to qualify that. I think consistency is the most important. In that case, I see for example that parameters in BaseNode are ordered : https://github.com/Fattorino/ImNodeFlow/blob/ce0cafc1c7ca859a8eaeba34c79580197a300175/include/ImNodeFlow.h#L616 There seems to be some inconsistency here between the BaseNode class and the InPin class whose constructor use following order: https://github.com/Fattorino/ImNodeFlow/blob/ce0cafc1c7ca859a8eaeba34c79580197a300175/include/ImNodeFlow.h#L1067 here the Filter parameter is placed out of order. Should I change the order of the parameters to be consistent between all functions, and if so, do you have a preferred order?

Fattorino commented 3 months ago

Yes, that's exactly what I'm talking about, in this case, I prefer defReturn before filter. If you find any other inconsistency feel free to reorder them trying to follow my previous preferences but with some creative freedom on your part

Fattorino commented 3 months ago

Are all inconsistencies taken care of now?

biglari commented 3 months ago

Are all inconsistencies taken care of now?

Yes, everything should be taken care of now. There are very few functions that take multiple parameters. I was waiting to mark my work as completed as I was contemplating if it made sense to swap some Parameters around, but I think in the end I think I was overthinking it.

Fattorino commented 3 months ago

@biglari Unfortunately, I have merged the commits without testing them, next time please make sure the code works or at least compiles before submitting the final PR. You rearranged the constructor but not the parameters where the constructor is used.

biglari commented 3 months ago

@biglari Unfortunately, I have merged the commits without testing them, next time please make sure the code works or at least compiles before submitting the final PR. You rearranged the constructor but not the parameters where the constructor is used.

Sorry about that, I should have set up an isolated environment so I could test the version I uploaded here. Hope I did not waste too much of your time.