Closed stephenberry closed 2 years ago
I included my brace initilization updates in this pull request. However, I think these changes are obvious as they:
I've ran the included test with C++11 and passed
Latest changes address issue #4. It reduced code and configuration and passed the unit tests.
Note, I'm currently stacking my pull requests into one, because I can't build and test the code without my initial fixes. Let me know if you don't like a specific commit and we can discuss.
Just a few minor things:
auto
. I generally prefer to avoid using auto
unless there is a good reason to use it, like a very long type name (I can think of a const_iterator
from a custom defined std::map
, for example), in which case it can make the code more readable. But, in my opinion, using it instead of built-in types or types with a short name just makes the code a little more obscure.
switch
statement. I would prefer to keep the original order, starting with the 32-bit integer, since they were originally sorted by likelihood (at least that was my intention, although that can be a bit arbitrary). Regarding style/conventions, I noticed that you have left an space between the keyword and the (
, and that the brackets there are "Java style", with the openings being in the same line as their corresponding keywords. Not a big deal, but I would like to keep the code as homogeneous as possible.
Flag macros. Just a couple of style related notes: Java style brackets, and names using underscore separator instead of "camel case".
@arturocepeda, Thanks for all the great feedback and expressing your style intents (useful for keeping code easy to read). As thrilled as I am about this project, digging deeper into the code has uncovered more design concerns that would take a lot of refactoring (especially as it regards to how the lexer works in the compiler and the ability to easily optimize the AST). I'm feeling it is a better use of my time to continue my current implementation of a C++ scripting engine. However, my version heavily uses C++20. This offers so many benefits to performance and ease of use, but I understand if others are not able to use C++20 yet.
I wish I could focus on your implementation as well as my own, but alas I'm limited on time. I plan to open source my scripting engine as well once it is a little more mature, and I feel C++20 is going to quickly gain popularity.
Anyway, thanks for inspiring me with this awesome project, you've put a lot of excellent work into it!
Sincerely, Stephen
Hey Stephen!
While I'm aware that I surely am missing many potentially helpful features from the modern C++ standards, it has always been a key point to me to keep Cflat easy to integrate in any project, and I see raising the required C++ version as a step backward in that regard. But, as you really would like to benefit from C++20 features, and also, as it seems, there are fundamental things in Cflat that you would do differently, I guess your decision makes total sense and I understand it. Looking forward to seeing your scripting engine in action!
Still, I'm glad my project has inspired you, and your contributions and suggestions have already been really useful, I appreciate that! In fact, I would like to merge some of your commits and implement some of the things you suggested, if you don't mind. And, of course, any future contribution will be more than welcome, if you find the time.
Cheers!
Minor fix to include
cmath
header forfabs
and usingstd::fabs
. Necessary to compile with Apple clang.