Skip to content
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

Closed

Conversation

Trico-Everfire
Copy link
Contributor

A whole lotta madness, may the void creatures lurking in the shadows be kind to you.

@@ -8,3 +8,4 @@ cmake-build-*/

# Generated
test/Helpers.h
test/res/test_results/test.fgd
Copy link
Owner

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;
Copy link
Owner

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)
Copy link
Owner

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){
Copy link
Owner

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>
Copy link
Owner

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);
Copy link
Owner

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)")) {
Copy link
Owner

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
Copy link
Owner

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

Copy link
Owner

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;
}

};
Copy link
Owner

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

@craftablescience
Copy link
Owner

Superseded by new FGD parser

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants