Update superscalar.cpp

[FreeTrade]

Received this suggestion from @anshulmttl to address problem I was having at

https://github.com/tevador/RandomX/issues/270

It seems to compile and run fine, although I'm unsure if it could have any side effects.

Comment ...

The static members were not being initialized so i have initialized them properly now.

I found that SupercalarInstruction::Null is not being initialized whereas the SuperscalarInstructionInfo::NOP is initialized just fine.

So i have made changed to SuperscalarInstruction to initialize SuperscalarInstruction::Null properly

[SChernykh]

I found that SupercalarInstruction::Null is not being initialized whereas the SuperscalarInstructionInfo::NOP is initialized just fine.

According to C++ standard, both must be initialized properly, so you're fixing non-existing bug which is caused by your incorrect compilation setup. You should fix your CMake files instead.

[anshulmttl]

I found that SupercalarInstruction::Null is not being initialized whereas the SuperscalarInstructionInfo::NOP is initialized just fine.

According to C++ standard, both must be initialized properly, so you're fixing non-existing bug which is caused by your incorrect compilation setup. You should fix your CMake files instead.

In software engineering we don't define a bug as non existent. A bug is found only after it has been tested in various environments and not just unit test cases designed to work. So BCHUnlimited is clearly a new environment for your software.

[SChernykh]

Of course the bug exists, in BCHUnlimited makefiles. It's just not in this cpp file. You're fixing a symptom, not the actual problem. How can you know that nothing else is broken because of that? So this why I'm saying that this PR will not be merged.

P.S. I'll repeat again - according to C++ standard, your fix will do nothing because info_ will never be nullptr here

[anshulmttl]

As per C++ standard the classes should be declared in header files and defined in C++ files but your code does not follow coding standards.

[anshulmttl]

Of course the bug exists, in BCHUnlimited makefiles. It's just not in this cpp file. You're fixing a symptom, not the actual problem. How can you know that nothing else is broken because of that? So this why I'm saying that this PR will not be merged.

P.S. I'll repeat again - according to C++ standard, your fix will do nothing because info_ will never be nullptr here

Your constructor is private so as per C++ standard your class cannot have any instance of SuperscalarInstruction except for SuperscalarInstruction::Null or info = SuperscalarInstructionInfo::NOP. info cannot be null ptr and infact you cannot even initialize class SuperscalarInstruction because it is private constructor. Check your C++ concepts.

[SChernykh]

As per C++ standard the classes should be declared in header files and defined in C++ files but your code does not follow coding standards.

Coding standards are not C++ standard. If this code broke C++ standard, it wouldn't compile at all. Classes can be declared even inside functions (although I don't support it except for very few use cases), if we talk about it.

info_ cannot be null ptr and infact you cannot even initialize class SuperscalarInstruction because it is private constructor. Check your C++ concepts.

So? Why do you check for nullptr in this PR then? Don't you see a contradiction here?

Your constructor is private

I didn't write that code, check git blame

[tevador]

As per C++ standard the classes should be declared in header files and defined in C++ files but your code does not follow coding standards.

Can you link to the relevant section of the C++ standard?

This PR won't be merged unless you can show that the code actually violates the specs. The fact that your code crashes is not a proof. There can be other reasons for that such as: a compiler bug or an incorrect build setup. This code has been audited and used for 4 years without issues on various platfoms and nobody has reported a problem.

I'm unsure if it could have any side effects.

A "side effect" could cause unintentional hard forks in the cryptocurrencies that use RandomX (and there are many), so any attempts to fix bugs must be done very carefully in this repository.

[SChernykh]

Even if there is a bug in this repository, it should be most likely fixed in CMakeLists.txt, and not in this .cpp file. But I give 99% chance it's something wrong with BCHUnlimited makefiles, or the way you try to integrate RandomX there.

[anshulmttl]

As per C++ standard the classes should be declared in header files and defined in C++ files but your code does not follow coding standards.

It is a general concept.

You have used inline functions in your code which is not good as per c++ standard. You have declared class in cpp file in instead of header file which is not good as per coding standards. Searching on google for these 2 will give you ample amount of information. Just for reference here is first reference i found. https://www.learncpp.com/cpp-tutorial/class-code-and-header-files/

[SChernykh]

Based on your comments, you don't even understand what is "C++ standard" and what is "Coding guide". Please don't try to teach us here while you yourself forgot how to write a proper copy constructor.

[tevador]

It is a general concept.

Header files declare public interfaces that are accessible across translation units. The class SuperscalarInstruction is an implementation detail of the public generateSuperscalar function and it not meant to be visible outside of superscalar.cpp.

[SChernykh]

RandomX uses C++14: https://www.iso.org/standard/64029.html - this is THE standard I was talking about. Not some random articles on learncpp. Unfortunately one can't get the final text of the standard for free, here's the latest free version: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4296.pdf

[anshulmttl]

Based on your comments, you don't even understand what is "C++ standard" and what is "Coding guide". Please don't try to teach us here while you yourself forgot how to write a proper copy constructor.

