-
-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rework string handling and add writing capability with formatting. #3
Conversation
@@ -8,3 +8,4 @@ cmake-build-*/ | |||
|
|||
# Generated | |||
test/Helpers.h | |||
test/res/test_results/test.fgd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be generated in the CMake binary dir
|
||
//WRITE | ||
|
||
[[nodiscard]] sourcepp::detail::UtilStringView Write() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this return a "string view"? Can't it return a std::string
? Try not to expose things in detail
namespaces to consumers, the detail namespace is strictly for the library you're working in
bool selfOwning = false; | ||
public: | ||
|
||
UtilStringView(const char* str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not exist, the std::string_view
constructor is sufficent
~UtilStringView() = default; | ||
|
||
template<typename T> | ||
explicit UtilStringView(const UtilStringView& t){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rule of 3/5/0: if you implement copy and move constructors, you must implement copy and move assignment operators as well
#include <vector> | ||
#include <iostream> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused include
auto includeFilePath = std::string{dirPath} + '/' + match[1].str(); | ||
file.open(includeFilePath); | ||
exclusionList.push_back(currentPath); | ||
file.open(currentPath, std::ios::binary); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto (code from earlier)
@@ -486,7 +517,7 @@ bool FGD::ParseFile() { | |||
continue; | |||
} | |||
|
|||
if (iequals(iter->string, "@include")) { | |||
if (iequals(iter->string, R"(@include)")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are all these strings using raw string notation? They don't have any characters that would warrant it
|
||
#include "Helpers.h" | ||
|
||
#define quoted(str) #str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use raw strings not macros
test/res/test_results/.gitkeep
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I said earlier, move this folder to the cmake binary dir and make it autogenerated
return selfOwning; | ||
} | ||
|
||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire class can be replaced with a char pointer and a bool, and if implemented correctly it's exactly as safe as this class
Superseded by new FGD parser |
A whole lotta madness, may the void creatures lurking in the shadows be kind to you.