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

Fix base::get_absolute_path() fixing base::normalize_path() function #120

Merged
merged 1 commit into from
Dec 26, 2024

Conversation

Gasparoken
Copy link
Member

Copy link
Collaborator

@aseprite-bot aseprite-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

base/fs.cpp Outdated
if (part == ".") {
last_dot = true;
std::vector<std::string> fn_parts;
bool has_root = (!_path.empty() && _path[0] == path_separator);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'has_root' of type 'bool' can be declared 'const' [misc-const-correctness]

Suggested change
bool has_root = (!_path.empty() && _path[0] == path_separator);
bool const has_root = (!_path.empty() && _path[0] == path_separator);

base/fs.cpp Show resolved Hide resolved
Copy link
Collaborator

@aseprite-bot aseprite-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

base/fs.cpp Outdated
if (!path.empty() && path[0] == path_separator) {
fn.push_back(path_separator);
std::vector<std::string> fn_parts;
bool has_root = (!path.empty() && path[0] == path_separator);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'has_root' of type 'bool' can be declared 'const' [misc-const-correctness]

Suggested change
bool has_root = (!path.empty() && path[0] == path_separator);
bool const has_root = (!path.empty() && path[0] == path_separator);

base/fs.cpp Outdated
fn.push_back(path_separator);
std::vector<std::string> fn_parts;
bool has_root = (!path.empty() && path[0] == path_separator);
bool last_dot = (parts.back() == ".");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'last_dot' of type 'bool' can be declared 'const' [misc-const-correctness]

Suggested change
bool last_dot = (parts.back() == ".");
bool const last_dot = (parts.back() == ".");

base/fs.cpp Outdated
std::vector<std::string> fn_parts;
bool has_root = (!path.empty() && path[0] == path_separator);
bool last_dot = (parts.back() == ".");
bool last_slash = parts.back().empty();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'last_slash' of type 'bool' can be declared 'const' [misc-const-correctness]

Suggested change
bool last_slash = parts.back().empty();
bool const last_slash = parts.back().empty();

Copy link
Member

@martincapello martincapello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just left a couple of minor comments.

base/fs.cpp Outdated
// Skip each dot part.
if (part.empty() || part == ".")
continue;
else if (part == "..") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "else" word can be removed here because you are calling "continue;" inside the previous if.

Suggested change
else if (part == "..") {
if (part == "..") {

@@ -281,13 +281,36 @@ std::string normalize_path(const std::string& _path)
// Replace multiple slashes with a single path_separator.
std::string path = fix_path_separators(_path);

std::string fn;
fn.reserve(path.size());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't remove the call to reserve, it is used to reserve memory beforehand and avoid having to resize the buffer when the string gets bigger.

@aseprite-bot
Copy link
Collaborator

clang-tidy review says "All clean, LGTM! 👍"

@dacap dacap assigned Gasparoken and unassigned dacap Dec 26, 2024
@dacap
Copy link
Member

dacap commented Dec 26, 2024

It looks like the precommit failed https://github.com/aseprite/laf/actions/runs/12483083166/job/34838239516#step:4:120

base::get_absolute_path() resulted in bugs when more than two or more
double dots were present in the input path.
The source of the error was the base::normalize_path() function
which has been fixed.
Also added tests in 'fs_tests.cpp'.
This fix is part of the solution of aseprite/aseprite#4851.
@aseprite-bot
Copy link
Collaborator

clang-tidy review says "All clean, LGTM! 👍"

@Gasparoken Gasparoken assigned dacap and unassigned Gasparoken Dec 26, 2024
@dacap dacap merged commit 266f050 into aseprite:main Dec 26, 2024
9 checks passed
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.

4 participants