Closed mundusnine closed 5 months ago
Hi @mundusnine!
I needed to support \" in my const char* string literals. The behavior was weird with my strings since we just replace anything other then the supported chars with \. For now I simply added the most common/simple of the escape chars. Maybe we should determine if we will support all or what would be the acceptable subset to support.
That's great, thanks! The only reason why '\n' was the only supported is because so far nobody had missed others, but I agree, it's a good thing to support those, too. I would say that now, with your contribution, the supported subset is more than enough.
I think it would maybe be pertinent to send an error or a general message to indicate when one escape char isn't supported ? Maybe it doesn't matter.
I agree, using an unsupported escape sequence should be an error. Will add that quickly after merging the pull request.
I added a test case for the escape chars. I don't know what would be the acceptable granularity for you(i.e. do we test everything that we support ?). Regarding the tests.cpp, their isn't a main function, so I guess you add this file to another in your other project to run the tests ? It doesn't really matter but maybe we could add a test_main.cpp file to the tests folder so , users that are exclusive to Cflat could just add that file to their compilation setup without adding the main function manually all the time in tests.cpp. Here is what I am thinking:
Although not everything is covered by tests, I always try to add a new one (or extend an existing one) whenever I add something or fix a bug, so the test you wrote is definitely a good addition! The reason why there isn't a main function for the tests is because I use a local Google Test project in Visual Studio, which handles the execution of the tests automatically. I just preferred not to commit the VS project to the repo because not everybody uses VS (as in your case), but having your local main file should do the trick for you to make sure that nothing breaks with new additions or changes in the code.
Just one minor thing: could you please switch to Allman indentation to match the code style used in Cflat? Then I'll gladly merge the pull request.
Thank you for your contributions, very much appreciated!
Hi @arturocepeda , I made the changes you asked for. Also, thank you for the clarifications.
Good day !
I needed to support
\"
in my const char* string literals. The behavior was weird with my strings since we just replace anything other then the supported chars with\\
. For now I simply added the most common/simple of the escape chars. Maybe we should determine if we will support all or what would be the acceptable subset to support.I think it would maybe be pertinent to send an error or a general message to indicate when one escape char isn't supported ? Maybe it doesn't matter.
I added a test case for the escape chars. I don't know what would be the acceptable granularity for you(i.e. do we test everything that we support ?). Regarding the tests.cpp, their isn't a main function, so I guess you add this file to another in your other project to run the tests ? It doesn't really matter but maybe we could add a
test_main.cpp
file to the tests folder so , users that are exclusive to Cflat could just add that file to their compilation setup without adding the main function manually all the time in tests.cpp. Here is what I am thinking:Sorry for the wall of text. Good day, thanks for this cool and useful project !