Mabofoul/step by step#4
Conversation
…s of header files, removed boost dependancy, removed/added spaces. included @xtrm-en work of PR #1, except his makefile (his makefile is better but i wont copy others work)
general improvement
ordering CON_TOKENTYPE handlers/types like ordered in enum CON_TOKENTYPE
The reason: i want to push the changs one step at a time to make it easier to review.
src/*: whitespaces src/construct.cpp: added a note you explained in the youtube video README.md: added eexplaination for "-o" flag
…he enums in src/construct_types.h
warning 2: functions missing return statements warning 3: for loop index is of type int but is compared to a value of type size_t
…t. also it is used in src/construct_flags.cpp so i added declaration. con_type looks like the token types while also being only used in the file itself so i renamed to _con_type. apply_macro_to_token() is used only in the file itself, so i set it as static and removed it from the header.
cstring is included for strcmp. so I replaced it with std::string comparison. boost is used for split, is_any_of, and to_lower. so I implemented the functionality we need.
… the types and functions you directly use, and not depend on the current header files to complete the needed includes. for example: the file src/construct_flags.h only needs to know the type std::string, so only #include <string>, and since the function implementation appears in src/construct_flags.cpp, it also needs an #include <string>. you should put that same include in both .h and .cpp files, since technically the .h file can be empty, then it won't have includes for arguments types, the .cpp has to do the includes. explaination for the importance of this rule: i remember something like this: <string> under one compiler / operating system does #include <algorithm> inside the header, so the program in the story ran filne without noticing the need of direct #include <algorithm>. once the same code was compiled with a different compiler / OS, it failed compilation since <algorithm> was not included.
…hrow error;". before, an error could go unnoticed, but now the program will end automatically (I didn't do try-catch)
Mabofoul/step by step
…rax' in factorial.asm
move empty line from top of output file to bottom
…parser fix parser breaking from whitespaces
windows adds exe to program name so makefile alywas recompiles
fixed extra newlines
|
Conversation regarding this PR is under PR #3 |
Thomas-995
left a comment
Thomas-995
left a comment
There was a problem hiding this comment.
Please try compiling and assembling the construct code before committing. In "strchr" findchr is macrod to sil because we cannot compare a single byte with a 64 bit register, which is why findchr is macrod to the 8 bit region of the register. This is also why when assembling it will give strchr.asm:10: error: mismatch in operand sizes
|
Sorry, I did not think of that! I will revert this change. Also, about the "split takes alot of lines" issue, in a next PR im moving it down: class IsAnyOf { template |
Thomas-995
dismissed
their stale review
If the changes to the construct code can be removed, as well as the removal of boost I can merge it. (Sorry accidentlly approved already)
I think as long ONLY the required string functions are added and they're placed neatly so as not to cause confusion it might be okay to just fit it into the deconstruct file. I would like it if you did not make them template functions however. I'll take a look at the next PR. |
|
also, i have just noted reg_to_str() is used only in reconstruct.cpp, so it should be static. |
|
reg_to_str being static makes sense yeah since it will likely only ever be used to reconstruct code so I like that yes. My main issue with making functions that are only used in their respective source file static is that then we would have to do it for all of them, but if you are up to that I don't mind. Also I am aware that this isn't the usual style, I do not have this habit from js haha. I just think having a curly brace on its own seperate line makes sense when closing a function, as it's the only indicator (besides indentation) that a function has ended. But for opening a function you already have the function decleration as a signal that a new function has been opened. It's just preference but I see no reason to change it for everything. |
|
Also I really like how you did the string functions here. Instead of cluttering the header file which is supposed to be a nice overview of functionality, you just quickly declared them at the start of the cpp file. I'll look over it properly tommorow, but this is good. |
|
Though why wouldn't you just make the string split function return a pointer to the result? |
I did that because I was mimicking boost::split, and I didnt think of doing it as you say. about code style, i will keep up with your style, since you started all of that. If I were the owner, I would run my style (also, indentation is 4 spaces), so now I cede this matter to you, with full acceptance in-sha-allah. |
strchr.con: reverted wrong change - findchr is 8bit while chr is 64bit, so they are different reg_to_str: changed back to use bitwidth as argument, and made static split: better args
use syscalls like functions, and pass arguments to function/syscall correctly
|
I see a problem in parsing commands with memory addresses, like "mov eax, dword ptr [ebx]". |
|
another thing: why do you do to_lower() to every line you parse? it makes strlwr.con example store the string already in lower case, which proves the action is wrong. also, there is no colon after "teststring". isn't that wrong? |
src/construct.cpp: free allocated memory src/deconstruct.cpp: remove to_lower() function and usage src/reconstruct.cpp: same apply-macro logic but better code examples/*.con: using macros for magic numbers examples/*.asm: showing the result of the example inputs
better apply macro
I'm unable to commit on this PR, because I have other commits locally and pushing them means you will review them and delay merging this branch. |
|
Pulled and compiled the branch, running construct on the example strlwr.con gives a SIGSEGV (Address boundary error). And the name of the compiled executable is construct.exe, it should be dependant on the OS. (Linux executables don't have an extension.) |
|
regarding construct.exe, windows must have the .exe even if you dont write any extention, while linux isn't affected with the extension, so i did put the extention. you can roll with it and fix that later if you know (it is not a "fix" since there is no problem, you just prefer the name under linux be "construct") . |
|
now everything works: i ran your original master on the 3 examples, and ran my PR on these examples, and compared the output files (didnt run them, and need to). the files are the same, which means that my code is a step forward. |
|
@Thomas-de-Bock please see the last update |
|
I'll take a look at it tomorrow. Also for the exe extension thing, that's not up for debate. It only had no file extension before because the build was never meant for windows. The exe extension is exclusive to windows executables and linux binaries have no extension, this is not my preference it is the general consensus. Compiling a program currently exclusively made for use on Linux to a file with the windows executable extension is very much a problem. I'll merge it if that's fixed and everything else works. |
|
"I'll merge it if that's fixed"... why not merge it then remove the .exe? |
2 participants
the 5th commit (c78612c) is a revert to start again from your master. I did so to push again the changes I have done but make each step visible by giving it its own commit. now it is easier for you to check file comparisons.
regarding the utility function split() that I added, I didnt test it, but I ran the whole program on the examples and it worked.