See it is written in 1st line of google why not to use inline functions :) https://www.google.com/search?q=issues+with+using+inline+functions&sca_esv=556671154&sxsrf=AB5stBhFiuZqPNrzPM3OncLxS3O41EBGaQ%3A1691998625085&ei=odnZZN3jBIOjseMPmt6YuAQ&ved=0ahUKEwiduIiI0tuAAxWDUWwGHRovBkcQ4dUDCBA&uact=5&oq=issues+with+using+inline+functions&gs_lp=Egxnd3Mtd2l6LXNlcnAiImlzc3VlcyB3aXRoIHVzaW5nIGlubGluZSBmdW5jdGlvbnMyBRAhGKABMggQIRgWGB4YHTIIECEYFhgeGB1IzSxQiAdYvitwA3gBkAEAmAGmAaAB3R6qAQQ0LjMwuAEDyAEA-AEBwgIKEAAYRxjWBBiwA8ICBxAjGIoFGCfCAgQQIxgnwgIIEAAYigUYkQLCAgsQABiABBixAxiDAcICBRAuGIAEwgILEC4YigUYsQMYgwHCAhEQLhiABBixAxiDARjHARjRA8ICERAuGIAEGLEDGIMBGMcBGK8BwgIKEAAYigUYsQMYQ8ICBxAAGIoFGEPCAgcQLhiKBRhDwgINEC4YigUYsQMYgwEYQ8ICBRAAGIAEwgIOEC4YgAQYsQMYxwEY0QPCAggQABiABBixA8ICChAAGIAEGEYY_wHCAgoQABiABBgUGIcCwgIGEAAYFhgewgIHECEYoAEYCsICBBAhGBXCAgoQIRgWGB4YDxgd4gMEGAAgQYgGAZAGCA&sclient=gws-wiz-serp

And see it is written on google first line why to declare class in header files. https://www.google.com/search?q=why+to+declare+class+in+header+file&sca_esv=556671154&sxsrf=AB5stBhnkIvBLzyL2D6s3DBoMyPsNuNC-A%3A1691998644873&ei=tNnZZP3qNNSgseMPhYCHqAo&ved=0ahUKEwj9lMCR0tuAAxVUUGwGHQXAAaUQ4dUDCBA&uact=5&oq=why+to+declare+class+in+header+file&gs_lp=Egxnd3Mtd2l6LXNlcnAiI3doeSB0byBkZWNsYXJlIGNsYXNzIGluIGhlYWRlciBmaWxlMgcQIRigARgKMgcQIRigARgKMggQIRgWGB4YHUiiNVDFBVizNHADeAGQAQCYAYcCoAGzIqoBBjcuMjUuMrgBA8gBAPgBAcICChAAGEcY1gQYsAPCAgcQIxiKBRgnwgINEC4YigUYsQMYgwEYQ8ICCBAAGIoFGJECwgINEAAYigUYsQMYgwEYQ8ICCxAAGIAEGLEDGIMBwgIEECMYJ8ICBxAAGIoFGEPCAggQLhiABBixA8ICCxAAGIoFGLEDGIMBwgILEC4YgAQYsQMYgwHCAgUQABiABMICCxAuGIoFGLEDGIMBwgIFEC4YgATCAgoQABiKBRixAxhDwgIIEAAYgAQYsQPCAgoQABiABBgUGIcCwgIGEAAYFhgewgIIEAAYFhgeGA_CAggQABgIGB4YDcICCBAAGIoFGIYDwgIFECEYoAHCAgoQIRgWGB4YDxgdwgIEECEYFeIDBBgAIEGIBgGQBgg&sclient=gws-wiz-serp

[SChernykh]

See it is written in 1st line of google why not to use inline functions :) And see it is written on google first line why to declare class in header files.

I learned 0 bits of new information from these links. Inline functions are perfectly fine for their use cases. Class definitions not in header files are also perfectly fine for their use cases. Also, read 9.8 Local class declarations from the pdf I linked above - what I was talking about. Class definitions can be even inside functions.

[anshulmttl]

I don't want to fight on this context again and again. I have just sent you the text which is available at google. I don't want any further discussion on this. You can decide on your own after reading this google articles.

[anshulmttl]

Please check tevador's reply. He has given the correct reply to my point. It is your design consideration. coding standard in C++ says it is better to write class declaration in header files, it does not say that class definitions cannot be written in CPP files or inside functions.

[SChernykh]

And my point is that this whole PR and conversation shouldn't have existed at all. The bug is somewhere else. Also, your point is not related to this PR at all, we're here not to discuss best coding practices, but to discuss one specific issue that @FreeTrade has.

Speaking of, I still didn't get an answer to my question

So? Why do you check for nullptr in this PR then? Don't you see a contradiction here?

[FreeTrade]

Appreciate all the comments and discussion on this. Looks like it won't be merged, but would like to leave it somewhere in case any one else runs into the same issue and this proves useful for them.

[FreeTrade]

Note: On further inspection - while this change allowed my code to compile and run in the build system, it only worked with very few flags set and produced incorrect hashes. Recommend against using.